758382 - Handle stage errors in content stages by advancing, not aborting

Status: VERIFIED FIXED

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

Description

[Richard Newman [:rnewman]]

Follow on from Bug 757646. If passwords dies for a local reason, let's just roll right on and try the next stage.

We need a way for a failure during session setup or insertion to result in happiness.

Bug 736393 bears on this.

Comment 1

[Nick Alexander :nalexander [he/him]]

Per discussion with rnewman:

Android sync has treated any error -- no matter how (in)consequential -- as catastrophic and aborted the sync entirely.  This approach is maximally cautious: it is most likely to protect against data loss, and most likely to produce useful bug reports.  I think it is the right choice when we had little faith in the code.

Coming through beta, however, I think we have some faith in the code.  I think we can roll through some errors in some stages without much concern of breaking other parts of the system, and in fact we can make the entire system more resilient.

Brass tacks: ServerSyncStage should log, not save bundles (so the failing data is retried later), and should continue through onSynchronizeFailed.

Comment 2

[Dave Miller [:justdave]]

(In reply to Nick Alexander :nalexander from comment #1)
> Android sync has treated any error -- no matter how (in)consequential -- as
> catastrophic and aborted the sync entirely.

How about separating it into pieces?  In the desktop client, you have a series of checkboxes to choose which items to sync.  Those items could all be listed separately in the Accounts & Sync panel within the Firefox Sync account.  As a bonus you know which part is syncing because it'll show the sync indicator on each one in turn as they sync if you're looking at the account, and it also gives you a convenient place to show the user that an error occurred, as you can show the sync error icon on the passwords option if it fails, etc.

Comment 3

[Richard Newman [:rnewman]]

(In reply to Dave Miller [:justdave] from comment #2)

> As a bonus you know which part is syncing because it'll show the
> sync indicator on each one in turn as they sync if you're looking at the
> account, and it also gives you a convenient place to show the user that an
> error occurred, as you can show the sync error icon on the passwords option
> if it fails, etc.

That's not really possible: the Android-managed checkboxes there correspond to particular manifest entries, not to arbitrary concepts within the SyncAdapter, and each is synced individually.

There's an outstanding bug to provide per-engine UI (Bug 753878), but there's unlikely to be alignment there.

Comment 4

[Richard Newman [:rnewman]]

Throwing this at you, Nick. Reassign if you're overburdened.

CCing folks for radar.

Comment 5

[Allison Naaktgeboren :ally]

blocks a blocker

Comment 6

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e2a092e3f00

Comment 7

[Allison Naaktgeboren :ally]

bah. error between seat & keyboard.

Comment 8

[Richard Newman [:rnewman]]

Putting in a blocking relationship for Aurora reminder.

Nick, please remember to set Target Milestone when you land on inbound.

Comment 9

[Richard Newman [:rnewman]]

Attachment: Patch for uplift.

Comment 10

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/0e2a092e3f00

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 629385 [details] [diff] [review]
Patch for uplift.

[Triage Comment]

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

approved for beta since this fix is already on aurora now

Comment 13

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-beta/rev/00bf9c9ad298

Comment 14

[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 Fennec password
5. sync device and observe in the logs that Sync aborts after 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 stage 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_Decrypt returned error -8177: The security password entered is incorrect.
...
E FxSync(23437)               org.mozilla.firefox :: PasswordsRepoSession :: Got null cursor exception in PasswordsRepoSession.fetchSince
E FxSync(23437)               org.mozilla.firefox :: PasswordsRepoSession :: Exception in fetch.
W FxSync(23437)               org.mozilla.firefox :: RecordsChannel :: onFetchFailed. Calling for immediate stop.
W FxSync(23437)               org.mozilla.gecko.sync.repositories.NullCursorException

6. observe 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@4153a118)...