30.10 KB, patch
|Details | Diff | Splinter Review|
4.64 KB, patch
|Details | Diff | Splinter Review|
Created attachment 663156 [details] [diff] [review] Log from client receiving wipe STR: 1) Install some add-ons 2) Set up Sync 3) Device A perform a reset sync to replace server data with local 4) Wait for Device B to sync 5) Add-ons are uninstalled as part of wipe and never reinstalled The bug is in reconciling and the addons reconciler. Essentially, the uninstall as part of wipe is being picked up as a local change. When the incoming records are applied, there are local changes newer than the server's so the server record is ignored. The local client then uploads the deleted record to the server, propagating uninstall to other clients.
I suspect this impacts Firefox all the way back to when add-on sync was released (Firefox 11 IIRC). Tracy: could you verify with Firefox 15? If it affects 15, it will affect 16 and 17.
status-firefox18: --- → affected
tracking-firefox18: --- → ?
yes, they are all affected.
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
tracking-firefox15: --- → ?
tracking-firefox16: --- → ?
tracking-firefox17: --- → ?
(In reply to Gregory Szorc [:gps] from comment #0) > When the incoming records are applied, there are local changes > newer than the server's so the server record is ignored. That's not an accurate statement. But, it's close enough.
Created attachment 663179 [details] [diff] [review] Handle updates to deleted add-ons properly, v1 Patch is trivial. I'm not feeling well, so extra eyes would be appreciated. We could also fix this by changing itemExists. Perhaps I should throw in a TPS test as well? I haven't verified this patch yet. But, the unit test failed on old code and now passes.
Comment on attachment 663179 [details] [diff] [review] Handle updates to deleted add-ons properly, v1 Patch only works for restartless add-ons. I'm definitely going to want a TPS test here.
Created attachment 663213 [details] [diff] [review] Handle updates to deleted add-ons properly, v2 This handles non-restartless add-ons properly. I also added a TPS test. I verified the TPS test fails before the patch and works with the patch. I'm pretty confident the cancelUninstall call is proper. It is a documented API after all. I figure Blair will tell me otherwise. I figure one r+ for landing on inbound and 2 r+ and Blair's f+ (and approval) for landing on aurora and beta.
Comment on attachment 663213 [details] [diff] [review] Handle updates to deleted add-ons properly, v2 [Approval Request Comment] Bug caused by (feature/regressing bug #): 534956 (initial add-on sync landing in Fx 11) User impact if declined: Resetting Sync causes all add-ons to be uninstalled and never return Testing completed (on m-c, etc.): None yet. Have auotmated tests as part of patch. QA will verify once it lands in m-c. Risk to taking this patch (and alternatives if risky): I don't believe it is too risky. We have pretty good test coverage of add-on sync. It's even better after this patch! String or UUID changes made by this patch: None I'm requesting patch approval before actual review because I want this to be on the release drivers' radar. If this patch changes, it's highly unlikely to deviate significantly from its current form.
Comment on attachment 663213 [details] [diff] [review] Handle updates to deleted add-ons properly, v2 Review of attachment 663213 [details] [diff] [review]: ----------------------------------------------------------------- I have a vague feeling that the reconciler should know about wipes, but this probably will suffice. I'm happy with the test coverage, at least. ::: services/sync/tests/tps/test_addon_wipe.js @@ +1,4 @@ > +/* Any copyright is dedicated to the Public Domain. > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +// This test verifies that behavior after wipe is proper. Explain what that is for the benefit of the reader. It's probably not the correct use of cutlery. ::: services/sync/tests/unit/test_addons_store.js @@ +434,5 @@ > + Svc.Prefs.set("addons.ignoreRepositoryChecking", true); > + store.wipe(); > + > + let addon = getAddonFromAddonManagerByID(addon1.id); > + do_check_null(addon); If you're following the "addon1" style, this should be addon2. But I'd prefer these three `addon` vars to be named: let installed = … let fetchDeleted = … let fetchInstalled = …
Attachment #663213 - Flags: review?(rnewman) → review+
This flow should be part of regular QA. Setting flag for litmus.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbe80d33bbcd https://hg.mozilla.org/services/services-central/rev/3e2c523a5c6f Waiting on Unfocused, mconnor, and release drivers for Aurora and Beta.
Whiteboard: [fixed in services]
Target Milestone: --- → mozilla18
Not chemspill-worthy on its own so we won't track this for 15. Let's get QA verification on the mozilla-central build with this landed and confirm before we evaluate the branch uplift.
tracking-firefox15: ? → -
tracking-firefox16: ? → +
tracking-firefox17: ? → +
tracking-firefox18: ? → +
Keywords: qawanted, verifyme
Tracy: when verifying this, don't use services-central builds. That tree is significantly different from m-c and you may encounter bugs related to that difference. This won't make it into mozilla-central for a few hours yet - PGO builds for inbound are still churning. Last realistic beta goes out on Tuesday. Hopefully we can have this verified by Monday so release drivers can ack it in time to get it in.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
status-firefox18: affected → fixed
Resolution: --- → FIXED
Comment on attachment 663213 [details] [diff] [review] Handle updates to deleted add-ons properly, v2 Review of attachment 663213 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Gregory Szorc [:gps] from comment #6) > I'm pretty confident the cancelUninstall call is proper. Yep. > I figure one r+ for landing on inbound and 2 r+ and Blair's f+ (and > approval) for landing on aurora and beta. I'm no gatekeeper to aurora/beta :)
Attachment #663213 - Flags: feedback?(bmcbride) → feedback+
Verified with Nightly builds of 20120924. Add-ons are no longer deleted as a result of a reset.
Status: RESOLVED → VERIFIED
Comment on attachment 663213 [details] [diff] [review] Handle updates to deleted add-ons properly, v2 [Triage Comment] We think this bug has been around since FF11, and the risk profile is not zero or near-zero, so given where we are in the cycle (beta 5 of 6) we're going to punt to FF17. Approved for Aurora 17.
status-firefox17: affected → fixed
status-firefox15: affected → wontfix
status-firefox16: affected → wontfix
Alex: Are you serious? Are you sure you can't get it into FF 16? People are really waiting for this fix. It would be worth some delay. I don't want to wait until FF 17...
Frank, Reset Sync is a fairly rare action, as far as we know. Who do you believe would be waiting for a fix?
Mike: For example the people (dis)cussing at http://my.opera.com/rejzor/blog/2012/04/24/firefox-sync-the-stupidest-and-most-unreliable-thing-ever and https://bugzilla.mozilla.org/show_bug.cgi?id=788417 . Keeping in mind that this is just the tip of the iceberg and that people starting with sync are tempted to use Reset more often than we might think, I find Gregory's fix quite important and I appreciate he has provided it so fast.
One way to work-around this is to turn off add-ons sync, do your Reset, then re-enable add-ons sync. However, it is possible Reset has put add-ons sync in a broken state. In this case, you may have to unlink all but one device from your account. Then get that one device to the state you want it (add-ons installed). Finally, pair each of your other devices back to your account.
Whiteboard: [fixed in services]
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.