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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: spohl, Assigned: spohl)

Tracking

(Blocks 1 bug, {qawanted})

unspecified
x86
Windows Vista
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox36 unaffected, firefox37- disabled, firefox38+ fixed, firefox39+ fixed)

Details

Attachments

(3 attachments, 11 obsolete attachments)

32.44 KB, patch
spohl
: review+
Details | Diff | Splinter Review
39.90 KB, patch
spohl
: review+
Details | Diff | Splinter Review
72.09 KB, patch
spohl
: review+
Details | Diff | Splinter Review
[Tracking Requested - why for this release]:

This is necessary to turn Adobe EME on.
Posted patch Patch (obsolete) — Splinter Review
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)?
Attachment #8573712 - Flags: feedback?(cpearce)
Posted patch Patch (obsolete) — Splinter Review
Forgot to stop observing the hidden pref for each GMP when we shut down the GMPProvider.
Attachment #8573712 - Attachment is obsolete: true
Attachment #8573712 - Flags: feedback?(cpearce)
Attachment #8573716 - Flags: feedback?(cpearce)
Comment on attachment 8573716 [details] [diff] [review]
Patch

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

That looks like the behaviour we wanted.
Attachment #8573716 - Flags: feedback?(cpearce) → feedback+
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.
Attachment #8573716 - Flags: review?(bmcbride)
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 ?
Attachment #8573716 - Flags: review?(bmcbride) → review-
Posted patch Patch (obsolete) — Splinter Review
(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
Attachment #8573716 - Attachment is obsolete: true
Attachment #8574018 - Flags: review?(dtownsend)
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?
Attachment #8574018 - Flags: review?(dtownsend) → review-
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.
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.
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #8574018 - Attachment is obsolete: true
Attachment #8577839 - Flags: review?(dtownsend)
Adding Syd and Maja since this will affect testing (see comment 10). Feel free to ping me if you have any questions.
Summary: [EME] Set media.gmp-eme-adobe.hidden to false for Windows Vista and above → [EME] Enable Adobe EME in GMP downloader and addons manager on Windows Vista and above
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.
Attachment #8577839 - Flags: review?(dtownsend)
Posted patch Patch (obsolete) — Splinter Review
Minor consts name-consolidation between GMPProvider.jsm and GMPInstallManager.jsm.
Attachment #8577839 - Attachment is obsolete: true
Attachment #8578613 - Flags: review?(dtownsend)
Posted patch Tests (obsolete) — Splinter Review
Test changes.
Attachment #8578613 - Attachment is obsolete: true
Attachment #8578613 - Flags: review?(dtownsend)
Attachment #8578614 - Flags: review?(dtownsend)
Comment on attachment 8578613 [details] [diff] [review]
Patch

Accidentally obsoleted this patch while uploading the tests patch.
Attachment #8578613 - Attachment is obsolete: false
Attachment #8578613 - Flags: review?(dtownsend)
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.
Attachment #8578613 - Flags: review?(dtownsend)
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
Attachment #8578614 - Flags: review?(dtownsend) → review+
Posted patch Patch (obsolete) — Splinter Review
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: AMI_startup@AddonManager.jsm:817:11 < AMP_startup@AddonManager.jsm:2468:5 < AMC_observe@addonManager.js: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?
Attachment #8578613 - Attachment is obsolete: true
Attachment #8578874 - Flags: feedback?(dtownsend)
Posted patch Tests (obsolete) — Splinter Review
Addressed feedback and updated for new patch. Carrying over r+.
Attachment #8578614 - Attachment is obsolete: true
Attachment #8578877 - Flags: review+
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: AMI_startup@AddonManager.jsm:817:11 <
> AMP_startup@AddonManager.jsm:2468:5 < AMC_observe@addonManager.js: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.
Attachment #8578874 - Flags: feedback?(dtownsend)
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #8578874 - Attachment is obsolete: true
Attachment #8579581 - Flags: review?(dtownsend)
Posted patch TestsSplinter Review
Updated for new patch. Carrying over r+.
Attachment #8579581 - Attachment is obsolete: true
Attachment #8579581 - Flags: review?(dtownsend)
Attachment #8579583 - Flags: review+
Attachment #8579581 - Attachment is obsolete: false
Attachment #8579581 - Flags: review?(dtownsend)
Attachment #8578877 - Attachment is obsolete: true
Sent to try, although hg.m.o is having responsiveness issues at the moment:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=67186994d462
Blocks: 1144903
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.
Attachment #8579581 - Flags: review?(dtownsend) → review+
Posted patch PatchSplinter Review
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+.
Attachment #8579581 - Attachment is obsolete: true
Attachment #8580274 - Flags: review+
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.
Keywords: checkin-needed
We'll need the green light from the EME team before we can land this, so removing checkin-needed keyword until we hear back.
Keywords: checkin-needed
We should land everything except the pref change to enable the download of Adobe's CDM.
...and now with the necessary test changes.
Attachment #8580454 - Attachment is obsolete: true
Attachment #8580454 - Flags: review?(cpearce)
Attachment #8580456 - Flags: review?(cpearce)
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.
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 on attachment 8580456 [details] [diff] [review]
Disable Adobe EME on all platforms for now

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

OK then!
Attachment #8580456 - Flags: review?(cpearce) → review+
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
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
Keywords: leave-open
Attachment #8580456 - Attachment is obsolete: true
Combined patch for aurora. Carrying over r+.
Attachment #8581633 - Flags: review+
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
Attachment #8581633 - Flags: approval-mozilla-aurora?
Attachment #8581633 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/a9f983fdb8a4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.