Page MenuHomePhabricator

marcin (Marcin)
User

Projects

User does not belong to any projects.

User Details

User Since
Dec 29 2021, 2:54 AM (90 w, 3 d)

Recent Activity

Tue, Sep 19

marcin updated the diff for D9199: Encrypt all notifications on the keyserver for Android devices.

Rebase before landing

Tue, Sep 19, 9:05 AM
marcin updated the diff for D9198: Enable encrypted notifications coalescing on in Androoid native code..

Rebase before landing

Tue, Sep 19, 9:04 AM
marcin updated the diff for D9149: Encrypt all notifications on the keyserver for iOS devices.

Rebase before landing

Tue, Sep 19, 9:04 AM
marcin updated the diff for D9144: Enable encrypted notification coalescing in NSE on iOS.
  1. Don't wait forever
  2. Rebase before landing
Tue, Sep 19, 9:03 AM
marcin added inline comments to D9144: Enable encrypted notification coalescing in NSE on iOS.
Tue, Sep 19, 8:50 AM
marcin closed D8730: Implement displaying error message notification for cases that might cause empty notification on Android.
Tue, Sep 19, 7:56 AM
marcin committed rCOMM53d447a72366: Implement displaying error message notification for cases that might cause… (authored by marcin).
Implement displaying error message notification for cases that might cause…
Tue, Sep 19, 7:56 AM
marcin added inline comments to D8730: Implement displaying error message notification for cases that might cause empty notification on Android.
Tue, Sep 19, 7:38 AM
marcin updated the diff for D8730: Implement displaying error message notification for cases that might cause empty notification on Android.

Some last changes and rebase before landing

Tue, Sep 19, 7:37 AM
marcin added inline comments to D8730: Implement displaying error message notification for cases that might cause empty notification on Android.
Tue, Sep 19, 7:33 AM
marcin edited reviewers for D9224: [native] commCoreModule methods for getting, setting, and clearing the commServicesAccessToken, added: bartek, kamil; removed: marcin.

I am leaving for vacation so I don't want to block this diff. I am adding two reviewers:

  1. @kamil for changes related to data clearance. I didn't add him as a blocking reviewer since he might be absent for a longer time. But if he is back quickly I would appreciate his review on the data clearance.
  2. @bartek for changes related to putting entire authentication payload in Secure Store at once.
Tue, Sep 19, 7:19 AM
marcin updated the summary of D8730: Implement displaying error message notification for cases that might cause empty notification on Android.
Tue, Sep 19, 6:57 AM
marcin updated the diff for D8730: Implement displaying error message notification for cases that might cause empty notification on Android.
  1. Take advantage of builder mutability.
  2. Use streams instead of iteration.
  3. Don't iterate twice over notifications id list when removing a notification id

THos should cover all comments from Tomek last review

Tue, Sep 19, 6:33 AM
marcin added inline comments to D8730: Implement displaying error message notification for cases that might cause empty notification on Android.
Tue, Sep 19, 5:23 AM
marcin updated the diff for D8730: Implement displaying error message notification for cases that might cause empty notification on Android.

Move isStaffRelease checks to better places

Tue, Sep 19, 4:43 AM
marcin updated the summary of D9144: Enable encrypted notification coalescing in NSE on iOS.
Tue, Sep 19, 3:55 AM
marcin requested review of D9144: Enable encrypted notification coalescing in NSE on iOS.
Tue, Sep 19, 3:53 AM
marcin updated the diff for D9144: Enable encrypted notification coalescing in NSE on iOS.

Use local notifications for collapsible notifications to achieve previous UX experience.

Tue, Sep 19, 3:53 AM

Mon, Sep 18

marcin updated the diff for D8730: Implement displaying error message notification for cases that might cause empty notification on Android.

Set auto cancel property to false only for staff releases

Mon, Sep 18, 8:21 AM
marcin planned changes to D8730: Implement displaying error message notification for cases that might cause empty notification on Android.
Mon, Sep 18, 6:46 AM
marcin updated the diff for D8730: Implement displaying error message notification for cases that might cause empty notification on Android.

Refactor using isStaffRelease utility.

Mon, Sep 18, 6:15 AM
marcin accepted D8929: [keyserver] modify createPickledOlmSession.
Mon, Sep 18, 2:47 AM

Fri, Sep 15

marcin closed D9179: Save encrypted payload hash in MariaDB keyserver for iOS notifs..
Fri, Sep 15, 7:23 AM
marcin committed rCOMM2c5c658b3434: Save encrypted payload hash in MariaDB keyserver for iOS notifs. (authored by marcin).
Save encrypted payload hash in MariaDB keyserver for iOS notifs.
Fri, Sep 15, 7:23 AM
marcin closed D9178: Enhance error check in CryptoModule decrypt. Add encrypted message hash to error message..
Fri, Sep 15, 7:23 AM
marcin committed rCOMM6b660b669592: Enhance error check in CryptoModule decrypt. Add encrypted message hash to… (authored by marcin).
Enhance error check in CryptoModule decrypt. Add encrypted message hash to…
Fri, Sep 15, 7:23 AM
marcin updated the diff for D9179: Save encrypted payload hash in MariaDB keyserver for iOS notifs..

Rebase before landing

Fri, Sep 15, 6:57 AM
marcin updated the diff for D9178: Enhance error check in CryptoModule decrypt. Add encrypted message hash to error message..

Rebase before landing

Fri, Sep 15, 6:56 AM
marcin updated the diff for D9149: Encrypt all notifications on the keyserver for iOS devices.

Enhance variable naming

Fri, Sep 15, 5:32 AM
marcin updated the diff for D9179: Save encrypted payload hash in MariaDB keyserver for iOS notifs..
  1. Update typing
  2. Save device token to hash mapping instead of plain hash list.
Fri, Sep 15, 5:11 AM

Thu, Sep 14

marcin accepted D9201: [keyserver] Don't send empty ID cleanup query in notifs code.
Thu, Sep 14, 6:22 AM
marcin added a comment to D9144: Enable encrypted notification coalescing in NSE on iOS.

The UX change doesn't seem particularly bad to me, but I'd like to understand if the approach you're using here (deleting the old notif) is similar to the approaches that other encrypted apps take. Is there no way to set a "collapse ID" from the NSE?

Thu, Sep 14, 5:35 AM
marcin updated the summary of D9199: Encrypt all notifications on the keyserver for Android devices.
Thu, Sep 14, 2:49 AM
marcin requested review of D9199: Encrypt all notifications on the keyserver for Android devices.
Thu, Sep 14, 2:49 AM
marcin updated the test plan for D9198: Enable encrypted notifications coalescing on in Androoid native code..
Thu, Sep 14, 2:46 AM
marcin requested review of D9198: Enable encrypted notifications coalescing on in Androoid native code..
Thu, Sep 14, 2:46 AM
marcin retitled D9149: Encrypt all notifications on the keyserver for iOS devices from Encrypt all notifications on the keyserver to Encrypt all notifications on the keyserver for iOS devices.
Thu, Sep 14, 2:31 AM

Wed, Sep 13

marcin requested review of D9179: Save encrypted payload hash in MariaDB keyserver for iOS notifs..
Wed, Sep 13, 7:09 AM
marcin requested review of D9178: Enhance error check in CryptoModule decrypt. Add encrypted message hash to error message..
Wed, Sep 13, 7:09 AM

Tue, Sep 12

marcin added a comment to D9144: Enable encrypted notification coalescing in NSE on iOS.

If the UX change originating from this diff is not acceptable the alternative approach we can experiment with is to silence every collapsible notifications and schedule local notifications from the NSE. This way we will be able to set request.identifier property (since we are the ones creating the UNNotificationRequest instance) and let the OS do the coalescing.

Tue, Sep 12, 7:42 AM
marcin updated the diff for D9144: Enable encrypted notification coalescing in NSE on iOS.

Update coalescing condition.

Tue, Sep 12, 7:04 AM
marcin requested review of D9149: Encrypt all notifications on the keyserver for iOS devices.
Tue, Sep 12, 6:35 AM
marcin attached a referenced file: F754694: Current_coalescing.mov.
Tue, Sep 12, 5:41 AM
marcin attached a referenced file: F754700: NSE_coalescing.mov.
Tue, Sep 12, 5:41 AM
marcin added a comment to D9144: Enable encrypted notification coalescing in NSE on iOS.

As mentioned in the description coalescing mechanism changes slightly and so does user experience. I recommend that reviewers watch recordings attached below:
Current coalescing:

NSE coalescing:

Tue, Sep 12, 5:41 AM
marcin updated the summary of D9144: Enable encrypted notification coalescing in NSE on iOS.
Tue, Sep 12, 5:18 AM
marcin requested review of D9144: Enable encrypted notification coalescing in NSE on iOS.
Tue, Sep 12, 5:15 AM
marcin accepted D9129: [lib] utility function to get array of OneTimeKeys from an OLMOneTimeKeys object.
Tue, Sep 12, 2:17 AM
marcin accepted D8956: [Keyserver] Validate incoming tunnelbroker messages.
Tue, Sep 12, 2:17 AM
marcin added inline comments to D8956: [Keyserver] Validate incoming tunnelbroker messages.
Tue, Sep 12, 2:16 AM
marcin accepted D9113: [native] get notifications one time keys.
Tue, Sep 12, 2:14 AM

Mon, Sep 11

marcin updated the diff for D9069: Schedule blob deletion on Android when large notifications arrives.

Remove unnecessary Kotlin WorkManager dependency

Mon, Sep 11, 7:12 AM
marcin attached a referenced file: F752842: Screenshot 2023-09-11 at 15.53.25.png.
Mon, Sep 11, 7:09 AM
marcin attached a referenced file: F752844: Screenshot 2023-09-11 at 16.06.04.png.
Mon, Sep 11, 7:09 AM
marcin added a comment to D9069: Schedule blob deletion on Android when large notifications arrives.

I checked our app .apk file size twice: first time on a parent commit to this one and second time on this commit. In between I uninstalled the app and executed ./gradlew clean. My results are below:
Parent commit:

Screenshot 2023-09-11 at 15.53.25.png (244×869 px, 54 KB)

This commit:
Screenshot 2023-09-11 at 16.06.04.png (239×874 px, 70 KB)

So we probably have already had guava in some other dependency since the size in MB didn't change at all. However please correct my if my method of checking the size was wrong.

Mon, Sep 11, 7:09 AM
marcin added a comment to D9091: Schedule blob deletion on iOS.

One last question: Is it guaranteed that we call the notif's completionHandler only after the deletion tasks are finished?

It is not guaranteed and it is impossible to guarentee that since contentHandler is called quickly in the NSE while blob deletion takes place on the background thread in the main app so the system decides when it is best to call it. Additionally main app might not be running when NSE processes notification. However why would such guarantee be necessary in the first place? Blob download and persistence is synchronous and writing blob metadata to the storage which main app reads during deletion takes place afterwards. So the deletion process won't start until notification payload is persisted on the device so when user opens the app the message will be loaded and immediately visible in the app regardless on when delete requests are issued.

Mon, Sep 11, 6:00 AM
marcin added inline comments to D9069: Schedule blob deletion on Android when large notifications arrives.
Mon, Sep 11, 5:43 AM
marcin accepted D9115: [lib] move a shared type to lib.
Mon, Sep 11, 5:18 AM
marcin added inline comments to D9114: [native] update action types and reducer for setting comm access token.
Mon, Sep 11, 5:14 AM
marcin accepted D9114: [native] update action types and reducer for setting comm access token.
Mon, Sep 11, 5:12 AM
marcin accepted D9084: [native] get signed prekeys.
Mon, Sep 11, 5:12 AM
marcin added a comment to D8476: [Keyserver] Publish prekeys to identity service for cron job.

I understand that I am not a reviewer but @kamil wanted me to see this diff and I would like to note one thing. This cron job will only fire if keyserver's very first prekey's were marked as published somewhere else. In practice keyserver prekeys are published for the first time here: https://github.com/CommE2E/comm/blob/master/keyserver/src/responders/keys-responders.js#L59. However the only reason prekeys are marked as published there is that at the time this code was implemented we wanted to launch e2e notifs ASAP but the identity service was not ready, so we agreed on a temporary solution to mark prekey's as published when user fetches them to initialise olm notifications session with the keyserver. That said as of this differential this line should be deleted and we should find a place to publish keyserver's prekeys for the very first time after olm accounts for the keyserver are created.

Mon, Sep 11, 5:01 AM

Fri, Sep 8

marcin added a reverting change for rCOMMba899f0f379e: Temporary changes for staff release: rCOMMcf96d9a28808: Revert "Temporary changes for staff release".
Fri, Sep 8, 8:05 AM
marcin committed rCOMMcf96d9a28808: Revert "Temporary changes for staff release" (authored by marcin).
Revert "Temporary changes for staff release"
Fri, Sep 8, 8:05 AM
marcin committed rCOMM616fd7c8febd: [native] codeVersion -> 258 (authored by marcin).
[native] codeVersion -> 258
Fri, Sep 8, 8:05 AM
marcin committed rCOMMba899f0f379e: Temporary changes for staff release (authored by marcin).
Temporary changes for staff release
Fri, Sep 8, 8:05 AM
marcin committed rCOMM892784fa78c5: [native] codeVersion -> 257 (authored by marcin).
[native] codeVersion -> 257
Fri, Sep 8, 8:05 AM
marcin updated the diff for D9091: Schedule blob deletion on iOS.
  1. Delete blobs even if app is running in the background.
  2. Fix type in ongoing.
Fri, Sep 8, 6:43 AM
marcin added a comment to D9091: Schedule blob deletion on iOS.

Shouldn't we also send a delete request when a notif is received while the app is running?

This is possible using the darwin notification mechanism. We already use it so that NSE can inform the backgrounded running app that it can fetch messages from the temporary storage. One thing that I need to check however is that whether darwin notifications are delivered instantly even if the app is in the background or they are queued until the app enters the foreground. In our case we would prefer to second since I don't want to execute networking code when the app is backgrounded (the reason for this was explained in one of my first comments on this diff: https://phab.comm.dev/D9091#inline-57952).

Fri, Sep 8, 6:02 AM
marcin added inline comments to D9091: Schedule blob deletion on iOS.
Fri, Sep 8, 5:58 AM
marcin accepted D9096: [web/native] Retrieve and encode the ed25519 key in the QR code.
Fri, Sep 8, 2:18 AM

Thu, Sep 7

marcin closed D8795: Implement synchronization mechanisms to address process, thread and class level concurrency..
Thu, Sep 7, 9:03 AM
marcin committed rCOMMaea53f271c7d: Implement synchronization mechanisms to address process, thread and class level… (authored by marcin).
Implement synchronization mechanisms to address process, thread and class level…
Thu, Sep 7, 9:03 AM
marcin closed D8906: Correct error handling across Objective-C and C++.
Thu, Sep 7, 9:03 AM
marcin closed D8907: Introduce memory monitoring in NSE.
Thu, Sep 7, 9:03 AM
marcin committed rCOMMaa2f2bd74443: Correct error handling across Objective-C and C++ (authored by marcin).
Correct error handling across Objective-C and C++
Thu, Sep 7, 9:03 AM
marcin committed rCOMMb0277def662b: Introduce memory monitoring in NSE (authored by marcin).
Introduce memory monitoring in NSE
Thu, Sep 7, 9:03 AM
marcin closed D8905: Introduce thread-safety to NotificationsCryptoModule during cuncurrent access.
Thu, Sep 7, 9:03 AM
marcin committed rCOMM70cf45852540: Introduce thread-safety to NotificationsCryptoModule during cuncurrent access (authored by marcin).
Introduce thread-safety to NotificationsCryptoModule during cuncurrent access
Thu, Sep 7, 9:03 AM
marcin updated the diff for D8795: Implement synchronization mechanisms to address process, thread and class level concurrency..

Rebase before landing

Thu, Sep 7, 8:41 AM
marcin updated the diff for D8907: Introduce memory monitoring in NSE.

Rebase before landing

Thu, Sep 7, 8:40 AM
marcin updated the diff for D8906: Correct error handling across Objective-C and C++.

Rebase before landing

Thu, Sep 7, 8:39 AM
marcin updated the diff for D8905: Introduce thread-safety to NotificationsCryptoModule during cuncurrent access.

Rebase before landing

Thu, Sep 7, 8:39 AM
marcin added inline comments to D8907: Introduce memory monitoring in NSE.
Thu, Sep 7, 8:36 AM

Wed, Sep 6

marcin updated subscribers of D9091: Schedule blob deletion on iOS.
Wed, Sep 6, 7:35 AM
marcin requested review of D9091: Schedule blob deletion on iOS.
Wed, Sep 6, 4:31 AM
marcin updated the diff for D9069: Schedule blob deletion on Android when large notifications arrives.

Rebase

Wed, Sep 6, 4:13 AM
marcin updated the diff for D9068: Send blob holder in large notification payload.

Rebase

Wed, Sep 6, 4:12 AM
marcin updated the diff for D8698: Upload iOS notification payload to blob service if its size exceeds limits.

Rebase

Wed, Sep 6, 4:12 AM
marcin updated the diff for D8665: Implement native iOS code to fetch data from blob service.

Share blob client code with the main app.

Wed, Sep 6, 4:11 AM
marcin accepted D9085: [native] getNotificationsOneTimeKeys.
Wed, Sep 6, 1:47 AM
marcin added inline comments to D9084: [native] get signed prekeys.
Wed, Sep 6, 1:45 AM
marcin accepted D9083: [native] generateAndGetNotificationsPrekey.
Wed, Sep 6, 1:39 AM
marcin added inline comments to D9076: [native] expose prekey generation to js.
Wed, Sep 6, 1:39 AM

Tue, Sep 5

marcin accepted D9074: [client-backup] extract logic for getting primary identity public key.
Tue, Sep 5, 2:22 AM
marcin accepted D9076: [native] expose prekey generation to js.
Tue, Sep 5, 1:50 AM
marcin requested changes to D9074: [client-backup] extract logic for getting primary identity public key.
  1. Primary keyword is not valid when talking about the "main" olm account - the convention is to use Content keyword.
  2. Identity keyword refers to curve25519. ed25519 is signing key.
Tue, Sep 5, 1:48 AM

Fri, Sep 1

marcin requested review of D9069: Schedule blob deletion on Android when large notifications arrives.
Fri, Sep 1, 10:04 AM
marcin requested review of D9068: Send blob holder in large notification payload.
Fri, Sep 1, 9:43 AM
marcin updated the diff for D8698: Upload iOS notification payload to blob service if its size exceeds limits.

Implement WIP of comm access token authentication on iOS

Fri, Sep 1, 3:58 AM