1424207 - Crash when modifying and syncing the History sync pref

Status: RESOLVED DUPLICATE of bug 1424727

(Firefox for iOS :: Data Storage, defect)

Description

[Simon Basca [:SimonB]]

Attachment: Client-2017-12-08-111303.ips.beta

Build: 10.4 (8211)
Device: iPad mini 4
iOS: 11.2 beta 6

Steps to reproduce: 
1. Open Firefox.
2. Sign in to FxA.
3. Go to Sync Options and make sure only History is enabled for sync.
4. Start a sync.

Actual results:
The app crashes.
If I sync anything else except History, Firefox will not crash. 

Note:
- Issue reproducible on all tested devices and iOS versions
- Issue reproducible only on Firefox 10.4(8211). This is not reproducible on Firefox Beta 10.4 (8182)(latest).

Comment 1

[Catalin Suciu [:csuciu]]

I can reproduce the crash with this steps:

1. Start both desktop and iOS with clean profiles
2. Create a new FxA on desktop
3. Sign in with the same FxA on iOS
4. Make sure both clients are in sync
5. On iOS, go to Settings => "Sync Options" and disable "History"
6. Sync on iOS = > Sync on desktop
        => Sometimes the crash happens after this step. If not:
7. On iOS, go to Settings => "Sync Options" and enable "History"
8. Sync on iOS = > Sync on desktop

Result: FF crashes

Comment 2

[Catalin Suciu [:csuciu]]

The build used for testing this was created from the master branch, so v10.x is not affected.

Comment 3

[Catalin Suciu [:csuciu]]

Attachment: Client-2017-12-11-144329.ips.beta

10.4(8245)

Comment 4

[Catalin Suciu [:csuciu]]

Please disregard the "v10.x is not affected" part from the previous comment.

I can reproduce this crash on Beta 10.4 (8245) by enabling the History section for sync on desktop.

STR:

1. Login with the same FxA on iOS and desktop.
2. Make sure both devices are in sync.
3. Go to the "Sync Settings" page on desktop and uncheck "History".
4. Sync on desktop => sync on mobile
5. 3. Go to the "Sync Settings" page on desktop and check "History".
6. Sync on desktop => sync on mobile

Result: Firefox crashes and will crash continuously every time it syncs this account.

Comment 5

[Richard Newman [:rnewman]]

Looking blindly (no symbolicated stack), I suspect Bug 1417034, particularly 995b5697e06b7d593a3c65d3b4d293dc7f651906 and 35783447e70ef102fb931f8d6eb281ce56df7449.

Comment 6

[James Hugman [:jhugman] [@jhugman]]

Fatal error at https://github.com/mozilla-mobile/firefox-ios/blob/master/Storage/SQL/SQLiteHistory.swift#L826 unwrapping an optional. 

#0	0x0000000117ed0410 in specialized _fatalErrorMessage(_:_:file:line:flags:) ()
#1	0x0000000110f46aeb in closure #1 in SQLiteHistory.insertOrUpdatePlace(_:modified:) at /Users/jhugman/workspaces/mozilla/firefox-ios/Storage/SQL/SQLiteHistory.swift:826
#2	0x0000000110f48e06 in partial apply for closure #1 in SQLiteHistory.insertOrUpdatePlace(_:modified:) ()
#3	0x0000000110f49bff in thunk for @callee_owned (@owned SQLiteHistory.HistoryMetadata?) -> (@owned Deferred<Maybe<String>>) ()
#4	0x0000000110f517a2 in partial apply for thunk for @callee_owned (@owned SQLiteHistory.HistoryMetadata?) -> (@owned Deferred<Maybe<String>>) ()
#5	0x0000000110d304b1 in closure #1 in chainDeferred<A, B>(_:f:) at /Users/jhugman/workspaces/mozilla/firefox-ios/Shared/DeferredUtils.swift:170
#6	0x0000000110d307c2 in partial apply for closure #1 in chainDeferred<A, B>(_:f:) ()
#7	0x0000000117bf4175 in specialized closure #1 in Deferred.bindQueue<A>(_:f:) at /Users/jhugman/workspaces/mozilla/firefox-ios/Carthage/Checkouts/Deferred/Deferred/Deferred.swift:94

Comment 7

[James Hugman [:jhugman] [@jhugman]]

Reproduced on the Simulator, on master.

That line is relatively old (Feb 2017), but it's not clear how toggling the History back on causes this code path.

Comment 8

[Richard Newman [:rnewman]]

I do not believe this is a new bug. At least, it doesn't look like any of the relevant code has changed.

When we apply an incoming history record, we call `ensurePlaceWithURL`, which inserts a row into history, but does not set a local modified time. Then we call `metadataForGUID`, which retrieves some of that row, including the `local_modified` timestamp. Which is `NULL`.

That _should_ be fine, except that somehow the history metadata has `shouldUpload` set to `true`. 

That is what happens when you reset.

So this is what happened:

1. We insert a history item from the server. It has no local modification time. 
2. You uncheck history; that marks the row as modified and clears the server mtime.
3. The server is wiped.
4. Then you check history.
5. Another client uploads the same record again.
6. We download the same record again, check it against the local row… which does _not_ have the same server modified time, and _also_ doesn’t have a local modified time. The timestamp unwrap fails: crash.

The fix is to make that line aware of the particular situation of a row being marked as changed but having no local modified time — which we can treat as older than the server record, because it’s almost certainly the same thing.

Comment 9

[Justin D'Arcangelo [:justindarc]]

This is the same root cause and is fixed by the patch in Bug 1424727.