Remove saved frame chains

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: bholley, Assigned: jandem)

Tracking

(Depends on 1 bug)

unspecified
mozilla49
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(5 attachments)

Reporter

Description

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

Updated

5 years ago
Depends on: 979926
Reporter

Updated

5 years ago
Blocks: 971673
Reporter

Comment 1

5 years ago
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?
Reporter

Updated

5 years ago
Flags: needinfo?(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.
Flags: needinfo?(luke)
Reporter

Comment 3

5 years ago
(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.
Flags: needinfo?(bzbarsky)
> Boris, what are your thoughts?

We should test what other UAs actually do.
Flags: needinfo?(bzbarsky)
(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.
Reporter

Comment 7

5 years ago
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
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?
Reporter

Comment 9

5 years ago
(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.
Reporter

Updated

5 years ago
Depends on: 981187
Reporter

Comment 10

5 years ago
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.
Blocks: 767938
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

5 years ago
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.
Reporter

Comment 13

5 years ago
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

5 years ago
(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.
Reporter

Comment 15

5 years ago
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.
Reporter

Updated

5 years ago
No longer blocks: 971673

Comment 16

5 years ago
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.
Reporter

Comment 17

5 years ago
Yeah. The things the debugger will care about will be entry points, which we're adding as a separate concept in bug 1006291.
I can probably get to this before Luke can. Setting NI as a reminder.
Flags: needinfo?(shu)
Assignee

Updated

3 years ago
Depends on: 1274193
Assignee

Updated

3 years ago
Depends on: 981201
Assignee

Comment 19

3 years ago
Clearing NI - bug 1274193 and bug 1274915 are taking care of this.
Flags: needinfo?(shu)
Assignee

Comment 20

3 years ago
Nobody is passing STOP_AT_SAVED anymore, so this is a no-op.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8757171 - Flags: review?(luke)
Assignee

Comment 21

3 years ago
The shell's evaluate function has 'saveFrameChain' option that we can remove. This also fixes up the jit-tests that used it.
Attachment #8757173 - Flags: review?(luke)
Assignee

Comment 22

3 years ago
Remove saved frame stuff from the testChromeBuffer.cpp jsapi test.
Attachment #8757174 - Flags: review?(luke)
Assignee

Comment 23

3 years ago
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.
Attachment #8757182 - Flags: review?(luke)
Assignee

Comment 24

3 years ago
We can land this after bz lands his patch to remove the JS_SaveFrameChain call in XPConnect.
Attachment #8757187 - Flags: review?(luke)
Comment on attachment 8757171 [details] [diff] [review]
Part 1 - Remove SavedOption from frame iters

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

\o/
Attachment #8757171 - Flags: review?(luke) → review+

Updated

3 years ago
Attachment #8757173 - Flags: review?(luke) → review+

Updated

3 years ago
Attachment #8757174 - Flags: review?(luke) → review+
> We can land this after bz lands his patch to remove the JS_SaveFrameChain call in XPConnect.

It's on inbound.  ;)

Updated

3 years ago
Attachment #8757182 - Flags: review?(luke) → review+
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?
Attachment #8757187 - Flags: review?(luke) → review+
Assignee

Comment 28

3 years ago
Thanks for the fast reviews.

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

D'oh, good catch.
You need to log in before you can comment on or make changes to this bug.