736393 - Don't abort on store failure

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Android Sync, defect, P1)

Description

[Richard Newman [:rnewman]]

We don't on one half of the process… arguably the wrong half! That is, we abort if we fail to store locally, but don't abort if we fail to upload.

We didn't notice before because failure didn't really apply (see Bug 736352).

Nick, I want your opinion in particular.

Comment 1

[Richard Newman [:rnewman]]

Calling this a P1 to either fix or WONTFIX.

Comment 2

[JP Rosevear [:jpr]]

Nomming because of blocking 747746

Comment 3

[Allison Naaktgeboren :ally]

mobile triage: +

Comment 4

[Nick Alexander :nalexander [he/him]]

Merged in develop:

https://github.com/mozilla-services/android-sync/commit/bbce060aec99a662640d69c9988075047fbef82c

Comment 5

[Nick Alexander :nalexander [he/him]]

Attachment: patch against m-i

Comment 6

[Richard Newman [:rnewman]]

Extra points if you capitalize "Don't" in the commit message before you land.

Comment 7

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e22cc18646d

Comment 8

[Allison Naaktgeboren :ally]

Comment on attachment 628859 [details] [diff] [review]
patch against m-i

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5e22cc18646d

Comment 10

[Richard Newman [:rnewman]]

I'd like this to land on Aurora alongside Bug 760614 and/or the stage continuation bug. nalexander, please bear that in mind.

Comment 11

[Richard Newman [:rnewman]]

(In reply to Richard Newman [:rnewman] from comment #10)
> I'd like this to land on Aurora alongside Bug 760614 and/or the stage
> continuation bug. nalexander, please bear that in mind.

The latter is Bug 758382.

Reasoning, now that I'm not typing on my phone: an upload failure with this fix alone will now prevent subsequent engines from syncing; it's safer but less usable. (See Bug 760360.)

I'd rather make a net step forward in one go for a beta, rather than one step kinda-back then two steps forward.

Comment 12

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-beta/rev/89ec8943347a

Comment 13

[Nick Alexander :nalexander [he/him]]

To observe this error before fix:

1. pair device to Sync account
2. sync device successfully
3. turn on Fennec master password
4. add new password on desktop
5. sync device and observe in the logs that Sync aborts during passwords stage (and does not continue to bookmarks stage)

To see that the fix is in place:

perform steps 1-4 above.

5. sync device and observe password store failure in log, similar to:

E GeckoLibLoad(23485)         Throw
E GeckoPasswordsProvider(23485) Error in NSSBridge
E JavaBinder(23485)           *** Uncaught remote exception!  (Exceptions are not yet supported across processes.)
E JavaBinder(23485)           java.lang.RuntimeException: java.lang.Exception: PK11SDR_Encrypt returned error -8177: The security password entered is incorrect.
E JavaBinder(23485)           
...
D FxSync(23437)               org.mozilla.firefox :: PasswordsRepoSession :: Record insert caused a RemoteException.
V FxSync(23437)               org.mozilla.firefox :: RecordsChannel :: Failed to store record with guid {f100d359-1bcc-b241-b639-ed634296ba5f}
V FxSync(23437)               org.mozilla.firefox :: CRecordConsumer :: Record stored. Notifying.
W FxSync(23437)               org.mozilla.firefox :: SynchronizerSession :: First RecordsChannel onFlowStoreFailed. Logging local store error.
W FxSync(23437)               android.os.RemoteException
W FxSync(23437)               	at org.mozilla.gecko.sync.repositories.android.PasswordsRepositorySession.insert(PasswordsRepositorySession.java:458)
W FxSync(23437)               	at org.mozilla.gecko.sync.repositories.android.PasswordsRepositorySession$5.run(PasswordsRepositorySession.java:350)
W FxSync(23437)               	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1076)
W FxSync(23437)               	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:569)
W FxSync(23437)               	at java.lang.Thread.run(Thread.java:856)
I FxSync(23437)               org.mozilla.firefox :: RecordsChannel :: onStoreCompleted. Notifying delegate of onFlowCompleted. Fetch end is 1339782788180, store end is 1339782800077
V FxSync(23437)               org.mozilla.firefox :: ServLocSynchronizerSess :: No failures fetching remote records.
W FxSync(23437)               org.mozilla.firefox :: ServLocSynchronizerSess :: Got 1 failures storing local records! Ignoring local store failures and continuing synchronizer session.

6. verify that password stage continues (and might fail, depending on what passwords Fennec has stored)
7. verify that sync continues to bookmarks stage, with logs similar to:

I FxSync(23437)               org.mozilla.firefox :: ServerSyncStage :: Advancing session even though stage failed. Timestamps not persisted.
I FxSync(23437)               org.mozilla.firefox :: SyncAdapter :: Stage completed: syncPasswords
I FxSync(23437)               org.mozilla.firefox :: GlobalSession :: Running next stage syncBookmarks (org.mozilla.gecko.sync.stage.AndroidBrowserBookmarksServerSyncStage@4119dfd8)...