Closed Bug 618389 Opened 9 years ago Closed 9 years ago

changePassphrase doesn't discard bulk keys

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta8+
fennec 2.0b3+ ---

People

(Reporter: rnewman, Assigned: rnewman)

Details

Attachments

(2 files, 2 obsolete files)

Just noticed this. It'll rewrap, and on the next sync politely note that the keys it pulls from the server are the same as the ones it already has.
mconnor, your call whether this is a blocker. Should have a patch momentarily.
blocking2.0: --- → ?
tracking-fennec: --- → ?
Target Milestone: --- → 2.0
Attachment #496875 - Flags: review?(mconnor)
This approach adds the logic to resetClient.

Tested with xpcshell-tests only. Thoughts?
Attachment #496875 - Attachment is obsolete: true
Attachment #496905 - Flags: review?(mconnor)
Attachment #496875 - Flags: review?(mconnor)
blocking2.0: ? → beta8+
tracking-fennec: ? → 2.0b3+
Attachment #496905 - Flags: review?(mconnor) → review+
Is this ready to go?
I think there's a bug in this that's causing CrossWeave to fail.

Looking now.
The previous (committed!) change would wipe the local key cache in an unfortunate spot, resulting in an empty cache when we come to perform an operation.

That caused a CrossWeave failure (though real clients would display an error and recover on the next sync). Fortunately we hadn't yet merged to m-c.

This fix removes the CK clearing operation from resetClient, and adds it explicitly where it's safe to do so. Noisier, yes, but tests pass, CrossWeave is happy, and I manually verified all three Reset Sync options and generating a new passphrase.

Review, philiKON or mconnor?
Attachment #497007 - Flags: review?
This is the entire change as a single patch for ease of reviewing.
Attachment #496905 - Attachment is obsolete: true
Comment on attachment 497007 [details] [diff] [review]
Fix previous patch. v1

Looks good!

>+  clearKeys: function clearKeys() {
>+    CollectionKeys.clear();
>+  },
>+  

That seems more indirection than we need... I don't feel super strongly about it, but I'd remove it and just inline the call :)
Attachment #497007 - Flags: review? → review+
Fixed and pushed. Thanks for the review, Philipp.

http://hg.mozilla.org/services/fx-sync/rev/81ac19fb2666
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
And another fix. Too enthusiastic with key clearing; broke a history CrossWeave test.

http://hg.mozilla.org/services/fx-sync/rev/02500bf9fcc6
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.