1205178 - [EME] GMP installed while profile used in 32bit will be used if profile used in 64bit Firefox

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

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

If the user installs a GMP in a profile running in 32 bit Firefox, and then uses that profile in a 64 bit Firefox, they will end up loading a 32bit DLL in a 64 bit process, which won't work.

We need to prevent that.

Comment 1

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

Attachment: Patch: Remember arch when installing GMP

Remember the arch of the GMP when we install it, and check that the arch hasn't changed when we add the GMP to the GMPService.

This means users migrated from Win32 to Win64 builds will have their GMPs uninstalled, and then have to re-install them next time they use them.

Comment 2

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

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

Comment 3

[Stephen A Pohl [:spohl]]

Comment on attachment 8661647 [details] [diff] [review]
Patch: Remember arch when installing GMP

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

Looks good, but let's move the creation of the KEY_PLUGIN_ABI pref before the KEY_PLUGIN_VERSION pref.

::: toolkit/modules/GMPInstallManager.jsm
@@ +912,5 @@
> +
> +        // Remember our ABI, so that if the profile is migrated to another
> +        // platform or from 32 -> 64 bit, we notice and don't try to load the
> +        // unexecutable plugin library.
> +        GMPPrefs.set(GMPPrefs.KEY_PLUGIN_ABI, GMPUtils.ABI(), gmpAddon.id);

According to the comment at line 904, we should create this pref (and presumably the KEY_PLUGIN_TRIAL_CREATE pref) before KEY_PLUGIN_VERSION to avoid race conditions.

::: toolkit/mozapps/extensions/internal/GMPProvider.jsm
@@ +504,3 @@
>      // Installed -> Check if files are missing.
>      let status = this._arePluginFilesOnDisk();
> +    status.mismatchedABI = false;

nit: move this below |status.installed| and before |status.valid| to match the structure from above

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cbe7dae98c5

Comment 5

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

Will need uplift.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2cbe7dae98c5

Comment 7

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

Comment on attachment 8661647 [details] [diff] [review]
Patch: Remember arch when installing GMP

Requesting uplift for Firefox 42.

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: Users who switch from Win32 to Win64 Firefox and use the same profile will find their EME doesn't work when running in Win64.
[Describe test coverage new/current, TreeHerder]: This patch includes xpcshell tests.
[Risks and why]: Low, pretty simple patch, has tests.
[String/UUID change made/needed]: None.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8661647 [details] [diff] [review]
Patch: Remember arch when installing GMP

Improve the 64 bit support and EME, taking it.

Comment 9

[Carsten Book [:Tomcat]]

Hi, this cause conflicts on beta when uplifting

grafting 302106:2cbe7dae98c5 "Bug 1205178 - Make GMPProvider check that GMPs are the right arch before loading. r=spohl"
merging toolkit/modules/GMPInstallManager.jsm
merging toolkit/modules/GMPUtils.jsm
warning: conflicts during merge.
merging toolkit/modules/GMPUtils.jsm incomplete! (edit conflicts, then use 'hg resolve --mark')
merging toolkit/modules/tests/xpcshell/test_GMPInstallManager.js
merging toolkit/mozapps/extensions/internal/GMPProvider.jsm
merging toolkit/mozapps/extensions/test/xpcshell/test_gmpProvider.js

Chris could you take a look, thanks!

Comment 10

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

(In reply to Carsten Book [:Tomcat] from comment #9)
> Hi, this cause conflicts on beta when uplifting
> Chris could you take a look, thanks!

Certainly. :)

Beta try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=46d013d60ceb

Rebased patch:
https://hg.mozilla.org/try/raw-rev/770b21c26c2e

Comment 11

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

(In reply to Chris Pearce (:cpearce) from comment #10)
> (In reply to Carsten Book [:Tomcat] from comment #9)
> > Hi, this cause conflicts on beta when uplifting
> > Chris could you take a look, thanks!
> 
> Certainly. :)
> 
> Beta try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=46d013d60ceb
> 
> Rebased patch:
> https://hg.mozilla.org/try/raw-rev/770b21c26c2e

I made a mistake rebasing the above.

Fixed:

Beta try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab196b098d69

(Correctly) rebased patch:
https://hg.mozilla.org/try/raw-rev/b74b9b4d96e3

Comment 12

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

https://hg.mozilla.org/releases/mozilla-beta/rev/7a794d1f6b44