1380472 - It's possible to end up with two records on the server representing the same addon

Status: RESOLVED WONTFIX

(Firefox :: Sync, defect)

Description

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: test_addon_serverdupe.js

It appears that the following case reliably reproduces an issue with a single profile on both mac and linux (haven't tried windows). I've attached a TPS test that reproduces the issue. It also causes `test_addon_nonrestartless_xpi.js` to fail in automation. (Note: I haven't tried to reproduce it outside of TPS)

1. Install non-restartless addon
2. Sync
3. Restart
4. Sync
5. Disable addon installed in first step
6. Sync

The server should now have two records, one saying the addon should be enabled, the other saying it should be disabled, but they both have different GUIDs. This causes the addon validator to fail.

It appears that when sync is starting up between steps 3 and 4 (after the restart), we hit http://searchfox.org/mozilla-central/source/services/sync/modules/addonsreconciler.js#467, and change the GUID of the already synced record (I'm not sure where the new GUID comes from).

Then, when we disable the addon, we mark it as changed (with the new GUID), and upload the a record for the same addon, but with a different GUID and setting for `enabled`.

[[I don't understand how this is intended to work, so it's possible this is either not a bug, or a known issue (If this is the case, the addon validator should be disabled for the tps test failing in automation)]]

I haven't been able to repro with a restartless addon, but since I don't know what exactly is happening here, it's possible that it just has slightly different repro steps.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Thanks for finding that Thom - it sounds bad. I guess the first step should be to get a test in xpcshell, then identify what's going wrong, and finally to try and determine how likely people are to find this problem in the wild and what remediation we should attempt.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Rob, I'm hoping you can help here. The general problem is that after installing a non-restartless addon, it gets assigned a different syncGUID after the restart. Best I can tell, on the restart the addon is *not* in the addons database, but XPIProvider.getInstallState() scans the install location, finds the XPI put there by the previous session, notices the "enabled" state is false, then continues with the install - at which point it *does* end up in the DB - but the next reference to addon.syncGUID assigns a new GUID (which then *does* end up in the DB, so that second GUID seems stable).

So I guess I'm asking:

* Is it correct that installing a non-restartless addon doesn't save metadata for these new addons but instead just drops the XPI in the profile?

And I guess the more meta question:

* Is it worth trying to fix this bug at all any more?  IIUC, because we don't sync system addons etc we are unlikely to ever hit this in practice in 57+, but I may well be missing something...

Comment 3

[Robert Helmer [:rhelmer]]

(In reply to Mark Hammond [:markh] from comment #2)
> Rob, I'm hoping you can help here. The general problem is that after
> installing a non-restartless addon, it gets assigned a different syncGUID
> after the restart. Best I can tell, on the restart the addon is *not* in the
> addons database, but XPIProvider.getInstallState() scans the install
> location, finds the XPI put there by the previous session, notices the
> "enabled" state is false, then continues with the install - at which point
> it *does* end up in the DB - but the next reference to addon.syncGUID
> assigns a new GUID (which then *does* end up in the DB, so that second GUID
> seems stable).

We're dropping support for non-restartless add-ons in 57 so while I am not surprised to hear that the behavior here is a bit wonky it might not be worth fixing at this point...

> So I guess I'm asking:
> 
> * Is it correct that installing a non-restartless addon doesn't save
> metadata for these new addons but instead just drops the XPI in the profile?
> 
> And I guess the more meta question:
> 
> * Is it worth trying to fix this bug at all any more?  IIUC, because we
> don't sync system addons etc we are unlikely to ever hit this in practice in
> 57+, but I may well be missing something...

All add-ons post-57 (incl. system add-ons) are required to be restartless now so we shouldn't hit it in any case moving forward.

If I missed something let me know, but sounds like we should probably WONTFIX.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Thanks Rob, let's not bother then!

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

Reopening this to investigate why it seems to have gotten worse (bug 1427835)

Comment 6

[Thom Chiovoloni [:tcsc] (ex-moco)]

Clearing priority so we re-triage this. (Forgot to do it when reopening)

Comment 7

[Thom Chiovoloni [:tcsc] (ex-moco)]

What comment 2 suggests seems to still be the case. It got worse in that it now happens every time instead of only happening when we install an addon via AddonsEngine.prototype.update.

However, I think the resolution is still that we don't care for the reasons outlined in comment 3.