Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions FirebaseRemoteConfig/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
# Unreleased
- [fixed] Fixed a bug where Remote Config does not work after a restore
of a previous backup of the device. (#14459)
- [fixed] Fixed a data race condition on the global database status flag
by synchronizing all read and write operations. (#14715)

# 12.3.0
- [fixed] Add missing GoogleUtilities dependency to fix SwiftPM builds when
building dynamically linked libraries. (#15276)
Expand Down
4 changes: 4 additions & 0 deletions FirebaseRemoteConfig/Sources/FIRRemoteConfig.m
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tusharkhandelwal8 there are several dispatch_syncs going on here. If we're on the main queue when dispatching synchronously, we could hold up the main queue and risk deadlocks. Is there a way to make the synchronization of the property asynchronous?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is fine as-is. The work done on the synchronizing queue is only ever:

  • read a global variable
  • write a global variable
  • nothing (empty block to synchronize)

In the future a more ideal architecture would either be to initialize asynchronously and then overwrite the database with the backup-restored data, or just initialize synchronously on the main thread instead of doing async initialization and then using a locking mechanism to wait for it. @tusharkhandelwal8 if you have the time you could try profiling app launch with this patch to see how the dispatch_sync affects launch times.

Regardless, I'm fine with approving this as a temporary fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion on profiling!
I profiled the app launch with the dispatch_sync patch to assess the performance overhead. The synchronization block almost increased 0.3% of the total measured App Launch Time in the test environment. Given the extremely low impact, this synchronous block seems like a good solution for now.

I agree that a fully synchronous or fully asynchronous initialization would be the long-term architectural goal.

Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,10 @@ - (instancetype)initWithAppName:(NSString *)appName

// Initialize RCConfigContent if not already.
_configContent = configContent;

// We must ensure the DBManager's asynchronous setup (which sets gIsNewDatabase)
// completes before RCNConfigSettings tries to read that state for the resetUserDefaults logic.
[_DBManager waitForDatabaseOperationQueue];
_settings = [[RCNConfigSettings alloc] initWithDatabaseManager:_DBManager
namespace:_FIRNamespace
firebaseAppName:appName
Expand Down
4 changes: 4 additions & 0 deletions FirebaseRemoteConfig/Sources/RCNConfigDBManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -130,4 +130,8 @@ typedef void (^RCNDBLoadCompletion)(BOOL success,

/// Returns true if this a new install of the Config database.
- (BOOL)isNewDatabase;

/// Blocks the calling thread until all pending database operations on the internal serial queue are
/// completed. Used to enforce initialization order.
- (void)waitForDatabaseOperationQueue;
@end
23 changes: 21 additions & 2 deletions FirebaseRemoteConfig/Sources/RCNConfigDBManager.m
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@
/// The storage sub-directory that the Remote Config database resides in.
static NSString *const RCNRemoteConfigStorageSubDirectory = @"Google/RemoteConfig";

/// Introduce a dedicated serial queue for gIsNewDatabase access.
static dispatch_queue_t gIsNewDatabaseQueue;

/// Remote Config database path for deprecated V0 version.
static NSString *RemoteConfigPathForOldDatabaseV0(void) {
NSArray *dirPaths =
Expand Down Expand Up @@ -82,7 +85,9 @@ static BOOL RemoteConfigCreateFilePathIfNotExist(NSString *filePath) {
}
NSFileManager *fileManager = [NSFileManager defaultManager];
if (![fileManager fileExistsAtPath:filePath]) {
gIsNewDatabase = YES;
dispatch_sync(gIsNewDatabaseQueue, ^{
gIsNewDatabase = YES;
});
NSError *error;
[fileManager createDirectoryAtPath:[filePath stringByDeletingLastPathComponent]
withIntermediateDirectories:YES
Expand Down Expand Up @@ -119,6 +124,8 @@ + (instancetype)sharedInstance {
static dispatch_once_t onceToken;
static RCNConfigDBManager *sharedInstance;
dispatch_once(&onceToken, ^{
gIsNewDatabaseQueue = dispatch_queue_create("com.google.FirebaseRemoteConfig.gIsNewDatabase",
DISPATCH_QUEUE_SERIAL);
sharedInstance = [[RCNConfigDBManager alloc] init];
});
return sharedInstance;
Expand Down Expand Up @@ -1219,7 +1226,19 @@ - (BOOL)logErrorWithSQL:(const char *)SQL
}

- (BOOL)isNewDatabase {
return gIsNewDatabase;
__block BOOL isNew;
dispatch_sync(gIsNewDatabaseQueue, ^{
isNew = gIsNewDatabase;
});
return isNew;
}

- (void)waitForDatabaseOperationQueue {
// This dispatch_sync call ensures that all blocks queued before it on _databaseOperationQueue
// (including the createOrOpenDatabase setup block) execute and complete before this method
// returns.
dispatch_sync(_databaseOperationQueue, ^{
});
}

@end
Loading