1070036 - [Linux] Exceptions during openh264 provider initialization

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[:Irving Reid (No longer working on Firefox)]

The add-on manager reports exceptions during provider start up via Telemetry. Recent telemetry reports are showing a high frequency (29% of beta 33 sessions) of uncaught exceptions in the OpenH264 provider initialization:

{"message":"[Exception... \"Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [mozIGeckoMediaPluginService.addPluginDirectory]\" nsresult: \"0x80040111 (NS_ERROR_NOT_AVAILABLE)\" location: \"JS frame :: resource://gre/modules/addons/OpenH264Provider.jsm :: OpenH264Provider.startup :: line 266\" data: no]","context":"provider startup","module"

Aggregated data is at https://s3-us-west-2.amazonaws.com/telemetry-public-analysis/addon_perf/data/weekly_exceptions_20140915.csv.gz, quick&dirty spreadsheet at https://docs.google.com/spreadsheets/d/1xwCv8s8mxTIwYWkqBZVssVWAlVW3Clm51V8gLliE-v8/edit?usp=sharing

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

We should hop on figuring out why this is failing so often on Linux.

I don't see any non-Linux failures in that spreadsheet.

Comment 2

[Georg Fritzsche [:gfritzsche]]

This is here, so not critical now for the provider setup in release:
https://hg.mozilla.org/releases/mozilla-beta/file/f14c89b414b6/toolkit/mozapps/extensions/internal/OpenH264Provider.jsm#l271

Looking at addPluginDirectory, this seems the likely source:
https://hg.mozilla.org/releases/mozilla-beta/file/f14c89b414b6/content/media/gmp/GMPService.cpp#l392
Do we actually support sandboxing on Linux yet? Should OpenH264 work there?

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

We do support sandboxing on linux, and OpenH264 should work there.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Hm, and 29% percent sessions seems a little high for scenarios where MOZ_DISABLE_GMP_SANDBOX is set:
https://hg.mozilla.org/releases/mozilla-beta/file/f14c89b414b6/security/sandbox/linux/Sandbox.h#l26

Comment 5

[Georg Fritzsche [:gfritzsche]]

I'm hoping for some input from jld.
The next-best option i can think of would be to confirm the failure source in GeckoMediaPluginService::AddPluginDirectory() through Telemetry.

Comment 6

[:Irving Reid (No longer working on Firefox)]

Hmm. My exception telemetry hook (AddonManagerPrivate.recordException) is JS-only. You could add new NS_ERROR codes (or just choose different existing codes) to indicate exactly where in the call chain the error was found.

(/me shakes fist at error handling systems that can't report context information)

The JS caller still needs to handle the error, because there are legitimate reasons for addPluginDirectory to throw NS_ERROR_NOT_AVAILABLE.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

(In reply to :Irving Reid from comment #6)
> Hmm. My exception telemetry hook (AddonManagerPrivate.recordException) is
> JS-only. You could add new NS_ERROR codes (or just choose different existing
> codes) to indicate exactly where in the call chain the error was found.
> 
> (/me shakes fist at error handling systems that can't report context
> information)

/me joins

> The JS caller still needs to handle the error, because there are legitimate
> reasons for addPluginDirectory to throw NS_ERROR_NOT_AVAILABLE.


What would those be?  We're not writing to the directory/profile or creating the directory (and for that matter, we aren't even creating a sandboxed GMP process here either - that only happens when it gets used).  Not to discount handling errors!  but I want to know what you're thinking of.

Comment 8

[Georg Fritzsche [:gfritzsche]]

[Tracking Requested - why for this release]:

Comment 9

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> [Tracking Requested - why for this release]:

... This may affect Loop functionality for a significant chunk of the Linux population.

Comment 10

[Maire Reavy [:mreavy]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> > [Tracking Requested - why for this release]:
> 
> ... This may affect Loop functionality for a significant chunk of the Linux
> population.

It's unlikely to impact Loop functionality because Desktop-Desktop calls currently prefer VP8.  It would impact calls between the Loop mobile app on FxOS and Linux Desktop users who had this problem;  those calls would have to use VP8.  

This would also impact interop between Linux Desktop and any partner endpoints that only supported H.264.

Comment 11

[:Irving Reid (No longer working on Firefox)]

(In reply to Randell Jesup [:jesup] from comment #7)
> (In reply to :Irving Reid from comment #6)
...
> > The JS caller still needs to handle the error, because there are legitimate
> > reasons for addPluginDirectory to throw NS_ERROR_NOT_AVAILABLE.
> 
> 
> What would those be?  We're not writing to the directory/profile or creating
> the directory (and for that matter, we aren't even creating a sandboxed GMP
> process here either - that only happens when it gets used).  Not to discount
> handling errors!  but I want to know what you're thinking of.

If Sandbox.isSupported ends up false at http://dxr.mozilla.org/mozilla-central/source/security/sandbox/linux/Sandbox.cpp#74, calls to addPluginDirectory will throw. Maybe that condition should be handled some other way, but with the current design we should catch the exception.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

(In reply to :Irving Reid from comment #11)

> If Sandbox.isSupported ends up false at
> http://dxr.mozilla.org/mozilla-central/source/security/sandbox/linux/Sandbox.
> cpp#74, calls to addPluginDirectory will throw. Maybe that condition should
> be handled some other way, but with the current design we should catch the
> exception.

So that would be a linux system that didn't support the fundamentals needed for sandboxing ("Determine whether seccomp-bpf is supported by trying to enable it with an invalid pointer for the filter.")  Perhaps these are VMs?  or much older/odder linux systems?  Can we get a more detailed host system breakdown than 'linux'?

Comment 13

[:Irving Reid (No longer working on Firefox)]

I could pull a sample of telemetry packets showing this error; the useful information is likely to be in the System Information section (you can see what we get by looking at about:telemetry on your own browser)

'version' is the kernel version ID string, 'arch' the CPU architecture, 'adapter*' the video adapter (may allow you to identify VMs, e.g. VMware shows up in video adapter)

Comment 14

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

This is the result of bug 1043733; see also bug 1039819 and bug 1041885.

To summarize: I used the Firefox Health Report data on Linux kernel version (the string printed by `uname -r`) for profiles from the then-current release (Firefox 30) to estimate seccomp-bpf support; the fraction lacking it should have been an order of magnitude less than what's reported in comment #0.

Questions:
1. What is the cause of this discrepancy?
2. Can we resolve the discrepancy and arrive at a better estimate of the actual figure?
3. How many of the seccomp-lacking users are on EOL'ed versions of their distributions?

Sandboxing was never a hard requirement for OpenH264 as far as I know, so we could triple-land a patch to disable bug 1043733 if needed (release management willing)… but then we need to figure out what to do for EME.  Options are:

1. Ship a setuid-root executable, so we can use the full Chromium sandbox.  Bug 1041885 goes into some of the problems here.  Additionally: for users who lack seccomp-bpf support and haven't been getting regular security updates, this sandbox is probably escapeable via known kernel vulnerabilities.

2. Continue desupporting non-seccomp-capable users for EME, but let them run OpenH264 unsandboxed.

Comment 15

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to :Irving Reid from comment #13)
> I could pull a sample of telemetry packets showing this error; the useful
> information is likely to be in the System Information section (you can see
> what we get by looking at about:telemetry on your own browser)
> 
> 'version' is the kernel version ID string, 'arch' the CPU architecture,
> 'adapter*' the video adapter (may allow you to identify VMs, e.g. VMware
> shows up in video adapter)

The kernel version string should be helpful.

For the architecture, non-x86 systems don't enable MOZ_GMP_SANDBOX, so they shouldn't be showing this error, but it couldn't hurt to have it.  VMness isn't directly relevant (in terms of whether sandboxing works), but combined with the kernel version info it might tell us something.

Comment 16

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

ni? for telemetry data with kernel versions.

Comment 17

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

So this is... odd.  Here's a summary of the FHR data submitted since 2014-09-02 (Firefox 32 release).  Following the Metrics group's usual procedures, it's based on a random sample rather than the entire data set.  The columns are version, estimated percent with seccomp-bpf support, and total number of records.

21 85 511
22 96 866
23 85 1108
24 45 4462
25 96 1457
26 92 2805
27 84 1133
28 96 2844
29 96 2493
30 96 4233
31 93 10230
32 97 15078
33 74 31
34 100 21
35 100 21

Note in particular the line for Firefox 33.  I think it's worth rerunning this with the full FHR data, but at the moment it looks more likely that Beta is, for whatever reason, significantly unrepresentative.

Comment 18

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Merging in Health Reports submitted during the Firefox 31 cycle (and not superseded by a more recent report from the same profile), grouped by version relative to current (so 0 is release, 1 is beta, etc.):

-3 96 4919
-2 96 6259
-1 94 14802
0 96 25851
1 74 39
2 100 34
3 95 44

Again, beta stands out.  Some other demographic differences between beta and release:

Debian stable/oldstable: 17.9% of beta, 0.8% of release
           RHEL 6 and 5:  5.2% of beta, 0.2% of release
   Ubuntu, all versions: 58% of beta,  90% of release

(Disclaimer: these are guesses based on the version string.  /-generic(-|$)/ → Ubuntu, /el[56]/ → RHEL 5/6, /-(686(|-pae|-bigmem)|amd64)$/ → Debian; Debian and (3.2.0 or 2.6.32) → stable/oldstable.  Also, the seccomp-bpf approximation is: ≥ 3.5 or (Ubuntu and 3.2).)

The Debian and RedHat versions in question are known to lack seccomp-bpf support and are still supported, whereas Ubuntu backported seccomp-bpf for 12.04 which is its oldest supported desktop version.

So this would explain the disturbingly large number reported for beta telemetry in comment #0.

Comment 19

[:Irving Reid (No longer working on Firefox)]

Attachment: beta-33-versions

This is count vs. 'version' string for Beta 33 telemetry received on 2014-09-18 (I have a separate file for Nightly 35 on the same day)

I didn't count the total number of Linux reports, so I don't have a percentage, but this confirms that the exceptions are happening in older kernels.

Comment 20

[:Irving Reid (No longer working on Firefox)]

Attachment: nightly-35-versions

Count vs. kernel version for Nightly 35 telemetry received on 2014-09-18 that contained the NOT_AVAILABLE exception.

Comment 21

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to :Irving Reid from comment #19)
> I didn't count the total number of Linux reports, so I don't have a
> percentage, but this confirms that the exceptions are happening in older
> kernels.

And exactly the older kernels expected: < 3.5 in general, < 3.2 for Ubuntu "-generic", RHEL 6 but not 7, no Fedora ("fc") newer than 15 (actually lower than the expected 17, but I'm not complaining, and in any case 19 is the oldest currently supported).  One or two exceptions in the nightly data, which I'm guessing are local custom builds.

So, the answer to the questions in comment #14 is that the telemetry stats on beta aren't a reason to start panicking about release, and there's no reason to doubt the FHR data, but they might be a reason to start worrying about beta/aurora/nightly users.

Comment 22

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Should we be reporting this error differently, from a programming/telemetry perspective?  It looks like there wasn't a resolution to what was brought up in comment #6 and comment #7.

(There is a separate question of what we should do about the UX of this error condition.  That needs a separate bug.)

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Tracking since H.264 is important.
FYI, we have just released beta 6. So, a fix will need to land soon if we want to see that bug fixed in 33.

Comment 24

[:Irving Reid (No longer working on Firefox)]

(In reply to Jed Davis [:jld] from comment #22)
> Should we be reporting this error differently, from a programming/telemetry
> perspective?  It looks like there wasn't a resolution to what was brought up
> in comment #6 and comment #7.

It's showing up in telemetry because it's an unhandled exception during provider start-up (and thus *may* indicate bustage in the field). Catching the exception inside your startup() and cleaning up the provider initialization appropriately is all I'm looking for.

If you want to add telemetry probes to explicitly count how many clients have sandboxing available and/or disabled, I'd treat that as a separate bug.

Comment 25

[Georg Fritzsche [:gfritzsche]]

So, we should handle catching the exception in this bug and do a follow-up for proper reporting of this.

(In reply to Sylvestre Ledru [:sylvestre] from comment #23)
> Tracking since H.264 is important.
> FYI, we have just released beta 6. So, a fix will need to land soon if we
> want to see that bug fixed in 33.

Per comment 2 and comment 21 this is not critical.

Comment 26

[Georg Fritzsche [:gfritzsche]]

(In reply to :Irving Reid from comment #24)
> If you want to add telemetry probes to explicitly count how many clients
> have sandboxing available and/or disabled, I'd treat that as a separate bug.

I filed bug 1072363 on reporting an error so we can tell, although we could morph it to just to proper reporting too. Jed, is this something you can take?

Comment 27

[Georg Fritzsche [:gfritzsche]]

Attachment: Catch NS_ERROR_NOT_AVAILABLE in OpenH264Provider

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Georg, OK. Thanks. Not blocking 33 for this then.

Comment 29

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I attached a patch to bug 1072363 which removes OpenH264 from the plugin list in about:addons and avoids the addPluginDirectory (by disabling the OpenH264Provider) if GMP is disabled for sandboxing reasons… but, rereading the earlier comments, maybe that patch makes more sense on this bug?

The one thing I haven't done yet is note the error in a form that telemetry or FHR would pick up.  But if they collect add-on information, that might also work.

Comment 30

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8494579 [details] [diff] [review]
Catch NS_ERROR_NOT_AVAILABLE in OpenH264Provider

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

::: toolkit/mozapps/extensions/internal/OpenH264Provider.jsm
@@ +271,5 @@
> +      try {
> +        gmpService.addPluginDirectory(this.gmpPath);
> +      } catch (e if e.name == 'NS_ERROR_NOT_AVAILABLE') {
> +        this._log.warning("startup() - adding gmp directory failed with " + e.name + " - sandboxing not available?");
> +      }

This is fine. Note that Log.jsm knows how to format exceptions, so you could use:

this._log.warming("startup() - adding gmp directory failed, sandboxing may not be available", e);

though in this case the extra info in the formatted exception wouldn't be very helpful, so no need to change this patch.

Comment 31

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d71444b75291

Comment 32

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8494579 [details] [diff] [review]
Catch NS_ERROR_NOT_AVAILABLE in OpenH264Provider

Approval Request Comment
[Feature/regressing bug #]: OpenH264 integration
[User impact if declined]: Avoids spamming Addon exception telemetry with a known issue.
[Describe test coverage new/current, TBPL]: We have proper test-coverage for this path.
[Risks and why]: Low-risk, this just trivially catches one expected exception.
[String/UUID change made/needed]: None.

Comment 33

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8494579 [details] [diff] [review]
Catch NS_ERROR_NOT_AVAILABLE in OpenH264Provider

Approving even it is not in m-c to make sure it is in beta 8

Comment 34

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/155cfcc5efa9
https://hg.mozilla.org/releases/mozilla-beta/rev/b6985e15046b

Comment 35

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

https://hg.mozilla.org/mozilla-central/rev/d71444b75291