Closed Bug 629463 Opened 9 years ago Closed 9 years ago

Follow on: take action on server records that cause local failures

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

Attachments

(1 file)

Follow on from Bug 622762, to decide if and when to re-upload a server record that we're having trouble processing.
Depends on: 622762
... and to decide (probably on a per-engine basis) whether to delete the server record directly. That's what we ought to do for client records with HMAC mismatches, at least.
We should at least delete broken Client records, or add similar exception-eating logic there.  But really, busted client records should be nuked for sure.
Tests and crossweave pass. Uses existing HMAC error call! Hooray!
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Attachment #515965 - Flags: review?(philipp)
Comment on attachment 515965 [details] [diff] [review]
Proposed patch. v1

At first I thought I wasn't going to find anything to criticize, but then I did and the world made sense again!

>+    _("Records now: " + clientsColl.get({}));
>+    _("Change our keys and our client ID, reupload keys.");
>+    Clients.localID = Utils.makeGUID();
>+    CollectionKeys.generateNewKeys();
>+    let serverKeys = CollectionKeys.asWBO("crypto", "keys");
>+    serverKeys.encrypt(Weave.Service.syncKeyBundle);
>+    do_check_true(serverKeys.upload(Weave.Service.cryptoKeysURL).success);
>+
>+    _("Sync.");
>+    do_check_true(!deleted);
>+    Clients._resetClient();

Why call _resetClient instead of resetClient here? Also (small nit), this line should probably go next to the reassignment of Clients.localID.
Attachment #515965 - Flags: review?(philipp) → review+
Comment on attachment 515965 [details] [diff] [review]
Proposed patch. v1

mconnor, a?

This allows users to break out of the "constant errors as a result of a bad client record" loop.
Attachment #515965 - Flags: approval2.0?
Comment on attachment 515965 [details] [diff] [review]
Proposed patch. v1

a=beltzner
Attachment #515965 - Flags: approval2.0? → approval2.0+
Comment on attachment 515965 [details] [diff] [review]
Proposed patch. v1

Phil r-'d but forgot to change the flag; that changes my decision!
Attachment #515965 - Flags: review-
Attachment #515965 - Flags: review+
Attachment #515965 - Flags: approval2.0-
Attachment #515965 - Flags: approval2.0+
(In reply to comment #7)
> Phil r-'d but forgot to change the flag; that changes my decision!

Huh?!? I r+'d!
(My comments were just nits... I was being a bit sarcastic in the first line, sorry if that was misleading. I was commenting on the fact that there's nothing wrong with the patch which can't obviously be! See I did it again...)
Attachment #515965 - Flags: review-
Attachment #515965 - Flags: review+
Attachment #515965 - Flags: approval2.0?
Attachment #515965 - Flags: approval2.0-
Comment on attachment 515965 [details] [diff] [review]
Proposed patch. v1

a=beltzner
Attachment #515965 - Flags: approval2.0? → approval2.0+
Pushed to fx-sync:

http://hg.mozilla.org/services/fx-sync/rev/5622ff3299b4
Whiteboard: [fixed in fx-sync]
Merged to s-c: http://hg.mozilla.org/services/services-central/rev/76784798d9a7
Whiteboard: [fixed in fx-sync] → [fixed in fx-sync][fixed in services]
Merged to m-c: http://hg.mozilla.org/mozilla-central/rev/76784798d9a7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in fx-sync][fixed in services]
Duplicate of this bug: 637258
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.