Closed Bug 1306516 Opened 5 years ago Closed 5 years ago

Widevine installation failure

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 + verified

People

(Reporter: ajones, Assigned: daleharvey)

References

Details

Attachments

(1 file, 1 obsolete file)

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
Assignee: nobody → dale
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
Flags: needinfo?(ajones)
(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?
Flags: needinfo?(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?
Flags: needinfo?(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 :))
Flags: needinfo?(dale)
(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?
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.
Flags: needinfo?(ajones)
Priority: -- → P1
Attached patch 1306516-1.patch (obsolete) — Splinter Review
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
Flags: needinfo?(dale)
Attachment #8797573 - Flags: review?(mconley)
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
Attachment #8797573 - Flags: review?(mconley)
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.
Attachment #8797573 - Attachment is obsolete: true
Attachment #8798413 - Flags: review?(mconley)
Duplicate of this bug: 1307997
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
Attachment #8798413 - Flags: review?(mconley) → review+
Tracking 52+ since it is important due to some users not being able to play Netflix.
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/7ff5c0b99e19
Fix expected return value from checkForAddons. r=mconley
https://hg.mozilla.org/mozilla-central/rev/7ff5c0b99e19
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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?
Flags: needinfo?(dale)
This has been filed as a new bug @ https://bugzilla.mozilla.org/show_bug.cgi?id=1309463, thanks for the extra information to debug
Flags: needinfo?(dale)
This is verified fixed on 52 per Comment 16 and Comment 17.
You need to log in before you can comment on or make changes to this bug.