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

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

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

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?

Comment 2

[Robert Helmer [:rhelmer]]

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

Comment 3

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

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

Comment 4

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

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.

Comment 5

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

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

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

Attachment: Bug 1366994 - prevent appDisabled addons from hanging sync.

Review commit: https://reviewboard.mozilla.org/r/143050/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/143050/

Comment 7

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

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

Comment 8

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

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c25cbb6d95e8
prevent appDisabled addons from hanging sync. r=tcsc

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I can confirmed that my profile starts syncing again. Thanks!

Comment 12

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

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

Comment 13

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

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.

Comment 14

[Gerry Chang [:gchang]]

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