1140263 - [EME] Enable Adobe EME in GMP downloader and addons manager on Windows Vista and above

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Stephen A Pohl [:spohl]]

[Tracking Requested - why for this release]:

This is necessary to turn Adobe EME on.

Comment 1

[Stephen A Pohl [:spohl]]

Attachment: Patch

I *think* this should take care of everything that we wanted, but I'd like for QA to confirm once we have a build.

Chris, does this seem like what we've discussed? Not sure if you can review this directly, or if we should ask someone else who's available to review promptly. I'm assuming we'd want to land and uplift this tomorrow Friday (US)?

Comment 2

[Stephen A Pohl [:spohl]]

Attachment: Patch

Forgot to stop observing the hidden pref for each GMP when we shut down the GMPProvider.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

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

That looks like the behaviour we wanted.

Comment 4

[Stephen A Pohl [:spohl]]

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

Blair, I believe you reviewed some of the initial work from Georg around GMP downloading. Georg is away for a while. Would you be able to review this? It's my understanding that we'd like to have this landed and uplifted in time for Monday's beta drop (US time). I apologize for the short notice, I didn't quite understand the urgency myself.

Comment 5

[:Gijs (he/him)]

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

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

r-, considering Unfocused is gone already, please request next review from :Mossop. :-)

::: modules/libpref/init/all.js
@@ +4551,2 @@
>  // Adobe EME is currently pref'd off by default and hidden in the addon manager.
>  pref("media.gmp-eme-adobe.hidden", true);

Pretty sure this should still define the pref on Windows (but with a default of false)

::: toolkit/modules/GMPInstallManager.jsm
@@ +291,5 @@
> +#ifdef XP_WIN
> +function IsWindowsVistaOrLater()
> +{
> +  let re = /Windows NT (\d+\.\d)/;
> +  let window = Services.wm.getMostRecentWindow("navigator:browser");

err, yeah, no. :-)

For one, not from toolkit you can't. Plus on OS X there might not even be a window. And for another, shudder at the regex parsing.


How about:

Services.sysinfo.getPropertyAsInt32("version") >= 6 ?

Comment 6

[Stephen A Pohl [:spohl]]

Attachment: Patch

(In reply to :Gijs Kruitbosch from comment #5)
> ::: modules/libpref/init/all.js
> @@ +4551,2 @@
> >  // Adobe EME is currently pref'd off by default and hidden in the addon manager.
> >  pref("media.gmp-eme-adobe.hidden", true);
> 
> Pretty sure this should still define the pref on Windows (but with a default
> of false)

This pref was added in bug 1089867 to hide Adobe EME until we were ready to turn it on. My understanding was that we'd ultimately remove it. GMPInstallManager.jsm[1] should do the right thing when the pref is missing.

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/GMPInstallManager.jsm#741

Comment 7

[Dave Townsend [:mossop]]

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

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

Setting the pref like this is setting us up for pain in the future if/when we want to enable this on XP, we won't know if the pref is set because the user set it or because the app set it. It's also being reset on every startup overriding any possible user choice. What I don't know is if that causes a pref change notification to trigger the rebuild you would need.

Even toggling the default at runtime has problems. I think we would be better off not setting any default for the hidden pref and instead having the code decide the default value based on OS info but we could probably do that later if we're going to be changing this stuff much.

Rebuilding the plugin list each time is sort of a problem. I've noted a few bugs with it below, it's also a bit wasteful. Might be out of time now but I think it would work better to instead always build the full plugin list and just make getAddonById etc. to not return hidden plugins. That would mean we didn't need the pref change notification at all.

::: toolkit/modules/GMPInstallManager.jsm
@@ +289,5 @@
>  });
>  
> +#ifdef XP_WIN
> +function IsWindowsVistaOrLater()
> +{

Nit: prevailing style has the brace on the previous line.

@@ +298,5 @@
>  /**
>   * Provides an easy API for downloading and installing GMP Addons
>   */
>  function GMPInstallManager() {
> +#ifdef XP_WIN

We're trying to avoid ifdefs in JS code now, just check Services.appinfo.OS.

@@ +300,5 @@
>   */
>  function GMPInstallManager() {
> +#ifdef XP_WIN
> +  if (!IsWindowsVistaOrLater()) {
> +    GMPPrefs.set(GMPPrefs.KEY_ADDON_HIDDEN, true, EME_ADOBE_ID);

This should set the pref in the default branch to preserve any user value here.

::: toolkit/mozapps/extensions/internal/GMPProvider.jsm
@@ +552,5 @@
>    buildPluginList: function() {
> +    GMP_PLUGINS.forEach(aPlugin => {
> +      Preferences.observe(GMPPrefs.getPrefKey(KEY_PLUGIN_HIDDEN, aPlugin.id),
> +                          this.onPrefHiddenChanged, this);
> +    }, this);

I'd rather put this in startup() for consistency with where we shut it down. Also we're unlikely to accidentally call startup() twice.

@@ +561,3 @@
>      let licenseInfo = pluginsBundle.GetStringFromName(GMP_LICENSE_INFO);
>  
>      let map = new Map();

At this point this._plugins may be full of existing plugins, you need to go through them and call shutdown on their wrappers.

@@ +561,4 @@
>      let licenseInfo = pluginsBundle.GetStringFromName(GMP_LICENSE_INFO);
>  
>      let map = new Map();
>      GMP_PLUGINS.forEach(aPlugin => {

It would be just dandy if you replaced this with |for (let aPlugin of GMP_PLUGINS)|

@@ +580,5 @@
>          plugin.wrapper = new GMPWrapper(plugin);
>          map.set(plugin.id, plugin);
>        }
>      }, this);
>      this._plugins = map;

Not sure why we don't just set things directly in this._plugins to start with?

@@ +584,5 @@
>      this._plugins = map;
>    },
> +
> +  onPrefHiddenChanged: function() {
> +    this.rebuildPluginList();

This isn't going to send the appropriate events to rebuild any existing UI or affect the various things that use those events, like telemetry? Do we care? If this is purely for testing we should probably add a comment to that effect and remove the code when no longer necessary.

it also never calls gmpService.addPluginDirectory for the newly visible plugins. Is that a problem?

Comment 8

[Stephen A Pohl [:spohl]]

Thanks, :mossop! I think this is a lot of thoughtful feedback that should be implemented.

Unfortunately, I'm running out of time to address all the feedback, test, request review again and address any followup. Unless someone else picks this up, I will have to do this on Monday.

Comment 9

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

No need to track for 37, which won't ship EME. I have added this to my 37 worry list. Tracking for 38+ as that is still a target for the EME release.

Comment 10

[Stephen A Pohl [:spohl]]

Attachment: Patch

This is a complete rewrite. Some of the things that happened here:
1. Eliminated the media.gmp-eme-adobe.hidden pref.
2. Started hiding EME GMPs if they're not supported and not forced visible.
3. Turned on Adobe EME on Windows Vista and above by default by declaring it "supported". It can be forced visible for testing purposes on other platforms.
4. Started respecting prefs of the following structure: media.{0}.forcevisible, where {0} is the GMP id. This allows for testing of yet-unsupported GMPs such as Adobe EME on OSX etc. Setting one of these prefs to true requires a restart of the browser to take effect, which should be acceptable since this is only used for testing. Toggling the old media.gmp-eme-adobe.hidden pref also required a restart.
5. Made KEY_* constants global in GMPInstallManager to match GMPProvider.
6. Started hiding EME GMPs when the global KEY_EME_ENABLED pref is turned off. This is one necessary part to solving bug 1142655 (step 4 in bug 1142655 comment 0) and it just made sense to include it here.

Comment 11

[Stephen A Pohl [:spohl]]

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

Comment 12

[Stephen A Pohl [:spohl]]

Adding Syd and Maja since this will affect testing (see comment 10). Feel free to ping me if you have any questions.

Comment 13

[Stephen A Pohl [:spohl]]

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

I'll cancel the review until I've updated the tests as well. I'd like to make sure that nothing slipped through the cracks.

Comment 14

[Stephen A Pohl [:spohl]]

Attachment: Patch

Minor consts name-consolidation between GMPProvider.jsm and GMPInstallManager.jsm.

Comment 15

[Stephen A Pohl [:spohl]]

Attachment: Tests

Test changes.

Comment 16

[Stephen A Pohl [:spohl]]

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

Accidentally obsoleted this patch while uploading the tests patch.

Comment 17

[Stephen A Pohl [:spohl]]

Try is green with these patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9aed96244d48

Comment 18

[Dave Townsend [:mossop]]

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

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

Thanks for all your work here, this is looking great. I'd like to see the isHidden checks moves somewhere shared before I r+ this.

::: toolkit/mozapps/extensions/internal/GMPProvider.jsm
@@ +555,5 @@
>        aCallback([]);
>        return;
>      }
>  
> +    let results = [];

You can actually keep the comprehension here I think:

let results = [for ([id, p] of this._plugins) if (!p.isHidden) p.wrapper];

(Note the ordering switch that ES7 has made to array comprehensions!)

@@ +613,5 @@
> +          return true;
> +        },
> +        get _isForcedVisible() {
> +          return GMPPrefs.get(KEY_PLUGIN_FORCEVISIBLE, false, this.id);
> +        },

These three methods are identical to those in GMPInstallManager. Can we share this code somewhere, maybe just expose an isPluginHidden function in GMPInstallManager or somewhere both the modules import. I'd like to keep maintenance of them as easy as possible.

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 8578614 [details] [diff] [review]
Tests

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

::: toolkit/mozapps/extensions/test/browser/browser_gmpProvider.js
@@ +45,5 @@
>  };
>  
> +function MockIsSupported() {
> +  return false;
> +}

This function is never used

Comment 20

[Stephen A Pohl [:spohl]]

Attachment: Patch

Thanks, Dave, for all the feedback!

While testing the patch after my updates I noticed that if we are flipping the KEY_EME_ENABLED pref, we don't update the addons manager UI (manually refreshing the UI works). If KEY_EME_ENABLED is true, we should see EME plugins displayed and they should be hidden when it's false. I guess I was relying on the fact that the "onPropertyChanged" notification (added to onPrefEMEGlobalEnabledChanged in bug 1140523) would trigger another call to getAddonsByTypes, but that doesn't seem to be the case. Dave, do you have an idea how I could solve this?

(In reply to Dave Townsend [:mossop] from comment #18)
> ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm
> @@ +555,5 @@
> >        aCallback([]);
> >        return;
> >      }
> >  
> > +    let results = [];
> 
> You can actually keep the comprehension here I think:
> 
> let results = [for ([id, p] of this._plugins) if (!p.isHidden) p.wrapper];
> 
> (Note the ordering switch that ES7 has made to array comprehensions!)

I've tried the following:
let results = [for ([id, p] of this._plugins) if (!installManager.isPluginHidden(p)) p.wrapper];

However, I'm getting the following error in the console:
addons.manager	ERROR	Exception loading provider GMPProvider from category "resource://gre/modules/addons/GMPProvider.jsm": SyntaxError: missing variable name (resource://gre/modules/addons/GMPProvider.jsm:561:24) JS Stack trace: [e-mail]:817:11 < [e-mail]:2468:5 < [e-mail]:55:7

I'm not sure why it's complaining about a missing variable name...

> @@ +613,5 @@
> > +          return true;
> > +        },
> > +        get _isForcedVisible() {
> > +          return GMPPrefs.get(KEY_PLUGIN_FORCEVISIBLE, false, this.id);
> > +        },
> 
> These three methods are identical to those in GMPInstallManager. Can we
> share this code somewhere, maybe just expose an isPluginHidden function in
> GMPInstallManager or somewhere both the modules import. I'd like to keep
> maintenance of them as easy as possible.

Moved to GMPInstallManager. Currently, I have to instantiate a GMPInstallManager object to call the isPluginHidden method on it. Could you let me know if this is what you had in mind or if there's a different way to do this?

Comment 21

[Stephen A Pohl [:spohl]]

Attachment: Tests

Addressed feedback and updated for new patch. Carrying over r+.

Comment 22

[Dave Townsend [:mossop]]

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

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

(In reply to Stephen Pohl [:spohl] from comment #20)
> Created attachment 8578874 [details] [diff] [review]
> Patch
> 
> Thanks, Dave, for all the feedback!
> 
> While testing the patch after my updates I noticed that if we are flipping
> the KEY_EME_ENABLED pref, we don't update the addons manager UI (manually
> refreshing the UI works). If KEY_EME_ENABLED is true, we should see EME
> plugins displayed and they should be hidden when it's false. I guess I was
> relying on the fact that the "onPropertyChanged" notification (added to
> onPrefEMEGlobalEnabledChanged in bug 1140523) would trigger another call to
> getAddonsByTypes, but that doesn't seem to be the case. Dave, do you have an
> idea how I could solve this?

Right this is because now we're actually changing the plugin from something entirely hidden to something visible. That is pretty straightforward, send an onExternalInstall InstallListener event (https://developer.mozilla.org/en-US/Add-ons/Add-on_Manager/InstallListener#onExternalInstall%28%29).

Hiding an existing plugin is slightly more complex, you'll have to send onUninstalling and onUninstalled events (https://developer.mozilla.org/en-US/Add-ons/Add-on_Manager/AddonListener#onUninstalling%28%29).

> (In reply to Dave Townsend [:mossop] from comment #18)
> > ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm
> > @@ +555,5 @@
> > >        aCallback([]);
> > >        return;
> > >      }
> > >  
> > > +    let results = [];
> > 
> > You can actually keep the comprehension here I think:
> > 
> > let results = [for ([id, p] of this._plugins) if (!p.isHidden) p.wrapper];
> > 
> > (Note the ordering switch that ES7 has made to array comprehensions!)
> 
> I've tried the following:
> let results = [for ([id, p] of this._plugins) if
> (!installManager.isPluginHidden(p)) p.wrapper];
>
> However, I'm getting the following error in the console:
> addons.manager	ERROR	Exception loading provider GMPProvider from category
> "resource://gre/modules/addons/GMPProvider.jsm": SyntaxError: missing
> variable name (resource://gre/modules/addons/GMPProvider.jsm:561:24) JS
> Stack trace: [e-mail]:817:11 <
> [e-mail]:2468:5 < [e-mail]:55:7
> 
> I'm not sure why it's complaining about a missing variable name...

Sorry my bad, looks like the new ES7 syntax doesn't support destructuring like that yet, the old ES6 syntax seems to work though?

[p.wrapper for ([id, p] of this._plugins) if (!installManager.isPluginHidden(p))]

Or the for loop is just fine if slightly less elegant.

> > @@ +613,5 @@
> > > +          return true;
> > > +        },
> > > +        get _isForcedVisible() {
> > > +          return GMPPrefs.get(KEY_PLUGIN_FORCEVISIBLE, false, this.id);
> > > +        },
> > 
> > These three methods are identical to those in GMPInstallManager. Can we
> > share this code somewhere, maybe just expose an isPluginHidden function in
> > GMPInstallManager or somewhere both the modules import. I'd like to keep
> > maintenance of them as easy as possible.
> 
> Moved to GMPInstallManager. Currently, I have to instantiate a
> GMPInstallManager object to call the isPluginHidden method on it. Could you
> let me know if this is what you had in mind or if there's a different way to
> do this?

I was just thinking of a top-level method in GMPInstallManager, no need to instantiate the manager object then.

Comment 23

[Stephen A Pohl [:spohl]]

Attachment: Patch

Having a common isPluginHidden method in GMPInstallManager.jsm wasn't straightforward because we were using a mock GMPInstallManager object in our tests, which resulted in various failures. I went ahead and factored this out into a GMPUtils.jsm file, which IMO we should have had anyway. I also moved GMPPrefs into this file since we basically had duplicate code in GMPProvider.jsm and GMPInstallManager.jsm.

(In reply to Dave Townsend [:mossop] from comment #22)
> (In reply to Stephen Pohl [:spohl] from comment #20)
> > Created attachment 8578874 [details] [diff] [review]
> > Patch
> > 
> > Thanks, Dave, for all the feedback!
> > 
> > While testing the patch after my updates I noticed that if we are flipping
> > the KEY_EME_ENABLED pref, we don't update the addons manager UI (manually
> > refreshing the UI works). If KEY_EME_ENABLED is true, we should see EME
> > plugins displayed and they should be hidden when it's false. I guess I was
> > relying on the fact that the "onPropertyChanged" notification (added to
> > onPrefEMEGlobalEnabledChanged in bug 1140523) would trigger another call to
> > getAddonsByTypes, but that doesn't seem to be the case. Dave, do you have an
> > idea how I could solve this?
> 
> Right this is because now we're actually changing the plugin from something
> entirely hidden to something visible. That is pretty straightforward, send
> an onExternalInstall InstallListener event
> (https://developer.mozilla.org/en-US/Add-ons/Add-on_Manager/
> InstallListener#onExternalInstall%28%29).
> 
> Hiding an existing plugin is slightly more complex, you'll have to send
> onUninstalling and onUninstalled events
> (https://developer.mozilla.org/en-US/Add-ons/Add-on_Manager/
> AddonListener#onUninstalling%28%29).

I noticed that we were already doing virtually the same thing in onPrefVersionChanged (sorry for not noticing earlier). I've tried to mirror what we do there, with the difference that we don't need to deal with uninstall just yet (i.e. this._gmpPath can remain set, as it's still valid). Uninstalling GMPs will be handled in future bugs.

Comment 24

[Stephen A Pohl [:spohl]]

Attachment: Tests

Updated for new patch. Carrying over r+.

Comment 25

[Stephen A Pohl [:spohl]]

Sent to try, although hg.m.o is having responsiveness issues at the moment:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=67186994d462

Comment 26

[Dave Townsend [:mossop]]

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

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

Looks good, thanks.

::: toolkit/mozapps/extensions/internal/GMPProvider.jsm
@@ +341,5 @@
> +      }
> +      AddonManagerPrivate.callAddonListeners("onUninstalled", this);
> +    } else {
> +      AddonManagerPrivate.callInstallListeners("onExternalInstall", null, this,
> +                                               null, false);

Looking at other examples looks like we also need an onInstalling AddonListener event after this.

Comment 27

[Stephen A Pohl [:spohl]]

Attachment: Patch

Thanks, Dave! I've gone ahead and also added the "onInstalling" notification to onPrefVersionChanged as it was missing there too. I've also changed the declaration of GMPPrefs in GMPUtils.jsm from |let GMPPrefs| to |this.GMPPrefs| as discussed via IRC. Carrying over r+.

Comment 28

[Stephen A Pohl [:spohl]]

Inbound and fx-team are currently set to "approval required", so setting this to checkin-needed in case someone else can land this before me.

Comment 29

[Stephen A Pohl [:spohl]]

We'll need the green light from the EME team before we can land this, so removing checkin-needed keyword until we hear back.

Comment 30

[Chris Pearce [:cpearce (Not reading bugmail)]]

We should land everything except the pref change to enable the download of Adobe's CDM.

Comment 31

[Stephen A Pohl [:spohl]]

Attachment: Disable Adobe EME on all platforms for now

Comment 32

[Stephen A Pohl [:spohl]]

Attachment: Disable Adobe EME on all platforms for now

...and now with the necessary test changes.

Comment 33

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8580456 [details] [diff] [review]
Disable Adobe EME on all platforms for now

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

Does setting edia.gmp-eme-adobe.forcevisible=true still allow the CDM to be downloaded on Windows? We don't want to lose the ability to download the CDM.

Comment 34

[Stephen A Pohl [:spohl]]

Yes, absolutely. media.gmp-eme-adobe.forcevisible will work as if we had declared the platform as supported, i.e. it will download Adobe EME by default, respond correctly to media.eme.enabled=false (by disabling downloads and hiding the CDM in the addons manager) etc. You can refer to GMPPrefs.isPluginHidden in GMPUtils.jsm in attachment 8580274 [details] [diff] [review] if you'd like to verify our behavior for yourself.

Comment 35

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8580456 [details] [diff] [review]
Disable Adobe EME on all platforms for now

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

OK then!

Comment 36

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/fx-team/rev/5ed058e1876b
https://hg.mozilla.org/integration/fx-team/rev/093ee924ee0e
https://hg.mozilla.org/integration/fx-team/rev/0e6aa605af74

Comment 37

[Stephen A Pohl [:spohl]]

Setting this to leave-open until we've turned this back on for Windows Vista and above (by backing out the third cset[1]).

[1] https://hg.mozilla.org/integration/fx-team/rev/0e6aa605af74

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5ed058e1876b
https://hg.mozilla.org/mozilla-central/rev/093ee924ee0e
https://hg.mozilla.org/mozilla-central/rev/0e6aa605af74

Comment 39

[Stephen A Pohl [:spohl]]

Anthony confirms that we're ready to turn this on for Windows Vista and above. Backed out cset 0e6aa605af74 to turn this on:
https://hg.mozilla.org/integration/fx-team/rev/a9f983fdb8a4

Comment 40

[Stephen A Pohl [:spohl]]

Attachment: Combined patch for aurora

Combined patch for aurora. Carrying over r+.

Comment 41

[Stephen A Pohl [:spohl]]

Comment on attachment 8581633 [details] [diff] [review]
Combined patch for aurora

Approval Request Comment
[Feature/regressing bug #]: Adobe EME
[User impact if declined]: EME CDMs cannot be downloaded and will not be displayed in the addons manager.
[Describe test coverage new/current, TreeHerder]: We have mochitest and xpcshell tests.
[Risks and why]: Minimal, due to large internal testing.
[String/UUID change made/needed]: none

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4f3336a230e7

Comment 43

[Stephen A Pohl [:spohl]]

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