1167197 - GMPProvider busted on Android

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

Not sure when this started, but with a local build I see the following in logcat when trying to use H264 with WebRTC:

Exception calling provider GMPProvider.startup: TypeError: this._info is undefined (resource://gre/modules/addons/GMPProvider.jsm:483:6) JS Stack trace: [e-mail]:483:7 < [e-mail]:493:12 < [e-mail]:520:14 < [e-mail]:208:12 < [e-mail]:670:5 < [e-mail]:838:9 < [e-mail]:2508:5 < [e-mail]:55:7" {file: "resource://gre/modules/Log.jsm" line: 749}]

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

This was caused by the patch in bug 1156566

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix GMPProvider on Android

Comment 3

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

Comment on attachment 8608825 [details] [diff] [review]
Fix GMPProvider on Android

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

That's much nicer! :)

I uplifted the regressor, so we'll need to do the same if GMPs are supposed to work on Android on 39 or 40.

::: toolkit/mozapps/extensions/internal/GMPProvider.jsm
@@ -463,5 @@
>        f.initWithPath(path);
>        return f.exists();
>      };
>  
> -    // Determine the name of the GMP dynamic library; it differs on every

I have basically the same code in test_gmpProvider.js in createMockPluginFilesIfNeeded(), can we do the same thing there? Does that test run on Android?

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix GMPProvider on Android

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Chris Pearce (:cpearce) from comment #3)
> Comment on attachment 8608825 [details] [diff] [review]
> Fix GMPProvider on Android
> 
> Review of attachment 8608825 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That's much nicer! :)
> 
> I uplifted the regressor, so we'll need to do the same if GMPs are supposed
> to work on Android on 39 or 40.

OK.

> 
> ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm
> @@ -463,5 @@
> >        f.initWithPath(path);
> >        return f.exists();
> >      };
> >  
> > -    // Determine the name of the GMP dynamic library; it differs on every
> 
> I have basically the same code in test_gmpProvider.js in
> createMockPluginFilesIfNeeded(), can we do the same thing there? Does that
> test run on Android?

This apparently does not run on Android (not totally sure why), but I fixed the test up too anyway.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e140d3141a8

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9e140d3141a8

Comment 8

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

Comment on attachment 8609392 [details] [diff] [review]
Fix GMPProvider on Android

Approval Request Comment
[Feature/regressing bug #]: Bug 1156566l, EME
[User impact if declined]: Bug 1156566 introduced a regression on Linux; the OpenH264 plugin on some Linux systems will be installed correctly, but deleted when the browser restarts and then re-downloaded 60 seconds later. Without this path uplifted, that behavior will happen.
[Describe test coverage new/current, TreeHerder]: This has unit tests, though not to cover the browser restart scenario which is causing problems.
[Risks and why]: Pretty low, just changes how we determine the file extension for dynamic libraries.
[String/UUID change made/needed]: None.

Comment 9

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

I must not forget to ensure this gets uplifted.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8609392 [details] [diff] [review]
Fix GMPProvider on Android

Approved for uplift to beta and aurora, so that we don't make Linux users download OpenH264 repeatedly.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Chris, want to file a bug for some future test-writing to cover this scenario?

Comment 12

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

(In reply to Liz Henry (:lizzard) from comment #11)
> Chris, want to file a bug for some future test-writing to cover this
> scenario?

bug 1171300.

Comment 13

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/6338c30d8da3

Comment 14

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

https://hg.mozilla.org/releases/mozilla-beta/rev/e2ed03987d19

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

This was approved for uplift to Aurora already. Adding a tracking flag for FF40 and FF41 as this is a regression that is painful from an end user point of view.