792990 - Reset Sync causes all add-ons to be uninstalled

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Gregory Szorc [:gps]]

Attachment: 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.

Comment 1

[Gregory Szorc [:gps]]

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.

Comment 2

[Tracy Walker [:tracy]]

yes, they are all affected.

Comment 3

[Gregory Szorc [:gps]]

(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.

Comment 4

[Gregory Szorc [:gps]]

Attachment: 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 5

[Gregory Szorc [:gps]]

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.

Comment 6

[Gregory Szorc [:gps]]

Attachment: 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 7

[Gregory Szorc [:gps]]

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 8

[Richard Newman [:rnewman]]

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 = …

Comment 9

[Richard Newman [:rnewman]]

This flow should be part of regular QA. Setting flag for litmus.

Comment 10

[Gregory Szorc [:gps]]

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.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 12

[Gregory Szorc [:gps]]

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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bbe80d33bbcd

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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 :)

Comment 15

[Tracy Walker [:tracy]]

Verified with Nightly builds of 20120924.  Add-ons are no longer deleted as a result of a reset.

Comment 16

[Alex Keybl [:akeybl]]

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.

Comment 17

[Gregory Szorc [:gps]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dcd3a21a3519

Comment 18

[Frank Breitling]

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...

Comment 19

[Mike Connor [:mconnor]]

Frank, Reset Sync is a fairly rare action, as far as we know.  Who do you believe would be waiting for a fix?

Comment 20

[Frank Breitling]

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.

Comment 21

[Tracy Walker [:tracy]]

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.

Comment 23

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/rev/3e2c523a5c6f