Closed Bug 736393 Opened 9 years ago Closed 9 years ago

Don't abort on store failure

Categories

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

ARM
Android
defect

Tracking

(blocking-fennec1.0 +)

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

People

(Reporter: rnewman, Assigned: nalexander)

References

Details

Attachments

(1 file)

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.
Calling this a P1 to either fix or WONTFIX.
Priority: -- → P1
Assignee: nobody → nalexander
Blocks: 747746
Nomming because of blocking 747746
blocking-fennec1.0: --- → ?
mobile triage: +
blocking-fennec1.0: ? → +
Attachment #628859 - Flags: review+
Extra points if you capitalize "Don't" in the commit message before you land.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla15
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:
Attachment #628859 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/5e22cc18646d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #628859 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I'd like this to land on Aurora alongside Bug 760614 and/or the stage continuation bug. nalexander, please bear that in mind.
(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.
Depends on: 760614
Depends on: 758382
Attachment #628859 - Flags: approval-mozilla-aurora+ → approval-mozilla-beta+
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)...
Status: RESOLVED → VERIFIED
Product: Mozilla Services → Android Background Services
Attachment #628859 - Attachment is patch: true
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.