Page MenuHomePhabricator

Migrate notifications crypto account to MMKV on native
ClosedPublic

Authored by marcin on Jul 5 2024, 11:21 AM.
Tags
None
Referenced Files
F2905137: D12676.id42941.diff
Sun, Oct 6, 3:36 AM
Unknown Object (File)
Wed, Sep 25, 5:45 PM
Unknown Object (File)
Wed, Sep 25, 5:44 PM
Unknown Object (File)
Sun, Sep 22, 11:38 PM
Unknown Object (File)
Sun, Sep 22, 7:25 AM
Unknown Object (File)
Sat, Sep 21, 12:55 AM
Unknown Object (File)
Thu, Sep 19, 12:28 AM
Unknown Object (File)
Wed, Sep 18, 2:38 PM
Subscribers

Details

Summary

This differential moves notifications crypto account to MMKV on native

Test Plan

Repeat test plan for the parent diff. If another device is able to create outbound session it means that key upload callback are working

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

We should be really careful in reviewing this diff. It seems complex, and notif storage in C++ has historically been an area where we've often introduced very serious issues

  1. Don't pass references to unique pointer between lambdas - a rookie mistake!. Pass shared pointers by copy.
  2. Switch to multi process mode on MMKV on Android
native/android/app/src/main/java/app/comm/android/fbjni/CommMMKV.java
23 ↗(On Diff #42235)

It took me 8 hours of debugging to get there. This diff worked fine on iOS but on Android the app was crashing. I established that notifs crypto account was either missing in mmkv after setting it or it was set partially (subsequence of bytes)- it resulted in strange UTF8 encoding JNI crashes.
I excluded all possible race conditions and data format issues. After encountering this comment: https://github.com/Tencent/MMKV/wiki/FAQ

Why does my MMKV instance lost data after App restart?
There are some possibilities:

You forget to set the multi-process mode to open a multi-process MMKV instance. Try setting multi-process mode to everywhere that access this MMKV instance. Check if it happens or not.

I realized what was going on. MMKV keeps some data in memory and some on the disk. According to some heuristics it flushes data in memory to disk from time to time. In MULTI_PROCESS_MODE however it flushes everything immediately resulting in consistent data after each write. This explained iOS specificity - on iOS we used MULTI_PROCESS_MODE from the start since we shared it between NSE and the app.

kamil added inline comments.
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
580–584 ↗(On Diff #42235)

with that rollback is no longer working for notifs module - should we implement this too? How this could be handled in MMKV?

656–657 ↗(On Diff #42235)

Not sure if this is safe - wondering if it's possible that if we don't remove notif account from SQLite it will cause some issues and override the already updated state in MMKV, but I don't see anything yet

native/cpp/CommonCpp/NativeModules/CommCoreModule.h
53–55 ↗(On Diff #42235)

overall I think that right now NotificationsCryptoModule should fully handle reading and persisting this - we have mixed responsibilities, and I see no reason for doing this here anymore. I feel like updating each NotificationsCryptoModule method to read content and persist makes the most sense and makes the code here cleaner.

The only reason for that is being able to rollback persisting, but let's discuss this separately in my comment below.

This is just an idea, wondering about your thoughts. Also given that this is already implemented and tested we can proceed and refactor this in the future.

I'm lacking too much context here

native/cpp/CommonCpp/NativeModules/CommCoreModule.h
53–55 ↗(On Diff #42235)

If you take a look at validateAndUploadPrekey you will see that we might want to execute a sequence operations governed by if conditions before persisting account state. With the design that you suggest we would have to persist after each call. This would result in unnecessary persists. We could refactor NotificationsCryptoModule to become statefull but this would look similar to what we already have in this differential.

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
580–584 ↗(On Diff #42235)

I intentionally put notifs persistence after content. If content persistence fails we won't even approach notifs persistence. On the other hand if we persist both accounts but there is a crash prior to commit call we won't be able to correctly rollback notifs account - MMKV doesn't provide transactional features.

kamil added inline comments.
native/cpp/CommonCpp/NativeModules/CommCoreModule.h
53–55 ↗(On Diff #42235)

okay, makes sense

This revision is now accepted and ready to land.Jul 31 2024, 8:26 AM