The goal is to remove dummy frames. They serve one primary purpose: provide a scope chain to serve as the context's "current scope chain" before the first interpreted frame is pushed (if one is ever pushed). This can be achieved without dummy frames by: - when entering a compartment, store the scope chain in a cx field and set cx->regs->fp to null - when asking about the current scope chain, use the cx field if cx->regs->fp is null, and use cx->regs->fp->scopeChain() otherwise. This allows us to enter a compartment infallibly (since stack frame allocation can oom) which allows us to merge JSAutoEnterCompartment with JSAutoRequest (which is used in some infallible contexts) which will kill some subtle compartment mismatch bugs (which is a FF4 blocker). It also allows us to kill cx->globalObject (which itself can be the source of subtle compartment bugs). It also totally rocks since we can now assume every frame is a scripted frame (and remove any conditionals pertaining to isDummyFrame/isScriptedFrame).

We can probably limp along without this, and we should try for a beta.

Mostly works, attacking tests. I think this has caught one family of potential cross-compartment mismatches: places that use js_GetScriptedCaller to find the "calling" stack frame and then use that to get objects associated with that stack frame (like 'this' or 'callee' or 'script'). Without this patch, that scripted caller frame can be from a different compartment, so any derived objects will be accessed from the wrong compartment. With the patch, each compartment-enter severs the fp->prev chain by clearing cx->regs. E.g.: http://mxr.mozilla.org/mozilla-central/source/js/src/jsobj.cpp#1113. now throws (no scripted caller) when 'eval' is called via a cross-compartment function proxy (so jit-test/tests/jaeger/bug588363-1.js now throws).

Andreas, Waldo: what *should* happen in a cross-compartment 'eval' call (such as the one in jit-test/tests/jaeger/bug588363-1.js)?

To save everyone some trouble, the test is: ({eval} = Object.defineProperty(evalcx("lazy"), "", {})) eval("eval(/x/)", [])

It should "work", in some sense. If the call doesn't occur immediately via an expression of the form |eval(...)|, it should definitely be an indirect eval. ES5 specifies eval, and it doesn't say it should throw EvalError or anything like that, like ES3 permitted. And it seems the only sensible position, although ES5 doesn't address it, is that this should be an eval in the context of the global object from which the eval function was retrieved. (I don't remember if we do this.) If it does occur immediately via |eval(...)|...I argue in bug 608473 it should also be an indirect eval, although we don't currently implement that. The other case that apparently tests is eval with nothing on the call stack at all -- no cross-compartment call or anything. That's bug 602994. It definitely should be an indirect eval. I'd think it would use the particular eval function's global as its context, although again ES5 doesn't speak to this.

The eval call in comment 4 is direct. It calls the sandbox's eval, though. ES5 and earlier do not admit more than one global object, so they're not helpful. But that's a two-arg eval call, and we ignore the second argument now. Is that just a crash test, because it's not testing direct vs. indirect eval. Random patch comments: +∞ on the simplification of eliminating dummy frames and unifying request and compartment entry/exit. globalObject_goingAwaySoon, bleah -- that _ trick I suggested worked once but I'm not buying it here. When is initialScopeChain not globalObject, I'm curious? /be

(In reply to comment #6) > globalObject_goingAwaySoon, bleah -- that _ trick I suggested worked once but > I'm not buying it here. Heh, I made it an ugly duck b/c Andreas should be nuking it right after this patch lands and it helps me remember what the code is ultimately going to look like. > When is initialScopeChain not globalObject, I'm curious? (Until said Andreas-killing-of-globalObject) they differ if you have JS_SetGlobalObject(x) and then enters a compartment with y where y->getGlobal() != x.

(In reply to comment #7) ...excuse the mangled sentence.

Also, to be clear, its not the one- vs. two-arg form of eval, you get the same failure with: evalcx('').eval('')

(In reply to comment #7) > Heh, I made it an ugly duck b/c Andreas should be nuking it right after this > patch lands and it helps me remember what the code is ultimately going to look > like. I think wait for "soon", don't predict it in names in code. That is a curse that assures we'll be reading it for the next year. > > When is initialScopeChain not globalObject, I'm curious? > > (Until said Andreas-killing-of-globalObject) they differ if you have > JS_SetGlobalObject(x) and then enters a compartment with y where y->getGlobal() > != x. That is an XSS bug on its face. I'm still not sure when initialScopeChain != globalObject in reality or in debugged reality. Andreas? /be

Doing this has stirred up more work than we originally expected. Postponing until after 4.0, but still really want.

Leaving this here. Applies on top of WIP 3 in bug 602994.

jag: why clear .x?

Bah. That was unintentional. Usually I'm paranoid enough to do a final reload before submitting, but ... Who can put that back? Best I can do here is change it to '?'

No problemo, just making sure I hadn't violated some protocol :)

Bobby: it looks like all the dependent bugs are fixed (much thanks to you of course). Is there anything, from a CAPS perspective, that would prevent us from ripping out dummy frames? IIUC, even before enablePrivilege stops using JS_SetFrameAnnotation, it won't matter since the annotation would be on a scripted (i.e., non-dummy) frame.

If you rip it out and the tree is green, it's probably ok. I don't understand the CAPS goop well enough to say for sure, though. Blake would know better. :-)

This patch prepares for the next one with a few simple changes: - we can use cx->global in a lot of places instead of passing 'parent' around - we don't need to use innerized-cx->globalObject as the scope chain, we have cx->global now! - I privatized cx->globalObject and renamed it to defaultCompartmentObject because that is all it does

Most of this patch is just removing uses of isScriptFrame. The thought-provoking part is the new JSContext::(enter|leave)Compartment functions and how they interact with (save|resore)FrameChain and setting the defaultCompartmentObject. Fortunately, it is all pretty simple. It will be a good bit simpler once we can replace entering a request with entering a compartment b/c then we can stop caring about defaultCompartmentObject. Looking green on try.

This patch changes the API for AutoCompartment and JSAutoEnterCompartment to be infallible. To deal with fallibility, we currently have an bool-returnin enter() method. Since this isn't necessary, I removed it so that the constructors do the entering. In maybe a fourth of the cases, enter() was used to conditionally enter the compartment. In these cases, I used Maybe<AutoCompartment>. Lastly, since I was touching all uses anyways, I renamed JSAutoEnterCompartment to JSAutoCompartment for symmetry with js::AutoCompartment. The patch is mostly mechanical, but requesting bholley for review b/c of all the proxy, XPConnect and DOM touching.

Comment on attachment 653967 [details] [diff] [review] make AutoCompartment infallible >diff --git a/dom/bindings/Codegen.py b/dom/bindings/Codegen.py >- JSAutoEnterCompartment ac; >- if (js::GetGlobalForObjectCrossCompartment(parent) != aScope) { >- if (!ac.enter(aCx, parent)) { >- return NULL; >- } >- } >- >+ JSAutoCompartment ac(aCx, parent); This may have been done this way for performance reasons. Is the no-op path for compartment enter cheap enough now that this shouldn't matter? >diff --git a/js/src/jscntxt.cpp b/js/src/jscntxt.cpp >--- a/js/src/jscntxt.cpp >+++ b/js/src/jscntxt.cpp >@@ -215,20 +215,16 @@ bool > JSRuntime::initSelfHosting(JSContext *cx) > { > JS_ASSERT(!selfHostedGlobal_); > RootedObject savedGlobal(cx, JS_GetGlobalObject(cx)); > if (!(selfHostedGlobal_ = JS_NewGlobalObject(cx, &self_hosting_global_class, NULL))) > return false; > JS_SetGlobalObject(cx, selfHostedGlobal_); > >- JSAutoEnterCompartment ac; >- if (!ac.enter(cx, cx->global())) >- return false; >- Hm, why is this ok? >@@ -3798,20 +3775,18 @@ DebuggerObject_getOwnPropertyDescriptor( > > RootedId id(cx); > if (!ValueToId(cx, argc >= 1 ? args[0] : UndefinedValue(), id.address())) > return false; > > /* Bug: This can cause the debuggee to run! */ > AutoPropertyDescriptorRooter desc(cx); > { >- AutoCompartment ac(cx, obj); >- if (!ac.enter() || !cx->compartment->wrapId(cx, id.address())) >- return false; >- >+ Maybe<AutoCompartment> ac; >+ ac.construct(cx, obj); Why do we no longer need to wrap the id? Righteous. r=bholley

(In reply to Bobby Holley (:bholley) from comment #21) > >+ JSAutoCompartment ac(aCx, parent); > > This may have been done this way for performance reasons. Is the no-op path > for compartment enter cheap enough now that this shouldn't matter? Entering a compartment (nop or not) is now very very fast (store and ++). Given that GetGlobalForObjectCrossCompartment and the branch isn't free, it seemed better to always enter/leave. Note: the JSAPI version (JSAutoEnterCompartment) does use a JS_PUBLIC_API call, but this could be made totally inline if we actually saw this hot enough to show up on profiles. > >- if (!ac.enter(cx, cx->global())) > >- return false; > >- > > Hm, why is this ok? Since cx->global() === cx->compartment->global, cx->global()->compartment == cx->compartment. > >- if (!ac.enter() || !cx->compartment->wrapId(cx, id.address())) > >- return false; > >- > >+ Maybe<AutoCompartment> ac; > >+ ac.construct(cx, obj); > > Why do we no longer need to wrap the id? Nice catch! I didn't mean to lose that line; slip o' the fingers.

(In reply to Luke Wagner [:luke] from comment #22) > (In reply to Bobby Holley (:bholley) from comment #21) > > >+ JSAutoCompartment ac(aCx, parent); > > > > This may have been done this way for performance reasons. Is the no-op path > > for compartment enter cheap enough now that this shouldn't matter? > > Entering a compartment (nop or not) is now very very fast (store and ++). > Given that GetGlobalForObjectCrossCompartment and the branch isn't free, it > seemed better to always enter/leave. Note: the JSAPI version > (JSAutoEnterCompartment) does use a JS_PUBLIC_API call, but this could be > made totally inline if we actually saw this hot enough to show up on > profiles. I think bz wrote that code, so he might have an idea of whether it needs to be inlined. Boris?

Comment on attachment 653513 [details] [diff] [review] do the deed Review of attachment 653513 [details] [diff] [review]: ----------------------------------------------------------------- Woo-hoo! ::: js/src/jscntxtinlines.h @@ +578,5 @@ > + enterCompartmentDepth_--; > + if (hasEnteredCompartment() || !defaultCompartmentObject_) > + compartment = c; > + else > + compartment = defaultCompartmentObject_->compartment(); I'd like to see a comment here explaining why this is correct. It took me a little while to work my way through the case analysis to figure out when |c| is correct vs. when we want to ignore it in favor of our possibly new defaultCompartmentObject_. @@ +593,5 @@ > + /* > + * If JSAPI callers want to JS_SetGlobalObject while code is running, > + * they must have entered a compartment (otherwise there will be no > + * final leaveCompartment call to set the context's compartment back to > + * defaultCompartmentObject->compartment(). Nit: Missing ). ::: js/src/jsdbgapi.cpp @@ -598,2 @@ > > js::AutoCompartment ac(cx, fp->scopeChain()); Nit: Nuke the extra newline above ac. ::: js/src/jsobj.cpp @@ +2644,5 @@ > JSAtom *atom; > > JSScript *script = cx->stack.currentScript(); > + if (!script) > + return false; Why is this necessary now?

Thanks Blake! (For the last comment, from irc: agreed it is not necessary.) https://hg.mozilla.org/integration/mozilla-inbound/rev/ea32388d45a8 https://hg.mozilla.org/integration/mozilla-inbound/rev/986c07b3f3e6 https://hg.mozilla.org/integration/mozilla-inbound/rev/0d61ae018d9f

arg, mac-only C++ files changing since last try build... https://hg.mozilla.org/integration/mozilla-inbound/rev/a3b7774cb5be

...and it turns out that the NULL check (in js_GetPropertyHelper) was necessary: https://hg.mozilla.org/integration/mozilla-inbound/rev/a2834312d80d (The currentScript != NULL check is dominated by a pc == NULL check, but currentScript returns NULL when cx->fp is in another compartment.)

https://hg.mozilla.org/mozilla-central/rev/ea32388d45a8 https://hg.mozilla.org/mozilla-central/rev/986c07b3f3e6 https://hg.mozilla.org/mozilla-central/rev/0d61ae018d9f https://hg.mozilla.org/mozilla-central/rev/a3b7774cb5be https://hg.mozilla.org/mozilla-central/rev/a2834312d80d