Closed Bug 758382 Opened 9 years ago Closed 9 years ago

Handle stage errors in content stages by advancing, not aborting

Categories

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

ARM
Android
defect

Tracking

(firefox14 verified, blocking-fennec1.0 +)

VERIFIED FIXED
mozilla15
Tracking Status
firefox14 --- verified
blocking-fennec1.0 --- +

People

(Reporter: rnewman, Assigned: nalexander)

References

()

Details

(Whiteboard: [sync])

Attachments

(1 file)

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.
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.
Priority: -- → P1
(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.
(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.
Throwing this at you, Nick. Reassign if you're overburdened.

CCing folks for radar.
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Whiteboard: [needs review :liuche]
blocks a blocker
tracking-fennec: --- → ?
bah. error between seat & keyboard.
tracking-fennec: ? → ---
blocking-fennec1.0: --- → ?
Whiteboard: [needs review :liuche] → [sync]
Putting in a blocking relationship for Aurora reminder.

Nick, please remember to set Target Milestone when you land on inbound.
Blocks: 736393
Target Milestone: --- → mozilla15
Attachment #629385 - Flags: review+
Attachment #629385 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/0e2a092e3f00
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
blocking-fennec1.0: ? → +
Comment on attachment 629385 [details] [diff] [review]
Patch for uplift.

[Triage Comment]
Attachment #629385 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
approved for beta since this fix is already on aurora now
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)...
Status: RESOLVED → VERIFIED
Product: Mozilla Services → Android Background Services
Product: Android Background Services → Firefox for Android
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.