Most addons aren't synced (vetoed by addon manager)

RESOLVED FIXED in Firefox 50

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tcsc, Assigned: markh)

Tracking

unspecified
Firefox 52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed, firefox51 fixed, firefox52 fixed)

Details

Attachments

(1 attachment)

Haven't dug into this (and it's possible it's only true for me...), but if I force a full resync of addons, most of my addons don't make it up to the server -- even ones that should.

Logs are full of lines like `SQLiteManager@mrinalkant.blogspot.com not syncable: vetoed by the addon manager`, etc.
Priority: -- → P1
Assignee: nobody → markh
The problem here is that addons already installed by the user when isSyncable landed do not have that isSyncable attribute in the addonsreconciler.json - only new addons installed since then get that attribute. So when the state is loaded from that file, isSyncable is undefined, so treated as false.

There were 2 approaches I could have taken in this patch:

* change the version number stored in addonsreconciler.json. The downside if this approach is that Firefox would discard the file entirely, then consider every addon "new", so re-upload the records. This would have worked but seems unnecessary - this file is really only used so Firefox knows the "old" state of an addon (eg, the enabled state), and then when it looks at the current actual state of the addon it can know if a change happened so the record is uploaded. Given the isSyncable attribute doesn't change the state of the record on the server, there's no actual need to have it in the file (ie, if somehow this attribute changed for an addon between the last sync and now, we don't need to consider that something forcing an upload.

* Always add an isSyncable attribute at runtime - we've already got the addon instance, so this is cheap. There's actually no reason to have it in the .json at all, but it's difficult to arrange for it to not be there given how the reconciler is written.

So I just went for option 2 - if seems a safer option.

The other sad thing is that I can't see a reasonable way to test this
Comment on attachment 8805006 [details]
Bug 1312021 - ensure the addonsreconciler always has the isSyncable attribute.

https://reviewboard.mozilla.org/r/88828/#review88110

That would do it. Is this definitely the only property we need to do this for? (I'm assuming yes, but might be worth checking the code to see if there are others we rely on)
Attachment #8805006 - Flags: review?(tchiovoloni) → review+
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/ed22ec2c5892
ensure the addonsreconciler always has the isSyncable attribute. r=tcsc
Comment on attachment 8805006 [details]
Bug 1312021 - ensure the addonsreconciler always has the isSyncable attribute.

Approval Request Comment
[Feature/regressing bug #]: Bug 1275139
[User impact if declined]: Existing user addons will no longer be synced
[Describe test coverage new/current, TreeHerder]: Existing tests pass
[Risks and why]: a simple 1 line patch. Risk is limited to addon syncing.
[String/UUID change made/needed]: None
Attachment #8805006 - Flags: approval-mozilla-beta?
Attachment #8805006 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/ed22ec2c5892
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Comment on attachment 8805006 [details]
Bug 1312021 - ensure the addonsreconciler always has the isSyncable attribute.

Fix seems simple and low risk, Aurora51+
Attachment #8805006 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi Mark, we are in Code Freeze mode (RC week for 50) and only taking fixes for severe recent regressions, crashes, sec issues. Is this a new regression in 50? If this is a known issue in 49, we might have to live with another 6 weeks of this in Release 50?
Flags: needinfo?(markh)
Comment on attachment 8805006 [details]
Bug 1312021 - ensure the addonsreconciler always has the isSyncable attribute.

Patch looks simple, has stabilized on aurora and nightly, taking it, Beta50+
Attachment #8805006 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Ritu Kothari (:ritu) from comment #8)
> Hi Mark, we are in Code Freeze mode (RC week for 50) and only taking fixes
> for severe recent regressions, crashes, sec issues. Is this a new regression
> in 50? If this is a known issue in 49, we might have to live with another 6
> weeks of this in Release 50?

This is a regression in 50 and does not exist in 49 - it's unfortunate that it wasn't detected until this late in the cycle, but it would also be unfortunate to break Syncing addons in 50 when it works in 49.
Flags: needinfo?(markh) → needinfo?(rkothari)
Thanks Mark! I took it in RC1 and was happy to see the patch was a one-liner.
Flags: needinfo?(rkothari)
You need to log in before you can comment on or make changes to this bug.