Closed Bug 1150765 Opened 5 years ago Closed 5 years ago

Content process sandboxing prevents hardware rendering for OpenGL on Mac with E10S

Categories

(Core :: Security: Process Sandboxing, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
e10s - ---
firefox40 --- fixed

People

(Reporter: jgilbert, Assigned: areinald.bug)

References

Details

Attachments

(3 files, 3 obsolete files)

Our content process sandboxing prevents us from getting a hardware-backed OpenGL context. OSX has a software renderer, so we are accidentally using that on e10s right now.

This means Mac's WebGL on E10S is not rendered by the GPU. (same for other GL users, such as SkiaGL, Flash, etc.)

With sandboxing disabled via setting the pref "security.sandbox.content.level" to 0, we get hardware rendering back.


Gecko will spew the GL renderer info for each created context when run with the envvar MOZ_GL_SPEW=1.

With e10s:
OpenGL version detected: 210
OpenGL vendor: Apple Inc.
OpenGL renderer: Apple Software Renderer
OpenGL vendor ('Apple Inc.') unrecognized

Without e10s:
OpenGL version detected: 210
OpenGL vendor: ATI Technologies Inc.
OpenGL renderer: AMD Radeon HD 6630M OpenGL Engine
OpenGL vendor ('ATI Technologies Inc.') recognized as: ATI
See Also: → 1150767
(In reply to Jeff Gilbert [:jgilbert] from comment #0)
> Our content process sandboxing prevents us from getting a hardware-backed
> OpenGL context. OSX has a software renderer, so we are accidentally using
> that on e10s right now.

Jeff, what you tell sounds familiar to me. Please provide the relevant portion of /var/log/system.log when trying to create the OpenGL context in e10s mode and with sandbox turned on (security.sandbox.content.level to 1).
(In reply to André Reinald from comment #1)
> (In reply to Jeff Gilbert [:jgilbert] from comment #0)
> > Our content process sandboxing prevents us from getting a hardware-backed
> > OpenGL context. OSX has a software renderer, so we are accidentally using
> > that on e10s right now.
> 
> Jeff, what you tell sounds familiar to me. Please provide the relevant
> portion of /var/log/system.log when trying to create the OpenGL context in
> e10s mode and with sandbox turned on (security.sandbox.content.level to 1).

I think this is the relevant line?

Apr  3 13:01:09 jgilberts-Mac-mini.local sandboxd[257] ([82440]): plugin-container(82440) deny iokit-open AMDRadeonX3000_AMDAccelDevice

I'll attach the full log excerpt.
Attached file system.log snippet
Attached patch bugzilla-1150765-1.patch (obsolete) — Splinter Review
Jeff, can you please confirm the above patch solves your issue?
Assignee: nobody → areinald
Status: NEW → ASSIGNED
Flags: needinfo?(jgilbert)
(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> I think this is the relevant line?
> 
> Apr  3 13:01:09 jgilberts-Mac-mini.local sandboxd[257] ([82440]):
> plugin-container(82440) deny iokit-open AMDRadeonX3000_AMDAccelDevice

I also think so. I could not reproduce with the patch applied.
(In reply to André Reinald from comment #4)
> Created attachment 8588367 [details] [diff] [review]
> bugzilla-1150765-1.patch
> 
> Jeff, can you please confirm the above patch solves your issue?

It does not. With the patch, I get:

> Apr  6 13:59:10 jgilberts-Mac-mini kernel[0]: Sandbox: plugin-container(84957) deny iokit-open AMDRadeonX3000_AMDAccelSharedUserClient

I poked around the internet fruitlessly trying to find docs on sandboxing rules, but I found this:
http://opensource.apple.com/source/WebKit2/WebKit2-7537.71/Resources/PlugInSandboxProfiles/com.apple.WebKit.plugin-common.sb?txt

By process of elimination, only one line seems necessary:

>            (iokit-connection "IOAccelerator")
Flags: needinfo?(jgilbert)
This works for me.
Attachment #8588783 - Flags: review?(areinald)
(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> By process of elimination, only one line seems necessary:
> >            (iokit-connection "IOAccelerator")

Jeff, unfortunately, if you skimmed through bug 1083344, you'll realize that for lack of documentation from Apple, getting correct rules has been a purely empirical process.

There were 2 approaches:
1. either add "allow" rules until to suppress all "denied" in the log (most of the ruleset),
2. take whole chapters from existing rules files (we did that for printing stuff).

I clearly prefer the 2nd, and your finding (iokit-connection "IOAccelerator") falls in that one.
Though I'd prefer we take the whole block after the ";; Graphics..." from com.apple.WebKit.plugin-common.sb

I'll write the patch in the afternoon or tomorrow.
Jeff, what system version are you running on?

I run 10.9.5 and (allow iokit-open (iokit-connection "IOAccelerator")) is rejected -> the content process crashes, and I have the rules printed on stdout, which means "syntax error somewhere".

Other issue: I have nvidia graphics, which may behave differently.
Attached patch bugzilla-1150765-2.patch (obsolete) — Splinter Review
I had to comment the (iokit-connection ...) on 10.9.5, or else the content process crashes.
Also noteworthy, even with the sandbox disabled, I still get:
OpenGL version detected: 210
OpenGL vendor: Intel Inc.
OpenGL renderer: Intel HD Graphics 4000 OpenGL Engine
OpenGL vendor ('Intel Inc.') recognized as: Intel

Though my MacBook Pro also has an nvidia card built in.
(In reply to André Reinald from comment #12)
> Created attachment 8590017 [details] [diff] [review]
> bugzilla-1150765-2.patch
> 
> I had to comment the (iokit-connection ...) on 10.9.5, or else the content
> process crashes.

Inexact: the first opened window (at launch) works fine. But closing and reopening results in a crash with a "rules dump" on stdout.
(In reply to Jeff Gilbert [:jgilbert] from comment #10)
> For reference, I did manage to find a reference to IOAccelerator on [1], in
> the Graphics section.
> 
> [1]:
> https://developer.apple.com/library/mac/documentation/DeviceDrivers/
> Conceptual/IOKitFundamentals/Families_Ref/Families_Ref.html#//apple_ref/doc/
> uid/TP0000021-BABIGAIA

Though the document makes no mention of sandboxing. The name match may be a coincidence.
(In reply to André Reinald from comment #15)
> (In reply to Jeff Gilbert [:jgilbert] from comment #10)
> > For reference, I did manage to find a reference to IOAccelerator on [1], in
> > the Graphics section.
> > 
> > [1]:
> > https://developer.apple.com/library/mac/documentation/DeviceDrivers/
> > Conceptual/IOKitFundamentals/Families_Ref/Families_Ref.html#//apple_ref/doc/
> > uid/TP0000021-BABIGAIA
> 
> Though the document makes no mention of sandboxing. The name match may be a
> coincidence.

It specifically is about iokit, which is the same thing we restrict with sandboxing. Exact match may be coincidental, but I don't think it's a conceptual coincidence.
Attached patch bugzilla-1150765-3.patch (obsolete) — Splinter Review
Jeff, can you please check if this patch fixes the issue on your machine?
Attachment #8588367 - Attachment is obsolete: true
Attachment #8590017 - Attachment is obsolete: true
Attachment #8590027 - Flags: feedback?(jgilbert)
I'm also curious why, even when the sandbox is disabled, on my machine, the Intel graphics card is used. Shouldn't the nvidia card be used instead?
Attachment #8588783 - Flags: review?(areinald)
Comment on attachment 8590027 [details] [diff] [review]
bugzilla-1150765-3.patch

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

Works for me!
Attachment #8590027 - Flags: feedback?(jgilbert) → feedback+
Comment on attachment 8590027 [details] [diff] [review]
bugzilla-1150765-3.patch

Steven, I changed the rules of the content process sandbox related to accelerated graphics. Adding some "allow", and grouping them under an explicit comment. It fixes Jeff's issues, and will improve readability.
Attachment #8590027 - Flags: review?(smichaud)
Comment on attachment 8590027 [details] [diff] [review]
bugzilla-1150765-3.patch

This looks fine to me.

At some point we should look more closely at these sandbox exceptions, and see if there's a reasonable way to avoid them -- for example by proxying the initialization of OpenGL hardware acceleration through the chrome process.  But I expect that will be a lot of work.
Attachment #8590027 - Flags: review?(smichaud) → review+
(In reply to comment #6)

> Related to bug 1151492?

Probably not.
(In reply to Steven Michaud from comment #21)
> At some point we should look more closely at these sandbox exceptions, and
> see if there's a reasonable way to avoid them -- for example by proxying the
> initialization of OpenGL hardware acceleration through the chrome process. 
> But I expect that will be a lot of work.

I'm not sure it would make sense to move those away from the content process, as it really makes sense to have this API used from there.

Thanks for the review, asking for check-in!
Keywords: checkin-needed
(In reply to comment #18)

> I'm also curious why, even when the sandbox is disabled, on my
> machine, the Intel graphics card is used. Shouldn't the nvidia card
> be used instead?

It's the OS that chooses which card gets used, and as best I can tell
the decision is pretty arbitrary.  But there's a nice utility called
gfxCardStatus (https://gfx.io/) that you can use to force the OS to
decide one way or the other.
(In reply to André Reinald from comment #23)
> (In reply to Steven Michaud from comment #21)
> > At some point we should look more closely at these sandbox exceptions, and
> > see if there's a reasonable way to avoid them -- for example by proxying the
> > initialization of OpenGL hardware acceleration through the chrome process. 
> > But I expect that will be a lot of work.
> 
> I'm not sure it would make sense to move those away from the content
> process, as it really makes sense to have this API used from there.
> 
> Thanks for the review, asking for check-in!

can we get a try run for this change ? thanks!
Flags: needinfo?(areinald)
Keywords: checkin-needed
In progress here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d9d7e1bee33
Flags: needinfo?(areinald)
Mistake, tests were run on 10.6 where sandbox is disabled. New try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f5dcd056920
Tests all green.
Keywords: checkin-needed
Hi, this failed to apply to mozilla-inbound:

patching file security/sandbox/mac/Sandbox.mm
Hunk #3 FAILED at 280
1 out of 3 hunks FAILED -- saving rejects to file security/sandbox/mac/Sandbox.mm.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bugzilla-1150765-3.patch

could you take a look, thanks!
Flags: needinfo?(areinald)
Keywords: checkin-needed
Minor changes were made in the same file (getting rid of XUL dependency).
Rebased the previous patch to current m-c.
Carrying r+.
Attachment #8590027 - Attachment is obsolete: true
Flags: needinfo?(areinald)
Attachment #8595254 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a82f563aec94
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.