1140523 - EME plugins don't send out correct onEnabling/onDisabling notifications when preferences change

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

The code assumes that when one of the two prefs that control whether the plugin is enabled or not change then the plugins state must have changed when it may not have.

Comment 1

[Dave Townsend [:mossop]]

[Tracking Requested - why for this release]:

My understanding is that EME is going to be enabled on some platforms in 37 so we should track this bug too. It will cause currently open add-ons manager windows to behave strangely if the user toggles one of the prefs from elsewhere (the global pref is apparently going to be in the Firefox UI somewhere).

Comment 2

[Lawrence Mandel [:lmandel] (use needinfo)]

EME will not ship in 37 so I'm not tracking for this release. Tracking for 38+ as 38 is the current target to ship EME.

Comment 3

[Stephen A Pohl [:spohl]]

Attachment: Patch

I've tried to address this, but there are two situations that I'm not sure how to handle. When an EME GMP is both appDisabled and userDisabled and the user enables the global pref (appDisabled becomes false), or when the GMP is userDisabled and the user disables the global pref (appDisabled becomes true), then we currently don't send out any notification since the GMP is still disabled. However, the behavior in the addons manager is slightly different when we're appDisabled or userDisabled. In these scenarios here, we fail to make the "ask-to-activate/always-activate/never-activate" drop-down list selectable (if the GMP is only userDisabled) or unselectabe (if the GMP becomes appDisabled) if the addons manager is open at the same time as the pref is flipped. As soon as the addons manager is refreshed, the drop-down list appears properly. Instead of not sending out any notification, should I rather send out an onEnabled followed by an immediate onDisabled notification? Or is it better to keep it the way it is because the GMP never actually becomes enabled, just a different form of disabled?

Comment 4

[Stephen A Pohl [:spohl]]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec086037cfe7

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 8577808 [details] [diff] [review]
Patch

Review of attachment 8577808 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/internal/GMPProvider.jsm
@@ +396,5 @@
>                                             "onEnabled" : "onDisabled",
>                                             this);
>    },
>  
> +  onPrefEMEGlobalEnabledChanged: function() {

Sending this notification here should cause the UI to update accordingly:

AddonManagerPrivate.callAddonListeners("onPropertyChanged", this, ["appDisabled"])

Comment 6

[Stephen A Pohl [:spohl]]

Attachment: Patch

(In reply to Dave Townsend [:mossop] from comment #5)
> ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm
> @@ +396,5 @@
> >                                             "onEnabled" : "onDisabled",
> >                                             this);
> >    },
> >  
> > +  onPrefEMEGlobalEnabledChanged: function() {
> 
> Sending this notification here should cause the UI to update accordingly:
> 
> AddonManagerPrivate.callAddonListeners("onPropertyChanged", this,
> ["appDisabled"])

That's it! Thanks, Dave! :-)

Updated patch, carrying over r+.

Comment 7

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a666d175b157

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I think this should basically be covered by regular EME testing, so setting as qe-verify-. Please set as qe-verify+ if you think any dedicated manual testing is needed here.

Comment 10

[Stephen A Pohl [:spohl]]

Comment on attachment 8578160 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Adobe EME
[User impact if declined]: EME plugins will not send out the proper onEnabling/onDisabling notifications in the addons manager UI when the user toggles the global EME pref, which can result in misleading UI (EME plugins may appear active when they aren't and vice-versa).
[Describe test coverage new/current, TreeHerder]: Local testing confirms that this is working properly. Has been on m-c since yesterday without any reported issues.
[Risks and why]: Limited to GMP and Adobe EME in particular.
[String/UUID change made/needed]: none.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8578160 [details] [diff] [review]
Patch

EME matters, taking it.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/08d7c1951e61