Closed Bug 1294439 Opened 8 years ago Closed 8 years ago

Add-ons aren't marked as softDisabled if they are already appDisabled

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox49 + fixed
firefox50 + fixed
firefox51 + fixed

People

(Reporter: mossop, Assigned: mossop)

References

Details

Attachments

(1 file)

If an add-on is already inactive because of app compatibility or other reasons when a new blocklist update marks it as unstable we don't set the softDisabled flag. This means that if the add-on subsequently becomes compatible again it will be fully enabled and the user will get no warning to disable it. The problem stems from bailing out without doing anything here:

https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/nsBlocklistService.js#1384

I'm not sure if the fix is as trivial as setting softBlocked regardless or not, I recall some complexity around how that property worked :(
Tracking since this sounds like a flaw in addon signing that we may want to fix all the way up to beta.
This is unrelated to add-on signing really
> Tracking since this sounds like a flaw in addon signing that we may want to fix all the way up to beta.

The issue is in the addon blocklist, Mossop is still investigating the other issue which is related to addon signing (https://bugzilla.mozilla.org/show_bug.cgi?id=1286368#c33).

If this turns out to be a simple fix, I'd suggest uplifting to release. It would fix the #15 crasher, #2 startup crasher, with > 1000 crashes over the last week (and probably other issues as well with other blocklisted addons).
(In reply to Marco Castelluccio [:marco] from comment #3)
> > Tracking since this sounds like a flaw in addon signing that we may want to fix all the way up to beta.
> 
> The issue is in the addon blocklist, Mossop is still investigating the other
> issue which is related to addon signing
> (https://bugzilla.mozilla.org/show_bug.cgi?id=1286368#c33).
> 
> If this turns out to be a simple fix, I'd suggest uplifting to release. It
> would fix the #15 crasher, #2 startup crasher, with > 1000 crashes over the
> last week (and probably other issues as well with other blocklisted addons).

FWIW I think the damage is already done here. Any fix to this wouldn't affect already existing blocks
Assignee: nobody → dtownsend
Comment on attachment 8780207 [details]
Bug 1294439: Mark add-ons as soft disabled even if they are app disabled.

https://reviewboard.mozilla.org/r/70944/#review68432

::: toolkit/mozapps/extensions/nsBlocklistService.js:1388
(Diff revision 1)
>  
>          // If the add-on is already disabled for some reason then don't warn
>          // about it
> -        if (!addon.isActive)
> +        if (!addon.isActive) {
> +          // But mark it as softblocked if not already user disabled.
> +          if (state == Ci.nsIBlocklistService.STATE_SOFTBLOCKED && !addon.userDisabled)

A brief comment here explaining the desired (userDisabled XOR softDisabled) invariant would be helpful for readers not familiar with that nuance...

::: toolkit/mozapps/extensions/test/xpcshell/test_softblocked.js:59
(Diff revision 1)
> +      Services.obs.removeObserver(arguments.callee, "blocklist-updated");
> +
> +      resolve();
> +    }, "blocklist-updated", false);
> +
> +    Services.prefs.setCharPref("extensions.blocklist.url", "http://localhost:" + gPort + "/data/" + aFile);

nitpick, a template string would be nice and readable here
Attachment #8780207 - Flags: review?(aswan) → review+
Comment on attachment 8780207 [details]
Bug 1294439: Mark add-ons as soft disabled even if they are app disabled.

https://reviewboard.mozilla.org/r/70944/#review68460

::: toolkit/mozapps/extensions/nsBlocklistService.js:1390
(Diff revisions 1 - 2)
>          // about it
>          if (!addon.isActive) {
> -          // But mark it as softblocked if not already user disabled.
> +          // But mark it as softblocked if necessary. Note that we avoid setting
> +          // softDisabled at the same time as userDisabled to make it clear
> +          // which was the original cause of the add-on becoming disabled in a
> +          // way that the user can change. 

trailing whitespace...
(In reply to Dave Townsend [:mossop] from comment #4)
> FWIW I think the damage is already done here. Any fix to this wouldn't
> affect already existing blocks

Note that we haven't pushed 48.0 to all users yet, so there's still time to avoid the damage for most of our users.
(In reply to Marco Castelluccio [:marco] from comment #10)
> (In reply to Dave Townsend [:mossop] from comment #4)
> > FWIW I think the damage is already done here. Any fix to this wouldn't
> > affect already existing blocks
> 
> Note that we haven't pushed 48.0 to all users yet, so there's still time to
> avoid the damage for most of our users.

But the blocklist change has happened, and that is when the problem occurs. You could conceivably roll back the block, wait for most users to get the update and then reinstate it but that means unblocking the add-on and probably raising the crash rate initially.
(In reply to Dave Townsend [:mossop] from comment #11)
> But the blocklist change has happened, and that is when the problem occurs.
> You could conceivably roll back the block, wait for most users to get the
> update and then reinstate it but that means unblocking the add-on and
> probably raising the crash rate initially.

Ok, I thought this bug was a regression in 48.
Turns out that this has existed since we implemented softblocking in the blocklist service.
Pushed by dtownsend@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b4e9c7e20cf8
Mark add-ons as soft disabled even if they are app disabled. r=aswan
Comment on attachment 8780207 [details]
Bug 1294439: Mark add-ons as soft disabled even if they are app disabled.

Approval Request Comment
[Feature/regressing bug #]: This bug has existed since softblocking was implemented
[User impact if declined]: In certain cases users will get add-ons automatically enabled when they should be disabled by default
[Describe test coverage new/current, TreeHerder]: Good tests on m-c
[Risks and why]: Low risk, this is just a small change to handle the specific case that is a problem and it is well tested.
[String/UUID change made/needed]: None
Attachment #8780207 - Flags: approval-mozilla-beta?
Attachment #8780207 - Flags: approval-mozilla-aurora?
Comment on attachment 8780207 [details]
Bug 1294439: Mark add-ons as soft disabled even if they are app disabled.

Belay that...
Attachment #8780207 - Flags: approval-mozilla-beta?
Attachment #8780207 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/b4e9c7e20cf8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Hi :mossop,
Since this bug is a regression and also affects 49/50, are you also considering to uplift this patch to 49/50?
Flags: needinfo?(dtownsend)
(In reply to Gerry Chang [:gchang] from comment #19)
> Hi :mossop,
> Since this bug is a regression and also affects 49/50, are you also
> considering to uplift this patch to 49/50?

Yes, just needed to wait for it to actually land.
Flags: needinfo?(dtownsend)
Comment on attachment 8780207 [details]
Bug 1294439: Mark add-ons as soft disabled even if they are app disabled.

Approval Request Comment - see comment 16
Attachment #8780207 - Flags: approval-mozilla-beta?
Attachment #8780207 - Flags: approval-mozilla-aurora?
Comment on attachment 8780207 [details]
Bug 1294439: Mark add-ons as soft disabled even if they are app disabled.

Patch contains new automated test (yay!), let's uplift to Aurora50.
Attachment #8780207 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8780207 [details]
Bug 1294439: Mark add-ons as soft disabled even if they are app disabled.

This patch fixes a flaw in addon signing. Take it in 49 beta. Should be in 49 beta 7.
Attachment #8780207 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.