[native] Implement utils to lock MMKV for entire sequence of operations and use to to sync encryption and decryption of notifs
ClosedPublic
Actions

Authored by marcin on Aug 2 2024, 8:16 AM.

Details

Reviewers

kamil
tomek
bartek

Commits

rCOMM4e3a7d067512: [native] Implement utils to lock MMKV for entire sequence of operations and use…

Summary

This differential implements utils to avoid data races between the main app and the notifs code. By default MMKV synchronizes only for a single operation (read , write or removal). Unfortunately this opens the gate for data races against notif encryption and decryption that happen on different thread/proces and involve reading and writing back the state of the olm session.

MMKV exposes public methods to lock and unlock the database for the sequence of operations. I wasn't aware of them since they are not mentioned in the documentation so I had to dig them up in the code.
They look like these: https://github.com/Tencent/MMKV/blob/v1.3.5/Core/MMKV.cpp#L1190-L1201, https://github.com/Tencent/MMKV/blob/v1.3.5/Core/MMKV.h#L396-L399

Test Plan

Added logs to introduced methods informing when the lock is acquired and when released. Sent e2ee notifs back and forth and examined the logs.

Diff Detail

Repository

rCOMM Comm

Branch

marcin/eng-8237

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

marcin created this revision.Aug 2 2024, 8:16 AM

Herald added a subscriber: ashoat. · View Herald TranscriptAug 2 2024, 8:16 AM

marcin edited the summary of this revision. (Show Details)Aug 2 2024, 8:24 AM

marcin edited the test plan for this revision. (Show Details)

marcin edited the summary of this revision. (Show Details)Aug 2 2024, 8:28 AM

Harbormaster completed remote builds in B30874: Diff 43041.Aug 2 2024, 8:32 AM

marcin requested review of this revision.Aug 2 2024, 8:32 AM

marcin added inline comments.Aug 2 2024, 8:42 AM

native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
617	This change makes all operations necessary to decrypt a notif run atomically and without the risk of corrupoting state after encryption. Unfortunately this means that we might have a network call (call to identity if brand new session creation is necessary) in this critical section. There is a way to enchance this by adopting the approach that we have for web. That is outside of the scope of this diff. and should be handled by separate task. (I will introduce it shortly)
native/cpp/CommonCpp/Tools/CommMMKV.h
35	The constructor of `ScopedCommMMKVLock` will acquire the lock while the destructor will release the lock. This is how we implement `finally` in C++. So the idea is to create instance of this class and perform a sequence of operations in the scope were the instance is valid.
native/ios/Comm/CommMMKV.mm
11	Interestingly `lock` and `unlock` methods are public in Android java wrapper but are not in Objective-C wrapper. However the [[ https://github.com/Tencent/MMKV/blob/v1.3.5/iOS/MMKV/MMKV/libMMKV.mm#L55-L60 \| Objective - C wrapper has an instance variable of type `mmkv::MMKV` ]] so we can use it to access lock and unlock methods. This is a bit hacky but it is well-known pattern that are using this pattern for `CommSecureStore` where we redefine `EXSecureStore` interface with its private methods. This allows us to use them even though they are not publicly available via header file.