Page MenuHomePhabricator

Use GlobalDBSingleton when touching database from the native iOS code.
ClosedPublic

Authored by marcin on Sep 27 2022, 4:53 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 15, 12:09 AM
Unknown Object (File)
Sat, Nov 9, 8:12 PM
Unknown Object (File)
Sat, Nov 9, 8:12 PM
Unknown Object (File)
Sat, Nov 9, 8:12 PM
Unknown Object (File)
Sat, Nov 9, 8:12 PM
Unknown Object (File)
Sat, Nov 9, 8:12 PM
Unknown Object (File)
Sat, Nov 9, 8:12 PM
Unknown Object (File)
Sat, Nov 9, 8:12 PM
Subscribers

Details

Summary

This differential replaces any direct database access in iOS native code with GlobalDBSingleton API

Test Plan

Creating test release with those changes would remove iOS 137 crash

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin retitled this revision from Use GlobalDBSingletonIOSProxy when touching database from the antive code. to Use GlobalDBSingletonIOSProxy when touching database from the native iOS code..
This revision is now accepted and ready to land.Sep 27 2022, 9:18 AM

GlobalDBSingletonISOProxy API changed so updating AppDelegate.

We should be really sure about this code. If there are any issues, it will probably cause crashes we don't know about. I encourage everybody on this diff to be extremely thorough. We should be 100% sure of every line here. We should not be guessing, or trying things that seem to work without understanding 100% of why/how they work.

This is ok assuming we're using the proxy. I'm not yet convinced D5233 that this is the right thing to do, but this diff seems to be ok.

marcin retitled this revision from Use GlobalDBSingletonIOSProxy when touching database from the native iOS code. to Use GlobalDBSingleton when touching database from the native iOS code..
marcin edited the summary of this revision. (Show Details)
native/ios/Comm/AppDelegate.mm
171 ↗(On Diff #17343)

How sure are we that this doesn't need to be within GlobalDBSingleton? Looking into it, it seems like a couple assignments to instance variables, which should be safe... but we need to make sure there are no assignments from other threads (only one thread should ever call attemptDatabaseInitialization and SQLiteQueryExecutor::initialize), and we should be sure that there are no risks from reading from the variable while the assignment is in progress (if there are, we could consider an atomic variable or something)

native/ios/Comm/AppDelegate.mm
171 ↗(On Diff #17343)

attemptDatabaseInitialization and SQLiteQueryExecutor::initialize can be called from any thread since those method do not touch database and use std::call_once semantics which makes them idempotent (after it is called for the first time subsequent calls are no-ops) and thread safe.

native/ios/Comm/AppDelegate.mm
171 ↗(On Diff #17343)

Makes sense, thanks!