1274193 - Audit frame iterators for saved frame chain removal

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

We want to remove the JSContext stack, but first we have to remove saved frame chains.

To remove saved frame chains, we have to audit all frame iterators in SpiderMonkey that use the STOP_AT_SAVED behavior (the default!) to see what they really want.

As a first step, I'm going to make SavedOption a non-default argument and just explicitly pass STOP_AT_SAVED everywhere. After that it will be much easier to grep and convert them one at a time.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Pass SavedOption explicitly

Just pass STOP_AT_SAVED explicitly everywhere.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Testing functions

Shell/testing functions don't really care and can just use GO_THROUGH_SAVED.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Some easy ones

This converts some places where we're only interested in the top frame, and we *know* there was no saved frame change between leaving script and using the iterator, so we can use GO_THROUGH_SAVED.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Trivial debugger ones

Debugger::handleBaselineOsr and Debugger::handleIonBailout are called without saved frame changes after the frame we're interested in.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 5 - More Debugger changes

Debugger::fireDebuggerStatement etc are interested in the top frame and it shouldn't be possible to save frames between that and the debugger hook.

Shu, the main wrinkle is InvokeInterruptCallback. Almost always, that will be called from the interpreter/JIT and the top frame should be sane. However, one can imagine some random piece of C++ code doing the following:

  AutoCompartment ac(cx, compartment);
  ..
  CheckForInterrupt(cx);

I changed it to check cx->compartment() == iter.compartment(), I think that's the sanest behavior - if we switched to a different compartment after leaving script, we shouldn't invoke debugger hooks in the script's compartment or things will just get confusing.

Let me know what you think.

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8754288 [details] [diff] [review]
Part 4 - Trivial debugger ones

Oops, I got a little overzealous (based on the assumption that the debugger should always see everything), didn't mean to steal the review.

Comment 7

[Shu-yu Guo [:shu]]

Comment on attachment 8754312 [details] [diff] [review]
Part 5 - More Debugger changes

Review of attachment 8754312 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Runtime.cpp
@@ +589,5 @@
>          // invoke the onStep handler.
>          if (cx->compartment()->isDebuggee()) {
> +            ScriptFrameIter iter(cx, FrameIter::GO_THROUGH_SAVED);
> +            if (!iter.done() &&
> +                cx->compartment() == iter.compartment() &&

I don't think this is needed -- the iter.script()->stepModeEnabled is a quick heuristic check to see if we should call Debugger:onSingleStep. That is, if iter.script()->stepModeEnabled(), then we *might* need to call the onStep handler, and if it's not enabled, then we definitely don't need to.

If iter.script() ends up referring to a non-debuggee script (despite cx->compartment()->isDebuggee()), Debugger::onSingleStep will do nothing and return JSTRAP_CONTINUE. It's slow to figure this out, hence this check. I don't think it's incorrect to leave out the compartment check.

But of course, run tests and see if things crash or something. :)

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Shu-yu Guo [:shu] from comment #7)
> I don't think it's incorrect to leave out the compartment check.

I intended it for a scenario like this:

* Tab A does alert() and spins a nested event loop.
* Tab B receives an event, ends up calling InvokeInterruptCallback before entering script.
* Now InvokeInterruptCallback tries to Debugger::onSingleStep for the alert()-calling script in tab A.

I know this is an edge case (both are debuggees, interrupt not from script, etc) and triggering an onStep is probably okay. Makes sense?

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Expression decompiler

For the expression decompiler we have a few options. This patch makes it check the script's compartment matches the current compartment.

We could also use FrameIter's principal filtering, but then we may quietly skip frames we're not allowed to see and it could cause even more confusion.

As the expression decompiler is an heuristic, and will almost always be used within one compartment, it seemed easiest and safest to only handle that case.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 7 - rollbackPartiallyInitializedObjects and ReportIncompatibleSelfHostedMethod

TypeNewScript::rollbackPartiallyInitializedObjects looks for a particular JSFunction* callee - that will only match frames belonging to the same compartment and it can safely look through saved frames.

ReportIncompatibleSelfHostedMethod is directly called from script.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 8 - fun.caller and fun.arguments

Initially I thought we had to change fun.caller to use FrameIter's principal filtering, but that would be wrong: if the caller is something we're not allowed to access, we should return null (instead of skipping to the next frame we *are* allowed to see).

We currently use CheckedUnwrap to implement this security check, and I think that will do the right thing with GO_THROUGH_SAVED. It's just like Error().stack: you can see through saved frame boundaries, but we apply security checks.

Comment 12

[Jan de Mooij [:jandem]]

With these patches, the remaining places where we use STOP_AT_SAVED are:

(1) JS::DescribeScriptedCaller and JS::GetScriptedCallerGlobal (jsapi.cpp)

Not sure yet what to do here - there are a bunch of callers...

(2) js::GetOutermostEnclosingFunctionOfScriptedCaller (jsfriendapi.cpp)

The only caller is mozJSComponentLoader::FindTargetObject. bz, do you know what that code is trying to do? We don't wrap the result AFAICS, so maybe we can assert/check the GetOutermostEnclosingFunctionOfScriptedCaller return value is in the same compartment? I'm not familiar with mozJSComponentLoader so I'm not sure what it needs exactly.

(3) js::DescribeScriptedCallerForCompilation (jsscript.cpp)

We use this for direct and indirect eval, to get the caller's filename/lineno/etc. Luke, do you think we should use principal filtering here?

Comment 13

[Jason Orendorff [:jorendorff]]

Comment on attachment 8754731 [details] [diff] [review]
Part 6 - Expression decompiler

Review of attachment 8754731 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsopcode.cpp
@@ +1493,5 @@
>       * Get the second-to-top frame, the caller of the builtin that called the
>       * intrinsic.
>       */
>      ++frameIter;
> +    if (frameIter.done() || !frameIter.hasScript() || frameIter.compartment() != cx->compartment())

You should be able to assert the compartment invariant here, right?

I'm not opposed to asserting but then testing anyway. Decompiler. :-P

Comment 14

[Jason Orendorff [:jorendorff]]

D'oh. Of course you can't. Never mind.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> bz, do you know what that code is trying to do?

Yeah.  In the case when the component loader uses a single global for all components, this is trying to find the right "this" object to evaluate the xpcom component script against (which is not the global itself, to avoid different components stomping on each other).  https://bugzilla.mozilla.org/show_bug.cgi?id=807845#c0 actually summarizes what's going on pretty well.

Given all that, restricting to "same compartment" just means restricting to the shared component global.  Maybe that's OK anyway.  

But the other thing is, jsloader.reuseGlobal is only true on b2g, so this code is only used on b2g.  I don't know whether there are any plans to use it on non-b2g and hence whether we care about it at all...  Bobby, do you know?

Comment 16

[Boris Zbarsky [:bzbarsky]]

> We use this for direct and indirect eval, to get the caller's filename/lineno/etc.
> Luke, do you think we should use principal filtering here?

We totally should.  Consider this conceptual testcase:

  <script>
    callSomeBrowserAPIThatInvokesTheCallback(eval.bind(undefined, "stuff"));
  </script>

If callSomeBrowserAPIThatInvokesTheCallback is implemented in C++, the eval will get the file/line of the callsite, right?  But if it's implemented in chrome JS, in our current world it won't.  I think it should, because that sort of implementation detail should not affect page-visible behavior.  Doing principal-filtering would allow us to do that.  If desired, I can try to find an actual API where this is an issue, but we certainly have a test API like that if we just want to write a test.

Note that there is still weirdness in cases like:

  <script>
    var el = document.documentElement;
    el.addEventListener("click", eval.bind(undefined, "throw new Error()"));
    el.click();
  </script>

This logs an exception with filename set to ""file:///Users/bzbarsky/baz.html line 4 > eval": the location of the click() call.  Which makes sense in terms of our impl, but it would be more helpful to use the file/line of the addEventListener call or something, in terms of actual debuggability.  That's a separate issue, though.

Comment 17

[Luke Wagner [:luke]]

(Thanks bz, agreed.)

Comment 18

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #15)
> > bz, do you know what that code is trying to do?
> 
> Yeah.  In the case when the component loader uses a single global for all
> components, this is trying to find the right "this" object to evaluate the
> xpcom component script against (which is not the global itself, to avoid
> different components stomping on each other). 
> https://bugzilla.mozilla.org/show_bug.cgi?id=807845#c0 actually summarizes
> what's going on pretty well.
> 
> Given all that, restricting to "same compartment" just means restricting to
> the shared component global.  Maybe that's OK anyway.  
> 
> But the other thing is, jsloader.reuseGlobal is only true on b2g, so this
> code is only used on b2g.  I don't know whether there are any plans to use
> it on non-b2g and hence whether we care about it at all...  Bobby, do you
> know?

Bill had some plans to do something similar in Desktop, and might know better than me whether this is relevant. Also worth asking njn what he thinks about eliminating that machinery.

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Bobby Holley (busy) from comment #18)
> Bill had some plans to do something similar in Desktop, and might know
> better than me whether this is relevant. Also worth asking njn what he
> thinks about eliminating that machinery.

The patch I was working on did something pretty different than what B2G does, so I don't care if we remove it or significantly change it.

Comment 20

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> (In reply to Shu-yu Guo [:shu] from comment #7)
> > I don't think it's incorrect to leave out the compartment check.
> 
> I intended it for a scenario like this:
> 
> * Tab A does alert() and spins a nested event loop.
> * Tab B receives an event, ends up calling InvokeInterruptCallback before
> entering script.
> * Now InvokeInterruptCallback tries to Debugger::onSingleStep for the
> alert()-calling script in tab A.
> 
> I know this is an edge case (both are debuggees, interrupt not from script,
> etc) and triggering an onStep is probably okay. Makes sense?

Okay -- disallowing this seems fine, even preferred, to me.

Comment 21

[Jan de Mooij [:jandem]]

Attachment: Part 9 - DescribeScriptedCallerForCompilation

This adds principal filtering to DescribeScriptedCallerForCompilation.

I added a jit-test based on bz's scenario, it's possible to test this in the shell with newGlobal's principal option.

Comment 22

[Jan de Mooij [:jandem]]

The patches in this bug take care of everything except (1) and (2) in comment 12. Those APIs are mostly used by code outside SpiderMonkey and I don't know what the plan there is.

Is there more I can do here or is this enough to unblock you?

Comment 23

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #22)
> Those APIs are mostly used by code outside SpiderMonkey and I
> don't know what the plan there is.

I could try working on this too, if someone can explain what the plan is for this (and for JS_IsRunning while we're at it). It seems pretty complicated though, so not sure how useful that'd be.

Comment 24

[Nicholas Nethercote [inactive]]

(In reply to Bill McCloskey (:billm) from comment #19)
> 
> The patch I was working on did something pretty different than what B2G
> does, so I don't care if we remove it or significantly change it.

I'm ok with it, too.

Comment 25

[Boris Zbarsky [:bzbarsky]]

OK, I thought about GetOutermostEnclosingFunctionOfScriptedCaller some more and I'm not actually sure what the right thing there is.  Can we just stop once we hit something from another compartment?  Are there situations where same-compartment code will be up from us but across an event loop spin here?  Kyle, do you happen to recall what the desired semantics are here?

For GetScriptedCallerGlobal, we set the scripted caller override any time we want to cut things off.  And the only caller of this function does some special principal handling (treats non-subsumed scripted caller different from no scripted caller).  So we definitely shouldn't principal-filter here, I think, but it's fine to walk across saved stacks in today's world.

For DescribeScriptedCaller, I would have to audit the callsites and see what's going on at them and what they end up using things for.  In _general_ I think principal-filtering is the right thing there, but I'd need to check them all to make sure.  I'm happy to take that part; it might be worth spinning out into a separate bug.

> and for JS_IsRunning while we're at it

I just looked at the non-js.cpp and non-jsapi-test consumers of JS_IsRunning.  The ones in jscntxt.cpp are not reached in Gecko, because cx->options().autoJSAPIOwnsErrorReporting() is always true.  The one in ~AutoLastFrameCheck in jsapi.cpp doesn't matter for the same reason; the overall condition tests false no matter what JS_IsRunning says.  The one in XPCComponents.cpp... it's totally not expecting JS_IsRunning to depend on whether frames are saved!  This was added as a way to run GC when the stack fully unwinds, so we know the stack scanner won't entrain stuff; see bug 661927.  In today's world, I guess it would want all the Rooted off the stack or something.  But of course none of that stack stuff is affected by JS_IsRunning.  We should add an API that answers the real question we care about.  Probably whether there are Rooted live on the lists of the JSContext involved (and maybe on the runtime?).  Terrence, does that seem like the most accurate reflection of what's needed here?  The other thing that might be checkable is whether cx is in a compartment or something, but that doesn't seem like what this code is about.  Anyway, this should be a separate bug, because it's going to change how some tests work I expect...

Comment 26

[Jan de Mooij [:jandem]]

Attachment: Part 10 - GetScriptedCallerGlobal

According to comment 25 this should be fine. I wanted to ask bz to review this, but he's not accepting review requests atm :)

(FWIW the fast path I added here last week also doesn't check for saved frame boundaries.)

Comment 27

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] from comment #25)
> For DescribeScriptedCaller, I would have to audit the callsites and see
> what's going on at them and what they end up using things for.  In _general_
> I think principal-filtering is the right thing there, but I'd need to check
> them all to make sure.  I'm happy to take that part; it might be worth
> spinning out into a separate bug.

OK, I filed bug 1274915.

> The one in
> XPCComponents.cpp... it's totally not expecting JS_IsRunning to depend on
> whether frames are saved!  This was added as a way to run GC when the stack
> fully unwinds, so we know the stack scanner won't entrain stuff; see bug
> 661927.

Hm that one also has O(NumContexts * NumActivations) behavior. I can work on this (bug 981201).

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Boris Zbarsky [:bz] from comment #25)
> OK, I thought about GetOutermostEnclosingFunctionOfScriptedCaller some more
> and I'm not actually sure what the right thing there is.  Can we just stop
> once we hit something from another compartment?  Are there situations where
> same-compartment code will be up from us but across an event loop spin here?
> Kyle, do you happen to recall what the desired semantics are here?

My recollection is that we did global reuse by compiling each component/JSM as a function in the context of a single global and applying said functions to a unique object.  If you have a JSM that does

function doStuff() {
  Components.utils.import("blahblahblah"); // Installs a blah() function.
}

doStuff();
blah();

Without global reuse this just works.  With global reuse, if you grab the immediate caller's this object and install the properties on there this script will fail.  Instead we need to go out to the outermost function (the "fake" one that we created) and get its this object and install the properties on there.

I think you can always terminate your search at a compartment boundary here.

Comment 29

[Boris Zbarsky [:bzbarsky]]

I meant to needinfo terrence for the end of comment 25.

Comment 30

[Terrence Cole [:terrence]]

It seems reasonable and the patch should be a trivial to write, regardless. The only question is whether there are any perma-rooteds that live above the event loop. I think probably not, but we'll want to double check.

Comment 31

[Jan de Mooij [:jandem]]

(In reply to Terrence Cole [:terrence] from comment #30)
> The only question is whether there are any perma-rooteds that live above the
> event loop. I think probably not, but we'll want to double check.

Yes that was also my worry... We could also go with an API that simply returns whether there are any activations on the stack, that might be good enough here and it avoids that potential footgun.

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f96ba14774c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/24ac08253efd
https://hg.mozilla.org/integration/mozilla-inbound/rev/722a014fc551
https://hg.mozilla.org/integration/mozilla-inbound/rev/23d2fda9a914
https://hg.mozilla.org/integration/mozilla-inbound/rev/192f327451ec

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/701378e19df1
https://hg.mozilla.org/integration/mozilla-inbound/rev/faa07a14d470
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee7ad0668921
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fa9dceab5e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/472fd2e0cf31

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f96ba14774c0
https://hg.mozilla.org/mozilla-central/rev/24ac08253efd
https://hg.mozilla.org/mozilla-central/rev/722a014fc551
https://hg.mozilla.org/mozilla-central/rev/23d2fda9a914
https://hg.mozilla.org/mozilla-central/rev/192f327451ec

Comment 35

[Jan de Mooij [:jandem]]

Attachment: Part 11 - GetOutermostEnclosingFunctionOfScriptedCaller

According to Kyle it's okay to stop at compartment boundaries, so this just checks the compartment.

With this, we only use STOP_AT_SAVED in DescribeScriptedCaller (bug 1274915).

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b47a1a49cda2

Comment 37

[Jan de Mooij [:jandem]]

And we're done here. The last STOP_AT_SAVED user is DescribeScriptedCaller, that's bug 1274915.

Comment 38

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8dbe70e2500

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73cf91bc2b67

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/701378e19df1
https://hg.mozilla.org/mozilla-central/rev/faa07a14d470
https://hg.mozilla.org/mozilla-central/rev/ee7ad0668921
https://hg.mozilla.org/mozilla-central/rev/8fa9dceab5e6
https://hg.mozilla.org/mozilla-central/rev/472fd2e0cf31

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/73cf91bc2b67