This differential introduces and implements method in CommCoreModule that clears Secure Store and SQLite.
Details
Details
Once this differential is applied 'clearSensitiveData' should be callable anywhere in JS and calling it in a random place should crash the app, with an error indicating that SQLite access is corrupted.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
|---|---|---|
| 959–968 ↗ | (On Diff #15261) | Why do we need to use a promise when this whole code runs on a single thread? |
| native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
|---|---|---|
| 959–968 ↗ | (On Diff #15261) | This is definitely my mistake. When I was working on this code I thought te entire operation should run on this->databaseThread. But after deeper testing and considerations I came to a conclusion this operation must run on the main thread, so I removed lines that place this code inside a lambda scheduled on databaseThread, but it appears I didn't realize that promise and future programming is no longer needed. |
Comment Actions
Remove unnecessary promise-future programming since all operations are executed on the same thread.
| native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
|---|---|---|
| 962 ↗ | (On Diff #15310) | Is it ok to throw JSError from this function? In e.g. createThreadStoreOperations we throw runtime_error and I'm wondering which approach should be preferred. |
| native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
|---|---|---|
| 962 ↗ | (On Diff #15310) | I think JSError should be preferred anywhere in CommCoreModule. Methods in CommCoreModule are expected to be called from react components. By throwing JSError we enable ourselves to handle errors from react components where we have much more knowledge and context to handle them properly. Even if we decide not to handle the error and crash the app we still have more information compared to being crashed by C++ error. We observed a crash caused by throwing standard C++ error: https://linear.app/comm/issue/ENG-1444/sqlite-thread-crash-on-ios-build-137, we addressed by re-throwing JSError. Code that is throwing std::runtime_error was written 9 months ago - as was the code for processThreadStoreOperations and processMEssageStoreOperations. We already have a Linear task to add JSError in those functions: https://linear.app/comm/issue/ENG-1471/change-error-handling-in. Probably the scope of this task should be extended. |
Comment Actions
Once this differential is applied 'clearSensitiveData' should be callable anywhere in JS and calling it in a random place should crash the app, with an error indicating that SQLite access is corrupted.
Have you tested this to verify the expected behavior? I usually pick a component that's simply and easy to work with (specifically privacy-preferences.react.js) to make calls to CommCoreModule.
Comment Actions
Due to the most recent update in D4728 entire database cleanup process is scheduled on databaseThread. We still somehow block the main thread here (we wait on the future to complete) but in a way that we already use (look at getAllMessagesSync method of CommCoreModule https://github.com/CommE2E/comm/blob/master/native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp#L173). Therefore I have an impression that this might be acceptable. But if @ashoat disagrees, then just left me a comment here and I will implement this function asynchronously to return a promise that will be handled in JavaScript (D4730).