Closed
Bug 626301
Opened 13 years ago
Closed 13 years ago
nsDOMWorkerScope's compartment has principals == NULL
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
INVALID
People
(Reporter: luke, Assigned: bent.mozilla)
References
Details
The patch in bug 602994 asserts (notably in EvalKernel) that the principals found via obj->compartment()->principals and via findObjectPrincipals are either the same or subsume each other. This assert fails on test_ctypes.xul because the scope chain's compartment's principals is NULL. The type of the global object for the scope chain is nsDOMWorkerScope. I'm not sure if having a null compartment principals is always wrong, but it seems like at least this should be brought into sync with findObjectPrincipals. Or NULL is equivalent to certain principals? Please advise. The easiest way to reproduce is to apply the WIP2 patch in bug 602994 and run test_ctypes.xul (--chrome --test-path=toolkit/components/toolkit/components/ctypes/tests/test_ctypes.xul).
Updated•13 years ago
|
Assignee: general → bent.mozilla
Comment 1•13 years ago
|
||
Not sure what we can do here. I think non-mainthread cannot have principals. Bent, am I right?
Assignee | ||
Comment 2•13 years ago
|
||
I don't know, we have a principal class that we use for workers, maybe nobody updated the worker code to use them when compartments were added? Principals are here: https://mxr.mozilla.org/mozilla-central/source/dom/src/threads/nsDOMWorkerSecurityManager.cpp#55
Comment 3•13 years ago
|
||
That looks like a dummy principal giving workers unrestricted access, no?
Comment 4•13 years ago
|
||
Yes, but workers are truly sandboxed from things they can't do. The principal is mainly there just to make other code happy.
Can we make it a null principal instead, just so we're not revisiting this conversation in a firedrill once someone adds a new thing to workers in the obvious and straightforward way?
Comment 6•13 years ago
|
||
Yeah, I would prefer a null principal, though, nsNullPrincipal may not be threadsafe.
Comment 7•13 years ago
|
||
Yeah. That should be possible. IIRC the threadsafety of null principals has changed since bent implemented workers.
Comment 8•13 years ago
|
||
We had problems in the old days with null (the pointer) principals -- trust as if system principal, or treat as null (the principal) principal, or throw up your hands in disgust? Workers do not run with system principal so we don't want the first. Null principals need state to distinguish them -- each is under an origin principal in the lattice, right? This says to me we should forbid null pointers and insist on (JSPrincipals *) members being non-null everywhere. /be
Assignee | ||
Comment 9•13 years ago
|
||
I don't quite understand, though. We don't have null (the pointer) principals, or at least we shouldn't. We use JS_CompileUCScriptForPrincipals to compile the worker's script, here: https://mxr.mozilla.org/mozilla-central/source/dom/src/threads/nsDOMWorkerScriptLoader.cpp#831 So why is our compartment not getting any principal? Do we just need to call some other JSAPI func somewhere?
Comment 10•13 years ago
|
||
Note that workers may end up getting a principal
Comment 11•13 years ago
|
||
...er, never mind, partial comment that turned out wrong, that I then forgot to clear it before CCing. Sorry about that!
Comment 12•13 years ago
|
||
JS_NewCompartmentAndGlobalObject(JSContext *cx, JSClass *clasp, JSPrincipals *principals) ^ thats where the principals have to be passed in
Reporter | ||
Comment 13•13 years ago
|
||
Any resolution on this? Is it ok for a compartment to have a NULL principal?
Reporter | ||
Comment 14•13 years ago
|
||
mrbkap explained that it is ok to have NULL principals, so this isn't a bug.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → INVALID
Assignee | ||
Comment 15•13 years ago
|
||
In any case I'm fixing this in my rewrite.
Reporter | ||
Comment 16•13 years ago
|
||
When you do, will we have the global invariant that compartment->principals != NULL?
Assignee | ||
Comment 17•13 years ago
|
||
I think so, yeah.
Reporter | ||
Comment 18•13 years ago
|
||
Cool, well then I'll be able to turn an 'if' into an assert in bug 602994.
You need to log in
before you can comment on or make changes to this bug.
Description
•