Updating addon userDisabled when it is appDisabled may not send an update notification

VERIFIED FIXED in Firefox 55

Status

()

VERIFIED FIXED
a year ago
a year ago

People

(Reporter: markh, Assigned: markh)

Tracking

Trunk
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox53 wontfix, firefox54 wontfix, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
Via bug 1366244, which has verbose logs.

Exact STR are a little painful to describe, but the tl;dr is:

* In <54, install some addon that's auto-disabled in 55, sync.
* When 55 starts, it auto-disables the addon.
* When 55 syncs, it finds an incoming record for the same item which indicates the .userDisabled state should be false (ie, enabled) - addons reports it as currently being true (ie, disabled).
* Sync tries to update the state.
* 55 declines to send addonlisteners a message that the state failed to flip. Sync waits for async listeners to fire to make progress.

Specifically:
* Here is where Sync checks oldState != newState, and doesn't make progress until its listener is called - http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/services/sync/modules/addonutils.js#491

* Here is where XPIProvider short-circuits without sending a notification - http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/toolkit/mozapps/extensions/internal/XPIProvider.jsm#5570

Rob, do you have any advice?
Flags: needinfo?(rhelmer)
(Assignee)

Updated

a year ago
Duplicate of this bug: 1366244
(Assignee)

Updated

a year ago
Summary: Trapping in the syncing state after attempt to workaround bug 1234181 → Updating addon userDisabled when it is appDisabled may not send an update notification
The userDisabled setter/getter is synchronous, so you shouldn't need to wait for a notification in any case, unless I'm missing the point :)
Flags: needinfo?(rhelmer)
(Assignee)

Comment 3

a year ago
(In reply to Robert Helmer [:rhelmer] from comment #2)
> The userDisabled setter/getter is synchronous, so you shouldn't need to wait
> for a notification in any case, unless I'm missing the point :)

huh, yeah, that makes sense. I guess I was assuming there's a good reason for the way Sync does it, but I see that mossop made the same point when this initially landed back in bug 534956 comment 27 - so yeah, I'll see if we can remove this complexity. Thanks.
(Assignee)

Comment 4

a year ago
While this works, it still seems a little wrong. The addon's userDisabled state does toggle and persist correctly, so it's not as though the operation is truly a no-op - arbitrary listeners missing that notification doesn't seem ideal (but yeah, probably doesn't matter much in this case)

I've got a patch where sync can work around this though, which I'll put up ASAP.
(Assignee)

Comment 5

a year ago
(In reply to Mark Hammond [:markh] from comment #4)
> While this works, it still seems a little wrong.

OMG - it seems this "problem" was known way back when the engine was added - addonutils.js already knows to not expect a notification from appDisabled addons, but it has a simple condition inverted:

http://searchfox.org/mozilla-central/rev/35b37316149108d53a02fb8498e250ea8a22ab5d/services/sync/modules/addonutils.js#493 should read |if (addon.appDisabled) {|

So somewhat bizarrely, this means that forever, Sync has made the callback *twice* for normal addons, and *never* for appDisabled addons.

However, simply fixing that condition doesn't solve all problems - a second listener in addonsreconciler.js also expects to see the notification so that its current view of the state is accurate - and without that Sync will re-upload the record for the appDisabled addon with the exact same state it had before.

So the patch I'm about to upload fixes both these problems, and adds a test that demonstrates both problems. It also adds the addon manager log output to the sync log, which could help with diagnostics (by default it's at WARN level, which seems fine)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Assignee: nobody → markh
status-firefox53: --- → wontfix
status-firefox54: --- → affected
status-firefox55: --- → affected

Comment 7

a year ago
mozreview-review
Comment on attachment 8871566 [details]
Bug 1366994 - prevent appDisabled addons from hanging sync.

https://reviewboard.mozilla.org/r/143050/#review147952

Looks great to me. This is a substantial simplification too!

::: services/sync/modules/addonutils.js:407
(Diff revision 1)
>     *        (bool) New value for add-on's userDisabled property.
> -   * @param cb
> -   *        (function) Callback to be invoked on completion.
>     */
> -  updateUserDisabled: function updateUserDisabled(addon, value, cb) {
> +  updateUserDisabled(addon, value) {
>      if (addon.userDisabled == value) {

Doesn't seem like this check matters that much anymore, and we could always run the body of the function (unless the addon has a setter?)
Attachment #8871566 - Flags: review?(tchiovoloni) → review+
(Assignee)

Comment 8

a year ago
mozreview-review-reply
Comment on attachment 8871566 [details]
Bug 1366994 - prevent appDisabled addons from hanging sync.

https://reviewboard.mozilla.org/r/143050/#review147952

> Doesn't seem like this check matters that much anymore, and we could always run the body of the function (unless the addon has a setter?)

Thanks! Yeah, this invokes a setter. A brief look shows that also short-circuits, but I figured I shouldn't push my luck too far, and this short-circuit might as well stay - so unless you object strongly, I think I'll leave it as is.

Comment 9

a year ago
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/c25cbb6d95e8
prevent appDisabled addons from hanging sync. r=tcsc

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c25cbb6d95e8
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
I can confirmed that my profile starts syncing again. Thanks!
(Assignee)

Comment 12

a year ago
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #11)
> I can confirmed that my profile starts syncing again. Thanks!

Thanks for getting back to us!
Status: RESOLVED → VERIFIED
(Assignee)

Comment 13

a year ago
Comment on attachment 8871566 [details]
Bug 1366994 - prevent appDisabled addons from hanging sync.

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: Sync may hang when syncing addons
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Risk is limited to Sync
[Why is the change risky/not risky?]:
[String changes made/needed]: None

Although we don't have stats for this, any addons which are "blocked" by the browser itself may have this problem. This will be a significant problem in 55 (when many addons are disabled due to e10s), and while it has been fixed there, it seems worthwhile getting it into beta.

That said, if it is considered too risky for beta this late in the cycle it wouldn't be the end of the world.
Attachment #8871566 - Flags: approval-mozilla-beta?
Comment on attachment 8871566 [details]
Bug 1366994 - prevent appDisabled addons from hanging sync.

It's very late in Beta54 and it would be too risky to take this in Beta54. Let it ride the train. Beta54-.
Attachment #8871566 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
status-firefox54: affected → wontfix
You need to log in before you can comment on or make changes to this bug.