1140522 - EME plugins aren't correctly using appDisabled, userDisabled and permissions properties

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

An app preference (KEY_EME_ENABLED) forces all EME plugins disabled which the user can't override but that is currently exposed through the userDisabled property. It should instead be part of the appDisabled property and permissions should be updated to indicate that the user cannot enable it in that case.

Comment 1

[Dave Townsend [:mossop]]

I'm told that KEY_EME_ENABLED will be user-controllable through some other UI.

This is kind of weird, I guess I would expect that to then hide all the EME plugins from the add-ons manager rather than make them look disabled, either way though there is certainly a problem with the permissions property, the user can't enable the plugin through the add-ons manager UI so it shouldn't claim they can. I still sort of think it should use the appDisabled but could be argued around depending on the UI that presents.

Comment 2

[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. This will cause the add-ons manager to suggest the user can enable a plugin when they can't.

Comment 3

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

Not tracking for 37 as EME won't ship in 37. (Bug I have added this to my 37 worry list.) Tracking 38+

Comment 4

[Stephen A Pohl [:spohl]]

Attachment: Patch

(In reply to Dave Townsend [:mossop] from comment #1)
> I'm told that KEY_EME_ENABLED will be user-controllable through some other
> UI.
> 
> This is kind of weird, I guess I would expect that to then hide all the EME
> plugins from the add-ons manager rather than make them look disabled

Yes, this is most likely how we're going to handle it. I have yet to hear final confirmation, but the plan is the following: When KEY_EME_ENABLED is toggled to false, all installed CDMs are uninstalled from disk and all CDMs are hidden from the addons manager. If a user toggles the pref back on, we should do the reverse and trigger a GMP check soon to install any CDMs that are available to the user.

In the meantime, I've tried to address the permissions issue reported in this bug, which I think should be addressed regardless.

Comment 5

[Stephen A Pohl [:spohl]]

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9000e74fa1fe

Realized that I forgot to update tests, so backed out in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14fda1809043

Notified jlund, KWierso and RyanVM in #releng that some test jobs will most likely burn between the two pushes. :-/

Comment 6

[Stephen A Pohl [:spohl]]

Attachment: Followup

This needed some additional work. The changes are:

1. Don't display a warning in the addons manager that the GMP will be installed shortly when it has been disabled.
2. Select "Never activate" when an addon is appDisabled, not only when it's userDisabled.
3. Don't show the GMP as active in the addons manager when it is appDisabled.
4. Set permissions of the GMP to 0 when it's appDisabled.

Comment 7

[Stephen A Pohl [:spohl]]

Attachment: Test changes

In addition to the test changes that became necessary by the patch in this bug, this also adds tests to ensure that we test the two scenarios when the GMP is "not installed and disabled", as well as "not installed but enabled".

Comment 8

[Stephen A Pohl [:spohl]]

Comment on attachment 8576836 [details] [diff] [review]
Test changes

Sorry, I'll need to update the wording in some of these tests before I submit for review again.

Comment 9

[Stephen A Pohl [:spohl]]

Attachment: Tests

Comment 10

[Stephen A Pohl [:spohl]]

Try is green AFAICT (none of the failures appear related to the changes here):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebec44afe8fc

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 8576832 [details] [diff] [review]
Followup

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

::: toolkit/mozapps/extensions/content/extensions.js
@@ +3117,5 @@
>          errorLink.value = gStrings.ext.GetStringFromName("details.notification.vulnerableNoUpdate.link");
>          errorLink.href = this._addon.blocklistURL;
>          errorLink.hidden = false;
> +      } else if (this._addon.isGMPlugin && !this._addon.isInstalled &&
> +                 !this._addon.appDisabled && !this._addon.userDisabled) {

Just use this._addon.isActive. Same for the other cases in this patch.

::: toolkit/mozapps/extensions/internal/GMPProvider.jsm
@@ +206,5 @@
>  
>    get version() { return GMPPrefs.get(KEY_PLUGIN_VERSION, null,
>                                        this._plugin.id); },
>  
> +  get isActive() { return !this.appDisabled && !this.userDisabled; },

Should this also test isInstalled maybe? Not totally sure.

Comment 12

[Stephen A Pohl [:spohl]]

Attachment: Followup

Thanks, Dave! Addressed feedback, carrying over r+.

(In reply to Dave Townsend [:mossop] from comment #11)
> ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm
> @@ +206,5 @@
> >  
> >    get version() { return GMPPrefs.get(KEY_PLUGIN_VERSION, null,
> >                                        this._plugin.id); },
> >  
> > +  get isActive() { return !this.appDisabled && !this.userDisabled; },
> 
> Should this also test isInstalled maybe? Not totally sure.

No, we always want a GMP to appear active in the addons manager when it's enabled (even when it's not installed yet). In the event that the GMP isn't installed yet, we display a message saying that the GMP will be installed shortly.

Comment 13

[Stephen A Pohl [:spohl]]

Submitted to try one last time to make sure I didn't miss anything:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f884e4d4ca2e

Comment 14

[Stephen A Pohl [:spohl]]

Try is green.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ce02e9d0771
https://hg.mozilla.org/integration/mozilla-inbound/rev/91ec1141b8ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/c91b83264bdc

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9ce02e9d0771
https://hg.mozilla.org/mozilla-central/rev/91ec1141b8ca
https://hg.mozilla.org/mozilla-central/rev/c91b83264bdc

Comment 16

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

We seem to have some tests here. Is any sort of manual QA needed explicitly for this? Note that EME is being actively monitored by QA, and there are other fixes being verified.

Comment 17

[Stephen A Pohl [:spohl]]

I've updated our automated tests to check for the changed behavior here, so I'm thinking that should be enough.

Comment 18

[Stephen A Pohl [:spohl]]

Attachment: Combined patch for aurora

Approval Request Comment
[Feature/regressing bug #]: Adobe EME
[User impact if declined]: EME plugins will not use appDisabled, userDisabled and permissions properly in the addons manager UI, which can result in misleading UI (options appear to be selectable when they aren't etc.).
[Describe test coverage new/current, TreeHerder]: Automated tests have been adjusted for this new behavior. 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 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/80bd1fddb5b9