1306516 - Widevine installation failure

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Widevine is failing to install with the following error. I'm assuming it is a regression in caused by bug 1267495.

14:16:25.357 1475198185357	Toolkit.GMP	ERROR	GMPWrapper(gmp-widevinecdm) findUpdates() - updateTask for gmp-widevinecdm threw: TypeError: gmpAddons.find is not a function (resource://gre/modules/addons/GMPProvider.jsm:295:22) JS Stack trace: GMPWrapper.prototype.findUpdates/this._updateTask<@GMPProvider.jsm:295:22 1 Log.jsm:753
	App_append resource://gre/modules/Log.jsm:753:9
	Logger.prototype.log resource://gre/modules/Log.jsm:389:7
	LoggerRepository.prototype.getLoggerWithMessagePrefix/proxy.log resource://gre/modules/Log.jsm:504:44
	Logger.prototype.error resource://gre/modules/Log.jsm:397:5
	GMPWrapper.prototype.findUpdates/this._updateTask< resource://gre/modules/addons/GMPProvider.jsm:306:9
	next self-hosted
	TaskImpl_run resource://gre/modules/Task.jsm:322:42
	bound TaskImpl_run self-hosted
	Handler.prototype.process resource://gre/modules/Promise-backend.js:937:23
	this.PromiseWalker.walkerLoop resource://gre/modules/Promise-backend.js:816:7
	bound  self-hosted
	bound bound  self-hosted

Comment 1

[Dale Harvey (:daleharvey)]

Yup definitely caused by 1267495. I am confused though, does anyone more familiar with this code know why there are 3 different code paths to installing these addons? at the least the XPIProvider and browser.js path get called at some point on startup

http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#3086
http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/GMPProvider.jsm#294
http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#1334

Also Anthony could you say what you do to hit this? when I open up a fresh profile I got via the browser.js method successfully

Comment 2

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dale Harvey (:daleharvey) from comment #1)
> Yup definitely caused by 1267495. I am confused though, does anyone more
> familiar with this code know why there are 3 different code paths to
> installing these addons? at the least the XPIProvider and browser.js path
> get called at some point on startup
> 

Maybe rhelmer knows?

Comment 3

[Robert Helmer [:rhelmer]]

(In reply to Dale Harvey (:daleharvey) from comment #1)
> Yup definitely caused by 1267495. I am confused though, does anyone more
> familiar with this code know why there are 3 different code paths to
> installing these addons? at the least the XPIProvider and browser.js path
> get called at some point on startup
> 
> http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/
> internal/XPIProvider.jsm#3086

This is a system add-on check (these are based on the way GMP works but should not be directly related otherwise).

> http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/
> internal/GMPProvider.jsm#294

This is the periodic (timer-based) check for GMP.

> http://searchfox.org/mozilla-central/source/browser/base/content/browser.
> js#1334

This one I am less familiar with, but I assume this is so we don't wait for the once-every-24h-timer in case the user doesn't have any GMP addons installed, so it doesn't block startup but runs shortly after first-run so DRM videos work?

Comment 4

[Robert Helmer [:rhelmer]]

(In reply to Dale Harvey (:daleharvey) from comment #1)
> Yup definitely caused by 1267495. I am confused though, does anyone more
> familiar with this code know why there are 3 different code paths to
> installing these addons? at the least the XPIProvider and browser.js path
> get called at some point on startup
> 
> http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/
> internal/XPIProvider.jsm#3086
> http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/
> internal/GMPProvider.jsm#294
> http://searchfox.org/mozilla-central/source/browser/base/content/browser.
> js#1334
> 
> Also Anthony could you say what you do to hit this? when I open up a fresh
> profile I got via the browser.js method successfully

From the line of code this is failing on, I bet this is only happening via the periodic timer check not the on-first-run check, and that `installManager.checkForAddons()` is returning something that the next line does not expect:

https://dxr.mozilla.org/mozilla-central/rev/9baec74b3db1bf005c66ae2f50bafbdb02c3be38/toolkit/mozapps/extensions/internal/GMPProvider.jsm#294-295

The return type was changed in bug 1267495 so that would make sense, line 295 probably needs to use the .gmpAddons property:

https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/GMPInstallManager.jsm#95-98

You can force a timer check using https://addons.mozilla.org/en-US/firefox/addon/timer-fire (or by pasting the right incantation into the console but I test this sort of thing enough I just keep Mossop's addon installed :))

Comment 5

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #3)
> (In reply to Dale Harvey (:daleharvey) from comment #1)
> > Yup definitely caused by 1267495. I am confused though, does anyone more
> > familiar with this code know why there are 3 different code paths to
> > installing these addons? at the least the XPIProvider and browser.js path
> > get called at some point on startup
> > 
> > http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/
> > internal/XPIProvider.jsm#3086

Hm ok this is a bit confusing/misleading - both the system add-on and GMP check are initiated here. We should consider separating this for clarity.

In any case - I am having trouble reproducing this too. I wonder if it only happens when updates are actually available, and only by the periodic and not the startup check?

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I don't know it happened but the GMPs all became uninstalled but the others reinstalled themselves. The error I posted was as a result of playing a video on Netflix which should trigger an install.

Comment 7

[Dale Harvey (:daleharvey)]

Attachment: 1306516-1.patch

So if I comment out the timer checks @ http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/GMPProvider.jsm#264,287 then use the "Fire Timer" addon and trigger an addon update check then I can reproduce this.

The fix is fairly obvious but I want to be able to reproduce to verify

Comment 8

[Dale Harvey (:daleharvey)]

Comment on attachment 8797573 [details] [diff] [review]
1306516-1.patch

Clearing review and gonna see if I can form a failing test out of http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_gmpProvider.js before landing

Comment 9

[Dale Harvey (:daleharvey)]

Attachment: Fix expected return value from checkForAddons.

These tests actually break with the fixed code, so there are tests against conformance to the implementation but require the mock to be kept up to date.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8798413 [details] [diff] [review]
Fix expected return value from checkForAddons.

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

This looks fine to me, though I note that there's another case where we need to update another install manager mock (see comment below).

Before landing this, can you do a quick sweep through the code for all uses of checkForAddons to make sure they're all conformant to the new API? If so, with the test_gmpProvider.js test fixed, and the tests passing, r=me.

::: toolkit/mozapps/extensions/test/browser/browser_gmpProvider.js
@@ +36,5 @@
>  function MockGMPInstallManager() {
>  }
>  
>  MockGMPInstallManager.prototype = {
> +  checkForAddons: () => Promise.resolve({

Looks like there's another one of these here:

http://searchfox.org/mozilla-central/rev/76609a05d6ef7ba4223ed79e479c73fb2543a107/toolkit/mozapps/extensions/test/xpcshell/test_gmpProvider.js#45

Comment 12

[Dale Harvey (:daleharvey)]

Good catch, fixed that and yup according to https://dxr.mozilla.org/mozilla-central/search?q=checkForAddons&redirect=false thats all of them. Pushing to try before merging @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=70410faa8d1784a86f83a053a37e1d37d1029ef7

Comment 13

[Marcia Knous [:marcia]]

Tracking 52+ since it is important due to some users not being able to play Netflix.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/7ff5c0b99e19
Fix expected return value from checkForAddons. r=mconley

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7ff5c0b99e19

Comment 16

[Bogdan Maris, Desktop QA]

I noticed that Widevine does not automatically install on Windows 64-bit latest Nightly 52.0a1 build (I waited even more then two hours), and based on bug 1307997 this should have fixed this. 
Note that if I hit Check for Updates, Widevine will install successfully. I've tested on Windows 10, 7, 8.1 using latest Nightly. On Mac I did not encounter this. 
Should we reopen this issue or log a new one?

Comment 17

[Dale Harvey (:daleharvey)]

This has been filed as a new bug @ https://bugzilla.mozilla.org/show_bug.cgi?id=1309463, thanks for the extra information to debug

Comment 18

[Andrei Vaida [:avaida]]

This is verified fixed on 52 per Comment 16 and Comment 17.