Closed Bug 1167197 Opened 9 years ago Closed 9 years ago

GMPProvider busted on Android

Categories

(Core :: Audio/Video, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox38 --- unaffected
firefox38.0.5 --- unaffected
firefox39 --- fixed
firefox40 + fixed
firefox41 + fixed

People

(Reporter: snorp, Assigned: snorp)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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: GMPWrapper.prototype._arePluginFilesOnDisk@GMPProvider.jsm:483:7 < GMPWrapper.prototype.validate@GMPProvider.jsm:493:12 < GMPProvider.startup@GMPProvider.jsm:520:14 < callProvider@AddonManager.jsm:208:12 < _startProvider@AddonManager.jsm:670:5 < AMI_startup@AddonManager.jsm:838:9 < AMP_startup@AddonManager.jsm:2508:5 < AMC_observe@addonManager.js:55:7" {file: "resource://gre/modules/Log.jsm" line: 749}]
This was caused by the patch in bug 1156566
Blocks: 1156566
Attached patch Fix GMPProvider on Android (obsolete) — Splinter Review
Attachment #8608825 - Flags: review?(cpearce)
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?
Attachment #8608825 - Flags: review?(cpearce) → review+
Attachment #8608825 - Attachment is obsolete: true
Attachment #8609392 - Flags: review+
(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.
https://hg.mozilla.org/mozilla-central/rev/9e140d3141a8
Assignee: nobody → snorp
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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.
Attachment #8609392 - Flags: approval-mozilla-beta?
Attachment #8609392 - Flags: approval-mozilla-aurora?
I must not forget to ensure this gets uplifted.
Flags: needinfo?(cpearce)
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.
Attachment #8609392 - Flags: approval-mozilla-beta?
Attachment #8609392 - Flags: approval-mozilla-beta+
Attachment #8609392 - Flags: approval-mozilla-aurora?
Attachment #8609392 - Flags: approval-mozilla-aurora+
Chris, want to file a bug for some future test-writing to cover this scenario?
(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.
Flags: needinfo?(cpearce)
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.
You need to log in before you can comment on or make changes to this bug.