Closed Bug 1072363 Opened 6 years ago Closed 6 years ago

Properly report when GMP initialization fails due to sandboxing not available

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox32 --- unaffected
firefox33 --- wontfix
firefox34 --- affected
firefox35 --- affected

People

(Reporter: gfritzsche, Assigned: jld)

References

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
At a minimum, I think we should exclude OpenH264 (and other GMP plugins) from about:addons.

Adding a UI string is probably too much for 33 (and then there's the question of how to explain this in a way that will be useful for users, without bothering those who aren't using the affected features), but an untranslated log message would be an improvement over the current situation.  The current patch for bug 1070036 looks like it would take care of that.
See Also: → 1043733
This patch disables the OpenH264 provider if GMP is unavailable due to not having sandboxing support.  (There's probably a better way to log a warning message than this, but the Log.repository.getLogger interface used elsewhere in this file didn't seem to work at this point in startup.)

This also mostly supersedes the patch in bug 1070036, because if the addon isn't registered then it can never try to call addPluginDirectory.
Assignee: nobody → jld
Attachment #8494920 - Flags: review?(georg.fritzsche)
Comment on attachment 8494920 [details] [diff] [review]
bug1072363-conditional-openh264-hg0.diff

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

We should also add a Telemetry probe (flag) for reporting .isSupported.
Is there a suitable general place in the GMPService for that?

::: toolkit/mozapps/extensions/internal/OpenH264Provider.jsm
@@ +363,5 @@
> +                                      AddonManager.TYPE_SUPPORTS_ASK_TO_ACTIVATE)
> +  ]);
> +} else {
> +  dump("Toolkit::OpenH264Provider: Not loading; Gecko Media Plugins not supported on this system.  "
> +       + "(Sandboxing not available?)\n");

Also Cu.reportError()?
Attachment #8494920 - Flags: review?(georg.fritzsche) → review+
Comment on attachment 8494920 [details] [diff] [review]
bug1072363-conditional-openh264-hg0.diff

Ok, we should also not trigger the GMP download & installation when we don't support GMP:
http://hg.mozilla.org/mozilla-central/annotate/5e704397529b/browser/base/content/browser.js#l1223
Attachment #8494920 - Flags: review+
Fixed the logging; the message now appears on stderr and in the Browser Console.

:jesup agrees that this is low-risk, so I'm going to see if I can get it uplifted to 34 and 33.

A separate patch will follow to similarly conditionalize the GMP downloader; I don't know if the usefulness/risk ratio for that is high enough for uplift.
Attachment #8494920 - Attachment is obsolete: true
Attachment #8495634 - Flags: review?(rjesup)
Attachment #8495634 - Flags: review?(georg.fritzsche)
Comment on attachment 8495634 [details] [diff] [review]
bug1072363-conditional-openh264-hg1.diff

The r? was… excessively eager.  This breaks `make package` somehow:

Assertion failure: XRE_GetProcessType() != GeckoProcessType_Default || mShuttingDown, at /home/jld/src/gecko-dev/content/media/gmp/GMPService.cpp:284
Attachment #8495634 - Flags: review?(rjesup)
Attachment #8495634 - Flags: review?(georg.fritzsche)
It's broken on m-c, too: start a debug xpcshell and evaluate Cc["@mozilla.org/gecko-media-plugin-service;1"].getService(Ci.mozIGeckoMediaPluginService; it will crash on exit.

I'm assuming this is because the observer service gets an NS_XPCOM_SHUTDOWN_THREADS_OBSERVER_ID but never a profile-change-teardown.

So… either I can fix that (and depend on uplifting that as well), somehow, or find another way to expose this single bit of information to script.
Hm, do we really need this on 33/34? We could still just uplift of the trivial exception handling patch and live with it until 35.
This doesn't break anything and most users won't even see the entry in about:addons.
34 would be nice and might still be doable, but at this point I agree it's not worth fighting for on 33.
Here's a slightly different approach which avoids bug 1073764.  It no longer logs to stderr (because at this point in the code I can just use the existing logger object), but the error message does appear in the Browser Console.

I still don't want to try to get this into 33; there's just too much going on here that I don't understand as well as I'd like.
Attachment #8495634 - Attachment is obsolete: true
Attachment #8496379 - Flags: review?(georg.fritzsche)
And this is the patch that avoids the unnecessary plugin download.

Try run with these two patches: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9143f32beb05
Attachment #8496380 - Flags: review?(georg.fritzsche)
Comment on attachment 8496379 [details] [diff] [review]
bug1072363-conditional-openh264-hg2.diff

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

::: toolkit/mozapps/extensions/internal/OpenH264Provider.jsm
@@ +268,5 @@
>  
> +    if (!gmpService.isSupported) {
> +      this._log.error("startup() - Not starting; Gecko Media Plugins not supported on"
> +                      + " this system.  (Sandboxing not available?)");
> +      AddonManagerPrivate.unregisterProvider(this);

Hm, sorry to bounce this with a question, but this still leaves us with a race between shutdown() and the pref-observers being called here (which trigger addPluginDirectory()).

If we target 35 and maybe 34, can we fix the xpcshell issue?
Attachment #8496379 - Flags: review?(georg.fritzsche)
Attachment #8496380 - Flags: review?(georg.fritzsche) → review+
(In reply to Georg Fritzsche [:gfritzsche] from comment #12)
> ::: toolkit/mozapps/extensions/internal/OpenH264Provider.jsm
> @@ +268,5 @@
> >  
> > +    if (!gmpService.isSupported) {
> > +      this._log.error("startup() - Not starting; Gecko Media Plugins not supported on"
> > +                      + " this system.  (Sandboxing not available?)");
> > +      AddonManagerPrivate.unregisterProvider(this);
> 
> Hm, sorry to bounce this with a question, but this still leaves us with a
> race between shutdown() and the pref-observers being called here (which
> trigger addPluginDirectory()).

Something I just thought of: would it be possible to call unregisterProvider(this) in the exception handler that bug 1070036 added?  If that coulc work then we wouldn't even need the XPIDL change.
(In reply to Jed Davis [:jld] from comment #13)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #12)
> > ::: toolkit/mozapps/extensions/internal/OpenH264Provider.jsm
> > @@ +268,5 @@
> > >  
> > > +    if (!gmpService.isSupported) {
> > > +      this._log.error("startup() - Not starting; Gecko Media Plugins not supported on"
> > > +                      + " this system.  (Sandboxing not available?)");
> > > +      AddonManagerPrivate.unregisterProvider(this);
> > 
> > Hm, sorry to bounce this with a question, but this still leaves us with a
> > race between shutdown() and the pref-observers being called here (which
> > trigger addPluginDirectory()).
> 
> Something I just thought of: would it be possible to call
> unregisterProvider(this) in the exception handler that bug 1070036 added? 
> If that coulc work then we wouldn't even need the XPIDL change.

That still doesn't allow us to tell why we got that exception, at the moment we can't tell the reason.

Is the xpcshell issue hard to fix? If not then never registering the provider seems like the better option.
With bug 1074561 targetting 33 this is not required for OpenH264 anymore.
Given that EME and other GMP requirements for the addon manager are apparently not known yet, i guess we can close this?
Yes, this is now somewhere between irrelevant and WONTFIX, given bug 1074561.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.