Page MenuHomePhabricator

Persist rescinds payloads in NSE so that AppDelegate can update thread statuses on app start.
ClosedPublic

Authored by marcin on Mar 1 2023, 4:23 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 2, 9:44 PM
Unknown Object (File)
Fri, Feb 23, 8:50 AM
Unknown Object (File)
Fri, Feb 23, 8:18 AM
Unknown Object (File)
Fri, Feb 23, 7:39 AM
Unknown Object (File)
Fri, Feb 23, 7:12 AM
Unknown Object (File)
Fri, Feb 23, 2:17 AM
Unknown Object (File)
Wed, Feb 21, 8:46 PM
Unknown Object (File)
Tue, Feb 20, 9:45 PM
Subscribers

Details

Summary

In this differential NSE persists rescind in a flat file storage we implemented for messages some time ago. AppDelegate reads the storage and updates thread statuses in SQLite if relevant.
As of this differential NSE is capable of providing entire functionality associated with rescind that we have implemented so far and codeVersion > 1000 can be removed and replaced with check for a specific code version.

Test Plan

Remove codeVersion > 1000 conditions. Build the app and kill it. Generate a some rescinds. Attach the debugger to moveMessagesToDatabase method and launch the app. Observe in the debugger payloads of rescinds read from temporary storage. Ensure they match threads that you
read on another device to generate rescinds.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin requested review of this revision.Mar 1 2023, 4:35 AM
atul added inline comments.
native/ios/Comm/AppDelegate.mm
322–334 ↗(On Diff #23333)

(I think this could probably be simpler (and maybe less error-prone) if we were to use the Foundation Codable API instead of NSJSONSerialization... but it looks like NSJSONSerialization is simpler if we're using Objective-C(++).

Nothing actionable, just a thought.)

336 ↗(On Diff #23333)

Should we react in a more "severe" way than logging + continueing if there's a failure here?

338 ↗(On Diff #23333)

Did you test initializing a std::string like this? I'm not super familiar with Objective C++

348 ↗(On Diff #23333)

Hm, why does this lambda need to be mutable?

native/ios/NotificationService/NotificationService.mm
36 ↗(On Diff #23333)

Is this TODO still relevant? Might be helpful to link some sort of Linear ticket associated with it?

(Realize this is outside the scope of this diff)

native/ios/Comm/AppDelegate.mm
322–334 ↗(On Diff #23333)

After a quick research I found that using Codable API in OBJ-C requires some bridges with Swift: https://stackoverflow.com/questions/50617542/how-to-use-codable-protocol-in-objective-c-data-model-class. Additionally I am not sure how stable is the current structure of the rescind payload - how likely are we to be changing it in future. Using Codable would require us to declare class with fixed attributes, so each change in rescind payload on the JS side would require a change in this class. This increases the coupling in the codebase. However, once we reach a point where notifications payload structures are unlikely to change we might consider representing them in native languages as more rigid and restrictive types rather than just dictionaries mapping String -> Object.

336 ↗(On Diff #23333)

In this particular case, if we ignore the error we will eventually pull the correct state of thread unread status from keyserver. Currently updating SQLite in response to notifications is a kind of redundancy that can be helpful when user opens the app in weak network environment but they still got the correct state because of received notifications. From the user perspective, killing the app here would be a poor experience (in worst case they would still get the correct state but just later). But from the dev perspective it might be useful since having an error here means that some error occurred on the NSE side since it failed to correctly persist rescind payload (it could be time out for example). This would make us investigate what had happened on the NSE side.
It is high time this issue is taken on: https://linear.app/comm/issue/ENG-2670/c-equivalent-of-dev-flag

338 ↗(On Diff #23333)

Every where in our codebase where we create std::string from NSString* we call UTF8String method before passing it to std::string constructor. Apple docs suggest to do it this way: https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Strings/Articles/CreatingStrings.html

348 ↗(On Diff #23333)

This answer: https://stackoverflow.com/a/5503357 nicely explains how capturing and mutability works in C++ lambdas. This lambda has to be mutable since the inner function: comm::ThreadOperations::updateSQLiteUnreadStatus(threadID, false) takes threadID be reference.

native/ios/NotificationService/NotificationService.mm
36 ↗(On Diff #23333)

This TODO was introduced here: https://phab.comm.dev/D3162. I am pretty sure that this TODO was automatically added when NotificationService.mm file was generated, but I will check with @tomek about this. Nevertheless I don't see any reason to remove this TODO until we actually put there some code that handles notification decryption and modifies self.bestAttemptContent.

tomek added inline comments.
native/ios/Comm/AppDelegate.mm
342 ↗(On Diff #23333)

Should we also extract a const for threadID?

native/ios/NotificationService/NotificationService.mm
36 ↗(On Diff #23333)

Yeah, this was added to indicate where we should put the code that modifies the message content. We can replace this with the code / function that actually modifies the message - when it will be introduced.

89 ↗(On Diff #23333)

Is it possible that a message has messageInfos and is a rescind at the same time?

This revision is now accepted and ready to land.Mar 13 2023, 7:02 AM
native/ios/Comm/AppDelegate.mm
342 ↗(On Diff #23333)

It would be a good measure.

native/ios/NotificationService/NotificationService.mm
89 ↗(On Diff #23333)

According to the keyserver code messageInfos field is never introduced to rescind payload and I can't imagine why would we do so. However technically no type system or unit tests prevent us from inserting this field in future. cc @atul, @ashoat - is it ever possible that rescind might have messageInfos field?

native/ios/NotificationService/NotificationService.mm
89 ↗(On Diff #23333)

No, I don't think this is possible

native/ios/NotificationService/NotificationService.mm
89 ↗(On Diff #23333)

Therefore I assume it is safe early return here once messageInfos content is persisted.

Why hasn't this been landed? As mentioned on Monday, we need to create a release with these changes ASAP so we can test the before the end of the month...

Rebase before landing. Extract thread ID key to constant

Why hasn't this been landed? As mentioned on Monday, we need to create a release with these changes ASAP so we can test the before the end of the month...

There were still a couple of comments I addressed on Tuesday and wanted to wait until Wednesday in case my answers would trigger change request from @tomek . This diff stack requires being particularly careful. As stated in my daily update from March 14 it was my plan to land it today and reach to @atul to make a release.