1173002 - [ChromeOnly, Exposed=(Window,System,Worker)] doesn't expose things to a worker loaded from resource:// by a system caller

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

:bholley and :smaug agree that this is probably unsupported by codegen, but should be.

This is biting me in bug 1149294, where I am trying to expose ChromeUtils and HeapSnapshot to workers.

Comment 1

[Bobby Holley (:bholley)]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #0)
> This is biting me in bug 1149294, where I am trying to expose ChromeUtils
> and HeapSnapshot to workers.

If ChromeUtils ends up being the grab-bag of utility methods that I imagine it will be, I think exposing it on Workers is a bad idea - we should add a separate interface called ThreadsafeChromeUtils for such things.

Comment 2

[Boris Zbarsky [:bzbarsky]]

When you say "things", do you mean a constructor or an interface member?  The mechanisms there are pretty different.

Assuming you meant the constructor, what is the url the ChromeWorker is loaded from and what principal is it running with?  Because the definition of "ChromeOnly" for a constructor in workers is ThreadsafeIsCallerChrome, which has nothing to do with whether the worker is a ChromeWorker and everything to do with the actual permissions it's running with.

Comment 3

[Bobby Holley (:bholley)]

Isn't ChromeWorker just a worker running with the off-main-thread equivalent of System Principal?

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Not doing reviews right now from comment #2)
> When you say "things", do you mean a constructor or an interface member? 
> The mechanisms there are pretty different.

The ChromeUtils singleton interface and the HeapSnapshot constructor.

> Assuming you meant the constructor, what is the url the ChromeWorker is
> loaded from and what principal is it running with?  Because the definition
> of "ChromeOnly" for a constructor in workers is ThreadsafeIsCallerChrome,
> which has nothing to do with whether the worker is a ChromeWorker and
> everything to do with the actual permissions it's running with.

This is inside an xpcshell test, loading a ChromeWorker with a "resource://test/..." url.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Isn't ChromeWorker just a worker running with the off-main-thread equivalent of System
> Principal?

Afaict, no.

A ChromeWorker gets some extra APIs (OS.Constants constants and ctypes bits, looks like).  But afaict that's orthogonal to its principal.

ThreadsafeIsCallerChrome does IsCurrentThreadRunningChromeWorker() which in turn does GetCurrentThreadWorkerPrivate()->UsesSystemPrincipal().

I agree that exposing ctypes but not giving it a system principal seems weird.  ccing the folks who might know something about what ChromeWorker is supposed to mean.

> loading a ChromeWorker with a "resource://test/..." url.

Odd.  That should give the worker a system principal, I'd think, due to the shenanigans in the worker scriptloader's OnStreamCompleteInternal.

Do you mind sending me a patch that shows the problem for you?

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: heap-snapshots-worker.patch

Running the new test introduced in this patch results in reference errors for both ChromeUtils and HeapSnapshot inside a ChromeWorker (and normal Worker too, if you modify the test).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Thanks.

So yeah we're landing in UsesSystemPrincipal() and have:

(gdb) p mLoadInfo.mPrincipalIsSystem
$2 = false
(gdb) p mIsChromeWorker
$3 = true

So the reason the OnStreamCompleteInternal shenanigans don't matter is that we set up the global object from CompileScriptRunnable::WorkerRun which creates the global and all that.  That's when we end up doing the WorkerPrivate::RegisterBindings.  This happens _before_ we've actually done the script load and munged the principal or resource://.  I recall ehsan or nsm running into this recently too.

And in particular, mLoadInfo.mPrincipalIsSystem gets updated to be "true" when ScriptLoaderRunnable::OnStreamCompleteInternal calls WorkerPrivate::SetPrincipal.  But that's long after we set up our global.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So I'm not sure how much I care about the mPrincipalIsSystem getting updated too late for the resource:// case, since I really want to nuke that resource:-upgraded-to-system hack anyway....

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Not doing reviews right now from comment #8)
> So I'm not sure how much I care about the mPrincipalIsSystem getting updated
> too late for the resource:// case, since I really want to nuke that
> resource:-upgraded-to-system hack anyway....

So how would I avoid a resource:// URI in this case? I tried using a file:// URI and relative URI and in both cases the script won't be loaded.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Not doing reviews right now from comment #7)
> Thanks.
> 
> So yeah we're landing in UsesSystemPrincipal() and have:
> 
> (gdb) p mLoadInfo.mPrincipalIsSystem
> $2 = false
> (gdb) p mIsChromeWorker
> $3 = true

The distinction between "chrome" and "system" in docshell land was one of the worst things that ever happened in Gecko. I can't believe we replicated it in workers. :-(

Comment 11

[Boris Zbarsky [:bzbarsky]]

> So how would I avoid a resource:// URI in this case?

Can you use a chrome:// URI?

But more importantly, let's forget the test. What will actual consumers be using here?

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Not doing reviews right now from comment #11)
> > So how would I avoid a resource:// URI in this case?
> 
> Can you use a chrome:// URI?

That's a good question. I don't know if one can access files in the test directory via a chrome:// URI.

> But more importantly, let's forget the test. What will actual consumers be
> using here?

The other workers used by the devtools server are resource:// URIs, so probably that, if possible. If we have to avoid resource:// URIs, then I'm sure we could hack around it.

Comment 13

[Boris Zbarsky [:bzbarsky]]

OK.  Ben, Bobby, how do you want this stuff to actually work?  It _really_ bothers me that we're implicitly upgrading resource stuff to system in various places.  It will come back to bite us security wise.  :(

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Bobby Holley (:bholley) from comment #10)
> > (gdb) p mLoadInfo.mPrincipalIsSystem
> > $2 = false
> > (gdb) p mIsChromeWorker
> > $3 = true

This is probably bad... ChromeWorker should always have a system principal I think. You can of course have a system principal regular Worker, it just doesn't get the ctypes (and someday other non-standard APIs).

> The distinction between "chrome" and "system" in docshell land was one of
> the worst things that ever happened in Gecko. I can't believe we replicated
> it in workers. :-(

We're not trying to have two levels of privilege or anything, it's just the name we gave to a type of worker that has extra APIs exposed. ChromeWorker could grow sync file apis, or sockets, or anything.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> ChromeWorker should always have a system principal I think.

I guess this is true because we never run workers whose loader is system but are not themselves system, and only system things can start a ChromeWorker.  OK.

At least once the load is done, since all that enforcement happens there.

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Looks like we're missing a simple thing here. This should make it work.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8617492 [details] [diff] [review]
Patch, v1

r=me, but I think it's worth documenting here why it's ok to set mPrincipalIsSystem: because in practice this worker will only run only if the script loader comes up with the system principal too.

And document near that code that this code depends on that....

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c21606016f3

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c21606016f3