Last Comment Bug 625199 - kill dummy frames
: kill dummy frames
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 2 votes (vote)
: mozilla17
Assigned To: Luke Wagner [:luke]
:
Mentors:
: 641558 (view as bug list)
Depends on: 602994 cpg 754202 785634
Blocks: 604813 713797
  Show dependency treegraph
 
Reported: 2011-01-12 15:02 PST by Luke Wagner [:luke]
Modified: 2015-10-07 09:57 PDT (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.x+


Attachments
WIP 1 (91.40 KB, patch)
2011-01-13 00:32 PST, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
WIP 2 (110.00 KB, patch)
2011-01-18 18:33 PST, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
preparatory cleanup (43.67 KB, patch)
2012-08-20 14:03 PDT, Luke Wagner [:luke]
mrbkap: review+
Details | Diff | Splinter Review
do the deed (101.02 KB, patch)
2012-08-20 14:07 PDT, Luke Wagner [:luke]
mrbkap: review+
Details | Diff | Splinter Review
make AutoCompartment infallible (199.37 KB, patch)
2012-08-21 14:51 PDT, Luke Wagner [:luke]
bobbyholley: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2011-01-12 15:02:46 PST
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).
Comment 1 Andreas Gal :gal 2011-01-12 15:05:17 PST
We can probably limp along without this, and we should try for a beta.
Comment 2 Luke Wagner [:luke] 2011-01-13 00:32:23 PST
Created attachment 503428 [details] [diff] [review]
WIP 1

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).
Comment 3 Luke Wagner [:luke] 2011-01-13 00:34:23 PST
Andreas, Waldo: what *should* happen in a cross-compartment 'eval' call (such as the one in jit-test/tests/jaeger/bug588363-1.js)?
Comment 4 Luke Wagner [:luke] 2011-01-13 00:36:20 PST
To save everyone some trouble, the test is:

  ({eval} = Object.defineProperty(evalcx("lazy"), "", {}))
  eval("eval(/x/)", [])
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2011-01-13 00:52:03 PST
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.
Comment 6 Brendan Eich [:brendan] 2011-01-13 03:06:12 PST
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
Comment 7 Luke Wagner [:luke] 2011-01-13 10:25:03 PST
(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.
Comment 8 Luke Wagner [:luke] 2011-01-13 10:26:59 PST
(In reply to comment #7)
...excuse the mangled sentence.
Comment 9 Luke Wagner [:luke] 2011-01-13 10:28:10 PST
Also, to be clear, its not the one- vs. two-arg form of eval, you get the same failure with: evalcx('').eval('')
Comment 10 Brendan Eich [:brendan] 2011-01-13 10:31:24 PST
(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
Comment 11 Luke Wagner [:luke] 2011-01-18 18:06:31 PST
Doing this has stirred up more work than we originally expected.  Postponing until after 4.0, but still really want.
Comment 12 Luke Wagner [:luke] 2011-01-18 18:33:46 PST
Created attachment 504948 [details] [diff] [review]
WIP 2

Leaving this here.  Applies on top of WIP 3 in bug 602994.
Comment 13 Luke Wagner [:luke] 2011-01-19 11:49:41 PST
jag: why clear .x?
Comment 14 jag (Peter Annema) 2011-01-19 14:15:06 PST
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 '?'
Comment 15 Luke Wagner [:luke] 2011-01-19 16:13:53 PST
No problemo, just making sure I hadn't violated some protocol :)
Comment 16 Luke Wagner [:luke] 2012-06-08 09:24:37 PDT
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.
Comment 17 Bobby Holley (:bholley) (busy with Stylo) 2012-06-08 09:48:43 PDT
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. :-)
Comment 18 Luke Wagner [:luke] 2012-08-20 14:03:06 PDT
Created attachment 653510 [details] [diff] [review]
preparatory cleanup

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
Comment 19 Luke Wagner [:luke] 2012-08-20 14:07:47 PDT
Created attachment 653513 [details] [diff] [review]
do the deed

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.
Comment 20 Luke Wagner [:luke] 2012-08-21 14:51:13 PDT
Created attachment 653967 [details] [diff] [review]
make AutoCompartment infallible

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 21 Bobby Holley (:bholley) (busy with Stylo) 2012-08-21 18:02:51 PDT
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
Comment 22 Luke Wagner [:luke] 2012-08-21 18:40:39 PDT
(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.
Comment 23 Bobby Holley (:bholley) (busy with Stylo) 2012-08-22 10:29:35 PDT
(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 24 Blake Kaplan (:mrbkap) 2012-08-23 16:49:29 PDT
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?
Comment 26 Luke Wagner [:luke] 2012-08-23 18:14:49 PDT
arg, mac-only C++ files changing since last try build...
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3b7774cb5be
Comment 27 Luke Wagner [:luke] 2012-08-23 18:45:55 PDT
...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.)
Comment 29 André Bargull 2015-10-07 09:57:49 PDT
*** Bug 641558 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.