Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reset RC per-instance userdefaults if config database was not found. #4896

Merged
merged 4 commits into from Feb 14, 2020

Conversation

dmandar
Copy link
Contributor

@dmandar dmandar commented Feb 12, 2020

Addresses b/148386739, #4677 , #4734. Backgroubd: We explicitly disable backup of the sqlite database. Hence upon restore, we do not have any data. This causes a fetch to be made, but we send the last eTag (which is saved in userdefaults and cannot be opted out of backup) along with the fetch request which causes the RC backend to send a NO_CHANGE back. Fix is to identify if the database is gone, and if so, reset the userdefaults.

Copy link
Contributor

@charlotteliang charlotteliang left a comment

Choose a reason for hiding this comment

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

Just to confirm, the issue happens when users uninstall the app and reinstall again but we thought config is fetched because user default is not reset?

@charlotteliang
Copy link
Contributor

LGTM on presubmit green

@@ -31,6 +31,7 @@
#define RCNTableNameInternalMetadata "internal_metadata"
#define RCNTableNameExperiment "experiment"

static BOOL gIsNewDatabase;
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be made thread-safe?

Copy link
Contributor Author

@dmandar dmandar Feb 13, 2020

Choose a reason for hiding this comment

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

it is. The variable is used on the database serial queue.

@@ -102,6 +102,14 @@ - (instancetype)initWithDatabaseManager:(RCNConfigDBManager *)manager
_userDefaultsManager = [[RCNUserDefaultsManager alloc] initWithAppName:appName
bundleID:_bundleIdentifier
namespace:_FIRNamespace];

// Check if the config database is new. If so, clear the configs saved in userDefaults.
if ([_DBManager isNewDatabase]) {
Copy link
Member

Choose a reason for hiding this comment

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

How can this test ever fail? I don't see gIsNewDatabase ever set to NO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test case setup method above creates a new test database. We just want to verify that this flag is set to true whenever an existing database is not found.

@charlotteliang charlotteliang requested review from ryanwilson and removed request for ryanwilson February 13, 2020 20:08
@charlotteliang
Copy link
Contributor

@ryanwilson nvm, I was adding you and wondering if NSUserDefault can opt out for backup, looks like it's not available.

@dmandar
Copy link
Contributor Author

dmandar commented Feb 13, 2020

Just to confirm, the issue happens when users uninstall the app and reinstall again but we thought config is fetched because user default is not reset?

It happens on a restore of a previous backup of the device. The config database is gone, but the last eTag is still remembered (since it is in the user defaults). Hence, a new fetch call succeeds but only returns NO_CHANGE since the eTag was sent up.

Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

Changelog should be updated with references to #4677 and #4734

@dmandar dmandar merged commit 90235b5 into master Feb 14, 2020
@dmandar dmandar deleted the restorebug branch February 14, 2020 23:08
@firebase firebase locked and limited conversation to collaborators Mar 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants