960820 - Make jsexn's ComputeStackString context- and savedFrameChain-agnostic

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

In bug 957688, I'm replacing an old-style CAPS check for stack boundaries with a principal filter, using the machinery implemented in bug 924905.

Once this is landed and sorted out, we should then make this stuff ignore contexts and frame chains, since the principal will filter everything appropriately.

Comment 1

[Luke Wagner [:luke]]

One step closer to a context-free world?

Comment 2

[Bobby Holley (:bholley)]

It's somewhat orthogonal. In general, I don't know if we even need the context filtering, because we should always have a saved frame chain boundary whenever we switch JSContexts. And it's possible that saved frame chain boundaries will outlives JSContexts (though their days are hopefully numbered as well).

Comment 3

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=fa0bcec48cf4

Comment 4

[Bobby Holley (:bholley)]

Attachment: Ignore contexts and saved frame chains for exception stack, and rely on the principal check. v2

It would be nice to update the JS shell tests to make sure the principal check
works, but we can't do that without bug 969273. So we add an XPConnect test that
verifies the filtering behavior.

This is a big change, so I'm requesting some heavyweight r/sr.

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8383824 [details] [diff] [review]
Ignore contexts and saved frame chains for exception stack, and rely on the principal check. v2

Hm, looks like I can only flag one superreviewer. Switching mrbkap to r?

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8383824 [details] [diff] [review]
Ignore contexts and saved frame chains for exception stack, and rely on the principal check. v2

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

::: js/src/jsexn.cpp
@@ +210,5 @@
>      {
>          RootedAtom atom(cx);
>          SuppressErrorsGuard seg(cx);
>          // We should get rid of the CURRENT_CONTEXT and STOP_AT_SAVED here.
>          // See bug 960820.

You can remove this comment now.

@@ +212,5 @@
>          SuppressErrorsGuard seg(cx);
>          // We should get rid of the CURRENT_CONTEXT and STOP_AT_SAVED here.
>          // See bug 960820.
> +        for (NonBuiltinScriptFrameIter i(cx, ScriptFrameIter::ALL_CONTEXTS,
> +                                         ScriptFrameIter::GO_THROUGH_SAVED,

So, IIUC, the reason why we want to GO_THROUGH_SAVED is that, since we're principal filtering, there is no reason we have in the browser to hide a domain from itself.

That leads to the question: when *should* we STOP_AT_SAVED?  Clearly, if we don't have a principal, but when don't we have a principal?  Sometimes we walk the entire stack for internal reasons, but then usually we want to see *everything*.  So, I guess what I'm asking is: can we remove JS_SaveFrameChain and, if not, what meaning does it have?  (Related, it's weird we now have both "saved" and "hidden" flags.)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8383824 [details] [diff] [review]
Ignore contexts and saved frame chains for exception stack, and rely on the principal check. v2

sr=me, but we need to figure out what the story is with DescribeStack

Comment 8

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #6)
> So, IIUC, the reason why we want to GO_THROUGH_SAVED is that, since we're
> principal filtering, there is no reason we have in the browser to hide a
> domain from itself.
> 
> That leads to the question: when *should* we STOP_AT_SAVED?  Clearly, if we
> don't have a principal, but when don't we have a principal?  Sometimes we
> walk the entire stack for internal reasons, but then usually we want to see
> *everything*.  So, I guess what I'm asking is: can we remove
> JS_SaveFrameChain and, if not, what meaning does it have?  (Related, it's
> weird we now have both "saved" and "hidden" flags.)

JS_SaveFrameChain has two primary uses that I know of:

(1) Halting stack frame iteration.
(2) Hiding scripted callers.

This bug removes one of the dependencies of (1), but not all of them. In particular, we still need to fix bug 960108, which I'll take a look at next.

I think we can fix (2) by just including an AutoHideScriptedCaller in AutoEntryScript.

Comment 9

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=53b8aa8fa7ec

Comment 10

[Luke Wagner [:luke]]

(In reply to Bobby Holley (:bholley) from comment #8)
> (1) Halting stack frame iteration.
> (2) Hiding scripted callers.
> 
> This bug removes one of the dependencies of (1), but not all of them. In
> particular, we still need to fix bug 960108, which I'll take a look at next.
> 
> I think we can fix (2) by just including an AutoHideScriptedCaller in
> AutoEntryScript.

If we HideScriptedCaller at all the same points we JS_SaveFrameChain, then can we just make hiding stop frame iteration (making DescribeFrameChain not be a special-case) and collapse "hiding" and "saved" into one thing?

Comment 11

[Boris Zbarsky [:bzbarsky]]

JS_SaveFrameChain affects not only frame iteration but also JS_IsRunning().  And we rely on that, because if JS_IsRunning the JS engine drops exceptions on the floor when you try to report them.  :(

Comment 12

[Luke Wagner [:luke]]

Oh, just saw bug 979730.

It seems like we'd want hiding a frame chain to also affect JS_IsRunning (seems like bug 979730 would have to).  On a side note, it looks like JS_IsRunning has very few callers (probably because it was always a rather poorly-defined and insufficient concept (e.g., it only says 'true' if you have a scripted frame and not if, e.g., if you setTimeout'd a native) and CurrentGlobalOrNull et al are much more meaningful); I wonder if we should just remove it and replace the few stragglers with a jsfriendapi (there already is one.

Lastly, could you explain more about how IsRunning affects JS engine error reporting?  I stepped through the ReportError path and it seems to convert reports into exceptions if JS is running, but the path is long and windy so I could have missed something.

Comment 13

[Bobby Holley (:bholley)]

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

Comment 14

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #10)
> If we HideScriptedCaller at all the same points we JS_SaveFrameChain, then
> can we just make hiding stop frame iteration (making DescribeFrameChain not
> be a special-case) and collapse "hiding" and "saved" into one thing?

Thinking about this a bit, I think this conflates two overlapping (but distinct) concepts. This means that I was mistaken in bug 978618 comment 27, but thankfully Boris ended up doing something different anyway.

Saving a frame chain means that, for the scope of the call, the engine should pretend that all JS execution outside the scope of the call did not exist, i.e. the engine is in an idle state (though there are some consumers, like rooting, and now Error.stack, that want to break through this abstraction). This effectively maps to JS_IsRunning(cx).

Hiding the scripted caller was supposed to indicate an override. It was in fact called 'override', until luke made me rename it in bug 937317 comment 49, which upon reflection I should have pushed back on, because it leads to precisely the kind of confusion we have here. The intention is not to say "there is no script running", but rather that, for introspection purposes, the most recent frame is handled by the embedding, which in our case maps to the 'incumbent script settings object'.

I have some ideas on how to clear this up. Let me get a bug on file.

Comment 15

[Luke Wagner [:luke]]

(In reply to Bobby Holley (:bholley) from comment #14)
> Saving a frame chain means that, for the scope of the call, the engine
> should pretend that all JS execution outside the scope of the call did not
> exist, i.e. the engine is in an idle state (though there are some consumers,
> like rooting, and now Error.stack, that want to break through this
> abstraction). This effectively maps to JS_IsRunning(cx).

As I was saying in comment 12, JS_IsRunning is a very creepy function because it's not clear what it means to who.  I'd like to remove it.  We need more semantically meaningful queries that we can actually answer in terms of spec concepts as has been done with 'incumbent script's global'.

> Hiding the scripted caller was supposed to indicate an override. It was in
> fact called 'override', until luke made me rename it in bug 937317 comment
> 49, which upon reflection I should have pushed back on, because it leads to
> precisely the kind of confusion we have here.

Heh, I don't think naming it 'override' would have changed this discussion other than to s/hidden/overriden/.  Taking 'hidden' as short for 'hidden from DescribeScriptedCaller' actually means something, but, then again, it's not clear what DescribeScriptedCaller actually means: sometimes it's just a best-effort stack-walk, sometimes it has hard spec-defined meaning (although did GetScriptedCallerGlobal() steal the one spec-defined use of DescribeScriptedCaller.

> The intention is not to say
> "there is no script running", but rather that, for introspection purposes,
> the most recent frame is handled by the embedding, which in our case maps to
> the 'incumbent script settings object'.

Perhaps we should have separate functions that specifically mention 'incumbent script settings object' (btw, 'settings object'?  what happened to 'global'?).  Then we can have a looser "best effort stack walk" that is used by all the random places where we just want to show the full stack (modulo principal subsumption).

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Lastly, could you explain more about how IsRunning affects JS engine error reporting?

Sure.  We call JS_ReportPendingException.  This eventually ends up in ReportError, via js_ReportUncaughtException calling JS_ReportErrorNumber, etc.  If JS_IsRunning, then as you note ReportError doesn't actually report the error, even though we explicitly asked for it to be reported.  Instead it generates a new exception and sets it on the JSContext.  Then the stack unwinds back to js_ReportUncaughtException which does a JS_ClearPendingException(cx) at the end there.  Voila, exception dropped on the floor.

And yes, "long and windy" is a good description for this stuff.  For example, if the exn on the JSContext were actually an Error object, or something that IsDuckTypedErrorObject returned true for, then js_ReportUncaughtException would just call the error reporter and be done with it.  But in this particular case a string was thrown, so reportp ends up null and we take the JS_ReportErrorNumber path, which just happens to not always report the exception.  :(

Comment 17

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #15)
> As I was saying in comment 12, JS_IsRunning is a very creepy function
> because it's not clear what it means to who.  I'd like to remove it.  We
> need more semantically meaningful queries that we can actually answer in
> terms of spec concepts as has been done with 'incumbent script's global'.

I'm fine with that in the long term, but at present it does have meaning, at least within our platform. See comment 16.

> > Hiding the scripted caller was supposed to indicate an override. It was in
> > fact called 'override', until luke made me rename it in bug 937317 comment
> > 49, which upon reflection I should have pushed back on, because it leads to
> > precisely the kind of confusion we have here.
> 
> Heh, I don't think naming it 'override' would have changed this discussion
> other than to s/hidden/overriden/.

It would have, at least in bug 978618 comment 27, because it makes it clearer that the embedding is supposed to set some corresponding state. I grant that, in terms of its present functionality, 'hide' is a reasonable name. But it caused me to momentarily forget the original intention of that machinery, and use it for something else.

> it's
> not clear what DescribeScriptedCaller actually means: sometimes it's just a
> best-effort stack-walk, sometimes it has hard spec-defined meaning (although
> did GetScriptedCallerGlobal() steal the one spec-defined use of
> DescribeScriptedCaller.

When I'm done with bug 979926, there will only be one call to DescribeScriptedCaller in Gecko.

> Perhaps we should have separate functions that specifically mention
> 'incumbent script settings object'

With bug 979926, everything will go through that concept.

> (btw, 'settings object'?  what happened to 'global'?).

Spec term vs gecko term.

>  Then we can have a looser "best effort stack walk" that is
> used by all the random places where we just want to show the full stack
> (modulo principal subsumption).

DescribeScriptedCaller isn't about showing a stack. It's about getting information on the most recent stack frame.

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/b6d23f380d7d