Database recreation listener logic is wrong

RESOLVED FIXED

Status

()

Firefox for iOS
Data Storage
P1
normal
RESOLVED FIXED
8 months ago
a month ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

unspecified
All
iOS

Firefox Tracking Flags

(fxios8.0+, fxios-v7.0 ?, fxios-v7.1 ?, fxios-v7.2 ?, fxios-v7.3 ?, fxios-v7.4 ?, fxios-v7.5 affected, fxios-v7.6 affected, fxios-v8.0 affected, fxios-v8.1 affected, fxios-v9.0 fixed)

Details

(Whiteboard: [MobileCore])

Attachments

(1 attachment)

(Assignee)

Description

8 months ago
I noticed that several times I've been left with partial bookmarks or partial history in my main Firefox profile.

The only way this can happen is if two things occur:

1. The database is moved/recreated.
2. The Sync timestamps are not reset.

This used to be impossible: in the course of initializing the profile we would create a BrowserSyncManager, and it would listen to reset notifications and do the right thing.

These days BrowserProfile.init is eager: it eagerly creates BrowserDB (line 243), which tries to create the schema table, which can trigger the database replacement logic.

BrowserSyncManager observes onDatabaseWasRecreated… but BrowserSyncManager takes a Profile as an init argument.

If the database is moved out of the way during the init of BrowserProfile, there will be no BrowserSyncManager listening for the database recreation logic, and we will be left with partial Sync data.

The fix is to somehow ensure that the correct Sync-related code runs if the DB needs to be recovered: perhaps via a flag in Profile, or by rearranging the init of BrowserSyncManager.

It's also necessary to detect this state and recover — perhaps by checking for a .bak file _once_ and re-syncing the appropriate database.
(Assignee)

Comment 1

8 months ago
Regression from https://github.com/mozilla-mobile/firefox-ios/pull/2419:

https://github.com/mozilla-mobile/firefox-ios/commit/dae882fbadd4e467f7a6e4f04007528931fffddc#diff-7a45ce0d70d55bdf297fcacd47e4e874R259
status-fxios-v7.0: --- → ?
status-fxios-v7.1: --- → ?
status-fxios-v7.2: --- → ?
status-fxios-v7.3: --- → ?
status-fxios-v7.4: --- → ?
status-fxios-v7.5: --- → affected
status-fxios-v7.6: --- → affected
status-fxios-v8.0: --- → affected
status-fxios-v8.1: --- → affected
(Assignee)

Updated

8 months ago
See Also: → bug 1368719

Updated

8 months ago
tracking-fxios: ? → 8.0+
Whiteboard: [MobileCore][needsuplift]

Updated

8 months ago
Attachment #8883737 - Flags: review?(sarentz)
Unclear what the risk is of landing this in 8.0 so late. Maybe this should move to 8.1?
Flags: needinfo?(rnewman)
Priority: -- → P1

Updated

8 months ago
Assignee: sarentz → rnewman

Updated

8 months ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

8 months ago
(In reply to Stefan Arentz [:st3fan] from comment #3)
> Unclear what the risk is of landing this in 8.0 so late. Maybe this should
> move to 8.1?

The risk of the change itself to everyday users is relatively low.

The larger risk, which I have neither examined through inspection nor tested, is whether the reset logic still works -- it's possible that this bug means it has never run since the Swift 3 migration.

It's unlikely to be worse than the current situation, which essentially requires signing out of Sync and signing back in to get your data back.

The conservative approach is to ship 8.0 and let this ride the trains. My hope is that justindarc is taking a holistic view of this and related bugs.
Flags: needinfo?(rnewman)
Attachment #8883737 - Flags: review?(jdarcangelo) → review+
(Assignee)

Comment 5

8 months ago
https://github.com/mozilla-mobile/firefox-ios/commit/64d03672a6364d30ac4daa5d6d56bb3d41cb09bf
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
(Assignee)

Updated

8 months ago
status-fxios-v9.0: --- → fixed
Uplifted to v8.x
Whiteboard: [MobileCore][needsuplift] → [MobileCore]
Attachment #8883737 - Flags: review?(sarentz)
You need to log in before you can comment on or make changes to this bug.