Closed Bug 1173002 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: fitzgen, Assigned: bent.mozilla)

References

Details

Attachments

(2 files)

: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.
See Also: → 1149294
(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.
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.
Isn't ChromeWorker just a worker running with the off-main-thread equivalent of System Principal?
(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.
> 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?
Component: DOM → DOM: Workers
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).
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.
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....
Summary: [ChromeOnly, Exposed=(Window,System,Worker)] doesn't expose things to a ChromeWorker → [ChromeOnly, Exposed=(Window,System,Worker)] doesn't expose things to a worker loaded from resource:// by a system caller
(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.
(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. :-(
> 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?
(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.
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.  :(
Flags: needinfo?(dave2008713)
Flags: needinfo?(bent.mozilla)
Flags: needinfo?(dave2008713) → needinfo?(bobbyholley)
(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.
Flags: needinfo?(bent.mozilla)
> 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.
Attached patch Patch, v1Splinter Review
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Attachment #8617492 - Flags: review?(bzbarsky)
Looks like we're missing a simple thing here. This should make it work.
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....
Attachment #8617492 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/3c21606016f3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Flags: needinfo?(bobbyholley)
You need to log in before you can comment on or make changes to this bug.