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

RESOLVED WONTFIX

Status

()

Firefox
Sync
RESOLVED WONTFIX
11 months ago
4 months ago

People

(Reporter: tcsc, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

11 months ago
Created attachment 8885913 [details]
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

11 months ago
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.

Updated

11 months ago
Priority: -- → P2
(Reporter)

Updated

11 months ago
Depends on: 1382044

Comment 2

10 months ago
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...
Flags: needinfo?(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.
Flags: needinfo?(rhelmer)

Updated

10 months ago
Assignee: nobody → markh

Comment 4

10 months ago
Thanks Rob, let's not bother then!
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → WONTFIX
(Reporter)

Updated

6 months ago
See Also: → bug 1427835
(Reporter)

Comment 5

6 months ago
Reopening this to investigate why it seems to have gotten worse (bug 1427835)
Assignee: markh → nobody
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Reporter)

Comment 6

5 months ago
Clearing priority so we re-triage this. (Forgot to do it when reopening)
Priority: P2 → --
Flags: needinfo?(tchiovoloni)
(Reporter)

Comment 7

4 months ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 10 months ago4 months ago
Flags: needinfo?(tchiovoloni)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.