979730 - Remove saved frame chains

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

See bug 960820 comment 8. We first need to remove all of the stack iteration dependencies.

Comment 1

[Bobby Holley (:bholley)]

Hm, I'm getting more skeptical about doing this.

It seems like one of the primary uses of saved frame chains is to interrupt script frame iteration when we start executing something unrelated.

There are lots of places in the JS engine that do something like:

NonBuiltinScriptFrameIter iter(cx);
if (iter.done())
    return;

or

ScriptFrameIter iter(cx);
if (iter.isFunctionFrame())
    data.calleeToken = CalleeToToken(iter.callee());

Suppose then that someone does a synchronous event dispatch, and that the event handler is non-scripted. If the associated JSNative ends up triggering one of these iterations, it'll do the wrong thing. An audit _might_ reveal that this can't happen today, but that's a pretty weak guarantee.

So it seems like we want saved frame chains, probably one for every script entry point (which mark these discontinuities from the perspective of the browser). Thoughts, luke?

Comment 2

[Luke Wagner [:luke]]

If *all* ScriptFrameIters used the principal-subsumption rule that ComputeStackString now uses (implicitly using cx->compartment()->principals), would there still be a problem in the situation you described?

Said differently: are we ever trying to hide an origin from itself already being on the callstack?

Grepping all ScriptFrameIter uses (I've just been auditing all this for bug 980059), here are the observable cases I can think of:
 - f.arguments: we scan the stack for the innermost activation of f and get that activation's arguments.  In the sync-event-dispatch case you're talking about, that means we'd also search the callstack leading up to the sync-event-dispatch.  That seems fine.
 - We get the filename and lineno of the caller of eval/Function (to use for the eval/Function JSScript).  If those are attributed to the (same-domain) caller of the sync-event-dispatch, that seems fine.
 - Error.stack where it also seems like the user would enjoy seeing all their own domain's frames.

Other than that, there are a bunch of cases involving the Debugger, dumping code, JIT bailing code (which usually *must* see all frames), and cases where we know something special about our caller and this issue doesn't arise.

So if you agree on the basic points, this suggests changing ScriptFrameIter to *always* use cx->principals-subsumption-filtering and removing SaveFrameChain.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #2)
> Said differently: are we ever trying to hide an origin from itself already
> being on the callstack?

Quite possibly. It's really a spec issue. But I think the naive way to build this stuff involves resetting the JS engine to some degree when the UA calls into script (i.e. entry points).

I'm not worried about stacks, because (a) they're mostly a debugging tool and rarely (though occasionally) used programatically and (b) they don't leak references to actual script objects.

Other stuff seems much more dicey though - having f.caller reach through two cross-origin showModalDialog calls sort of creeps me out, and strikes me as something that other vendors might not want to implement. This is different from our normal cross-compartment calls because entry points are the only places in the spec where script execution might change origins. So UAs without sophisticated security membranes like Gecko might be very loathe to leak script objects across those barriers.

Then again, bz hacked some code back in bug 390488 to specifically make arguments.caller work across synchronous event dispatch. In this case, it only works the entry points are back-to-back. But if we go with the more conservative approach, we'd probably have to remove this feature.

Boris, what are your thoughts?

> Other than that, there are a bunch of cases involving the Debugger, dumping
> code, JIT bailing code (which usually *must* see all frames), and cases
> where we know something special about our caller and this issue doesn't
> arise.

That sounds like a weak guarantee, tbh. A lot of the cases may be benign, but it seems like a correctness bug (or worse) waiting to happen.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Boris, what are your thoughts?

We should test what other UAs actually do.

Comment 5

[Luke Wagner [:luke]]

(In reply to Bobby Holley (:bholley) from comment #3)
> (In reply to Luke Wagner [:luke] from comment #2)
> Quite possibly. It's really a spec issue. But I think the naive way to build
> this stuff involves resetting the JS engine to some degree when the UA calls
> into script (i.e. entry points).

Other than showModalDialog and corner cases involving alert, is there any other way to cause content-observable reentrance into content?  If it's just those two cases, then it seems like this isn't a spec question; for alert() we shouldn't be reentering content in the first place and showModalDialog is deprecated and sounds like it could actually be removed from Chrome/FF.

> That sounds like a weak guarantee, tbh. A lot of the cases may be benign,
> but it seems like a correctness bug (or worse) waiting to happen.

I'd of course do a slower and more careful audit before anything landed, of course, if that's what you mean by "weak guarantee", but I've already just performed a rather careful audit of all stack iteration for bug 980059 so I don't expect to find anything new.

Comment 7

[Bobby Holley (:bholley)]

I've written a quick test for the sync event dispatch behavior of other UAs in [1]. It looks like Blink and WebKit don't do any kind of frame chain saving in that case. I can't test IE, because I don't have a VM on my new machine. I'll rectify that on monday.

(In reply to Luke Wagner [:luke] from comment #5)
> > That sounds like a weak guarantee, tbh. A lot of the cases may be benign,
> > but it seems like a correctness bug (or worse) waiting to happen.
> 
> I'd of course do a slower and more careful audit before anything landed, of
> course, if that's what you mean by "weak guarantee"

I wasn't concerned about the thoroughness of your audit, but rather with the potential foot-gun-ness for new uses. But it sounds like other browsers fall more on the side of "one contiguous stack", so I could be persuaded to do that as well, so long as we built the principal-filtering into it by default.

[1] http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cscript%3E%0Afunction%20go%28%29%20{%0A%20%20document.body.setAttribute%28%27onclick%27%2C%20%22console.log%28%27same-document%20event%20handler%20caller%3A%20%27%20%2B%20arguments.callee.caller%29%3B%20console.log%28%27same-document%20event%20handler%20stack%3A%20%27%20%2B%20%28new%20Error%28%29%29.stack%29%3B%22%29%3B%0A%20%20window[0].document.body.setAttribute%28%27onclick%27%2C%20%22console.log%28%27cross-document%20event%20handler%20caller%3A%20%27%20%2B%20arguments.callee.caller%29%3B%20console.log%28%27cross-document%20event%20handler%20stack%20%27%20%2B%20%28new%20Error%28%29%29.stack%29%3B%22%29%3B%0A%20%20dispatchClick%28%29%3B%0A}%0Afunction%20dispatchClick%28%29%20{%0A%20%20document.body.dispatchEvent%28new%20MouseEvent%28%27click%27%29%29%3B%0A%20%20window[0].document.body.dispatchEvent%28new%20MouseEvent%28%27click%27%29%29%3B%0A}%0A%3C%2Fscript%3E%0A%3Cbody%3E%0A%3Ciframe%20onload%3D%22go%28%29%3B%22%3E

Comment 8

[Luke Wagner [:luke]]

I assume dispatchEvent does not spin a nested event loop?  Assuming that, it seems like, in the case of same-origin dispatchEvent, the user would expect/want to see the full callstack involving the callers of dispatchEvent.  Ignoring alert/showModalDialog, are there any cases where content can observe itself reentering via a nested event loop?

Comment 9

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #8)
> I assume dispatchEvent does not spin a nested event loop?

No. It does push a Script Entry point though.

> Assuming that, it
> seems like, in the case of same-origin dispatchEvent, the user would
> expect/want to see the full callstack involving the callers of
> dispatchEvent.

Given the conceptual implications of entry points, I don't know if that's obvious for f.caller, but I'm fine with that behavior since that's what other UAs do. 

> Ignoring alert/showModalDialog, are there any cases where
> content can observe itself reentering via a nested event loop?

In Gecko, sync XHR.

Comment 10

[Bobby Holley (:bholley)]

Currently frame chain saving happens during JSContext pushing. So if we want to get rid of pushing, we need to do this. Setting the dep.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Also in Gecko alert() and such.  And while the spec says that should not happen, I think that part of the spec is user-hostile and should change.

Comment 12

[Jim Blandy :jimb]

Here's a possible argument in favor of keeping saved frame chains, but have them serve a more limited role:

For debugging, we'd like to be able to ask, "Why did the browser start running this code in the first place? On whose behest are you executing?" In bug 961325, we're doing this by attaching metadata to particular Activations, which, outside the JS engine, you can read as attaching metadata to particular stack frames. The accessor for this metadata walks up the stack and returns the metadata from the oldest live frame that has any. We have an RAII-style class that provides a factory function for the metadata. (This ensures that it's cheap when the metadata is never requested.)

For that application, having event dispatch save the frame chain would be ideal, because there's no interesting causal connection between the thing that spun up a nested event loop and the event handlers that it calls.

However, if we could be sure to provide this dispatch metadata everywhere in Gecko where it makes sense, then that would probably cover every event handler dispatcher. Since younger execution reason providers "shadow" older ones, that might have the same effect as saving the frame chains, at least in terms of ensuring the older ones don't show through where they're inappropriate.

Comment 13

[Bobby Holley (:bholley)]

It seems like the information about why we're running script should really live on the script settings stack - in particular, on AutoEntryScript (see bug 989528). Long-term, we're going to set things up so that there _must_ be an AutoEntryScript at the top of the stack, or the JS engine will refuse to run script (see bug 991758).

We could make an "execution reason" a mandatory argument to the AutoEntryScript constructor, to make sure we always have it. This would be good mental discipline anyway, to make sure we're always clear on why we're doing it.

Comment 14

[Jim Blandy :jimb]

(In reply to Bobby Holley (:bholley) from comment #13)
> It seems like the information about why we're running script should really
> live on the script settings stack - in particular, on AutoEntryScript (see
> bug 989528). Long-term, we're going to set things up so that there _must_ be
> an AutoEntryScript at the top of the stack, or the JS engine will refuse to
> run script (see bug 991758).
> 
> We could make an "execution reason" a mandatory argument to the
> AutoEntryScript constructor, to make sure we always have it. This would be
> good mental discipline anyway, to make sure we're always clear on why we're
> doing it.

I don't understand the discussion in bug 989528 fully. But if there's a natural way to tie execution reasons to specific JSAPI calls --- as opposed to bug 961325's current approach of, essentially, temporarily establishing reasons that apply implicitly to all JS invocations that occur in their scope --- then that would be welcome.

We have two uses planned for execution reasons:

1) We want to count allocations per reason. A developer usually has an intuitive expectation of how expensive handling a given kind of event is; if we can surprise them, then we've been helpful.

2) We want to display execution reasons as part of the call stack. Often it's obvious from the handler function why it was called, but there's no advantage to making people infer these things; we have the data, so we should just tell them.

Comment 15

[Bobby Holley (:bholley)]

Yeah, I think both of those can be solved by full conversion to AutoEntryScript, which is well underway. That would let us make absolutely sure that we always have a reason.

Can you file a bug for putting a mandatory execution reason on the AutoEntryScript constructor? Or maybe just morph bug 961325 to do that? I think that's definitely the way to go.

Comment 16

[Jim Blandy :jimb]

Having read more of this cluster of bugs, I don't think the saved frame chain is so important to debugging any more. The Debugger's view of the stack is produced with a GO_THROUGH_SAVED ScriptFrameIter, so the saved frame chain operations don't really affect us.

Comment 17

[Bobby Holley (:bholley)]

Yeah. The things the debugger will care about will be entry points, which we're adding as a separate concept in bug 1006291.

Comment 18

[Shu-yu Guo [:shu]]

I can probably get to this before Luke can. Setting NI as a reminder.

Comment 19

[Jan de Mooij [:jandem]]

Clearing NI - bug 1274193 and bug 1274915 are taking care of this.

Comment 20

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Remove SavedOption from frame iters

Nobody is passing STOP_AT_SAVED anymore, so this is a no-op.

Comment 21

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Remove saveFrameChain evaluate option

The shell's evaluate function has 'saveFrameChain' option that we can remove. This also fixes up the jit-tests that used it.

Comment 22

[Jan de Mooij [:jandem]]

Attachment: Part 3 - testChromeBuffer jsapi test

Remove saved frame stuff from the testChromeBuffer.cpp jsapi test.

Comment 23

[Jan de Mooij [:jandem]]

Attachment: Part 4 - JS_IsRunning can ignore saved frames

The only place where Gecko code (that uses saved frame chains) can call JS_IsRunning is PreciseGCRunnable::Run, and that caller doesn't care about saved frame chains.

JS_IsRunning and cx->currentlyRunning() are weird anyway, we can remove/simplify them soon.

Comment 24

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Remove saved frame chains

We can land this after bz lands his patch to remove the JS_SaveFrameChain call in XPConnect.

Comment 25

[Luke Wagner [:luke]]

Comment on attachment 8757171 [details] [diff] [review]
Part 1 - Remove SavedOption from frame iters

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

\o/

Comment 26

[Boris Zbarsky [:bzbarsky]]

> We can land this after bz lands his patch to remove the JS_SaveFrameChain call in XPConnect.

It's on inbound.  ;)

Comment 27

[Luke Wagner [:luke]]

Comment on attachment 8757187 [details] [diff] [review]
Part 5 - Remove saved frame chains

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

\o/

::: js/src/jsapi.cpp
@@ -5010,5 @@
>      return cx->currentlyRunning();
>  }
>  
> -JS_PUBLIC_API(bool)
> -JS_SaveFrameChain(JSContext* cx)

Also remove from jsapi.h?

Comment 28

[Jan de Mooij [:jandem]]

Thanks for the fast reviews.

(In reply to Luke Wagner [:luke] from comment #27)
> Also remove from jsapi.h?

D'oh, good catch.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2f18efe44a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d835e1ad4222
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a9f082d0ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4e2f8a9df18
https://hg.mozilla.org/integration/mozilla-inbound/rev/29e2af4247e1

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d2f18efe44a3
https://hg.mozilla.org/mozilla-central/rev/d835e1ad4222
https://hg.mozilla.org/mozilla-central/rev/b1a9f082d0ae
https://hg.mozilla.org/mozilla-central/rev/f4e2f8a9df18
https://hg.mozilla.org/mozilla-central/rev/29e2af4247e1