960108 - Make JS::DescribeStack walk the entire runtime stack

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrea Marchesini [:baku]]

Bz suggested to split part of the patch for bug 629607 in order to check if it doesn't break code before landing the rest.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: stack.patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

This bug could use a much better summary.  ;)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8360469 [details] [diff] [review]
stack.patch

And this could use a _much_ better commit message.  Please fix it to actually describe what's being changed (e.g. now you're iterating the entire runtime's stack, across the event loop and whatnot, not just the one for the incoming JSContext).

r=me

Comment 4

[Andrea Marchesini [:baku]]

Attachment: stack.patch

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b96ae9f43a89

https://tbpl.mozilla.org/php/getParsedLog.php?id=33104059&tree=Mozilla-Inbound

Comment 7

[Andrea Marchesini [:baku]]

msucan, can you tell me what you think about this orange test?
With this patch we expose all the stack trace. Also internal methods are shown.
If we want to have the same behaviour, we have to filter out what is chrome only code.

Comment 8

[Mihai Sucan [:msucan]]

(In reply to Andrea Marchesini (:baku) from comment #7)
> msucan, can you tell me what you think about this orange test?
> With this patch we expose all the stack trace. Also internal methods are
> shown.
> If we want to have the same behaviour, we have to filter out what is chrome
> only code.

That's really nice to see the chrome stack as well, but this is not ideal for web developers. We should filter out chrome code. Can you please do this?

Is it possible to have a preference that we can enable, or an option for Components.stack()? It would be really useful to show chrome code in stack traces when browser.devtools.chrome=true.

Thanks!

Comment 9

[Andrea Marchesini [:baku]]

I need info about this. bholley, can you tell me how to know if the current stack frame is chrome or content?
I'm not expert of this code, but it doesn't require to change the whole world I'm happy to do it :)

Comment 10

[Bobby Holley (:bholley)]

Hm.

So it looks like there are various places in the tree (including the devtools stuff here) that depend pretty precisely on the current behavior of Components.stack (including Promise.jsm and whatnot). This is kind of worrisome, because these are all chrome callers, which means that the new behavior would give them a full unfiltered stack (since the System Principal subsumes everything), even if they're being called by (and returning information to) untrusted code.

We could go through and update/audit them, but that's probably a lot of work. Here's what I propose:

(1) Make JS::DescribeStack take an enum StopAtFrameChainBoundary/DontStopAtFrameChainBoundary, and pass this param all the way down through the call chain from nsXPCComponents::GetStack to JS::DescribeStack. The current nsXPCComponents::GetStack can pass StopAtFrameChainBoundary to keep the current behavior.

(2) Introduce a new API: Components.utils.getStackForScope(jsval scope); We call js::UncheckedUnwrap on the object that's passed on, enter its compartment, and then invoke nsXPConnect::GetCurrentJSStack(DontStopAtFrameChainBoundary).

(3) File bugs on devtools and the various consumers to switch to the new API, which will let them get rid of all of the crap where they filter out the frames for the code that actually invokes Components.stack (see for example [1]). They can just pass in the global for the content they're servicing, and the stack frames will be filtered appropriately for its viewing pleasure.

Does that sound reasonable to everyone?

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Promise.jsm#220

Comment 11

[Mihai Sucan [:msucan]]

(In reply to Bobby Holley (:bholley) from comment #10)
> Hm.
> 
> So it looks like there are various places in the tree (including the
> devtools stuff here) that depend pretty precisely on the current behavior of
> Components.stack (including Promise.jsm and whatnot). This is kind of
> worrisome, because these are all chrome callers, which means that the new
> behavior would give them a full unfiltered stack (since the System Principal
> subsumes everything), even if they're being called by (and returning
> information to) untrusted code.
> 
> We could go through and update/audit them, but that's probably a lot of
> work. Here's what I propose:
> 
> ...
> 
> Does that sound reasonable to everyone?

Sounds good to me. Then the ConsoleAPI.js code can invoke Cu.getStackForScope(contentWindow) to get the stackframes specific for the scripts in the given contentWindow? Does it include stackframes from scripts in iframes of the same domain?

Comment 12

[Bobby Holley (:bholley)]

(In reply to Mihai Sucan [:msucan] from comment #11)
> Sounds good to me. Then the ConsoleAPI.js code can invoke
> Cu.getStackForScope(contentWindow) to get the stackframes specific for the
> scripts in the given contentWindow? Does it include stackframes from scripts
> in iframes of the same domain?

It gets the stack frames _as visible for_ a given content window. It basically takes the entire stack, and filters out the things that the content's principal does not subsume. So anything same-origin will appear.

Comment 14

[Luke Wagner [:luke]]

Note: won't DescribeStack still stop when it reaches hidden (this is hard-coded in JS::DescribeStack)?  This relates to the general question of what does 'hidden' vs 'saved' actually mean.  It'd be nice to understand: what does 'saved' mean such that we want JS::DescribeStack to GO_THROUGH_SAVED but still stop at 'hidden'.

Comment 15

[Luke Wagner [:luke]]

Oh, well in light of bug 979730, I guess the difference is "saved is going away".

Comment 16

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #14)
> Note: won't DescribeStack still stop when it reaches hidden (this is
> hard-coded in JS::DescribeStack)?  This relates to the general question of
> what does 'hidden' vs 'saved' actually mean.  It'd be nice to understand:
> what does 'saved' mean such that we want JS::DescribeStack to
> GO_THROUGH_SAVED but still stop at 'hidden'.

It is? I don't see it. Where does that happen?

Comment 17

[Luke Wagner [:luke]]

Ugh, I'm confusing DescribeStack with DescribeScriptedCaller.  More evidence we need the split in bug 979926 comment 1.  Logically, one would expect DescribeScriptedCaller to be DescribeStack[0].

Comment 18

[Bobby Holley (:bholley)]

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

Comment 19

[Bobby Holley (:bholley)]

This looks green now, presumably because the Console API went to C++. I'm going to do a quick audit of the Components.stack users, to see if any of them expose the result to script. If not, this can go up for review.

Comment 20

[Bobby Holley (:bholley)]

Ok, so I've done the audit. In general, the consumers here just take the results of Component.stack and write them to stdout (with dump()) or to the console, neither of which are visible to untrusted script.

The one question mark I have is Promise.jsm, which does some heavy stack munging, the results of which escape in non-obvious ways.

nsm, is there any danger that the result of the Components.stack stuff in Promise.jsm would end up in the hands of untrusted script? I'm guessing not, given that untrusted code should only be using DOM Promises. but I want to double-check.

Comment 21

[Luke Wagner [:luke]]

(Also: for such a high-use module, is there an alternative to using Components.stack?  That is a pretty slow operation and it also, iirc, arbitrarily cuts off the stack at some depth limit so possibly dangerous, depending on how it's being used.)

Comment 22

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

punting both comment 20 and 21 to dteller who seems to have written much of it.

bholley, I can't think of a obvious way in which content code could use Promise.jsm, so no, unlikely to be an issue.

Comment 23

[Bobby Holley (:bholley)]

The other concern I had is with the stacktraces of Exceptions that use this stuff (i.e. DOMException).

If an Xray caller does |new contentWindow.DOMException(..);|, the principal filter used during script frame iteration will be System, so it will get an unfiltered stack. However, the DOMException will live in the content scope, meaning that content can access it once the stack frame have been created. This isn't an issue for |new contentWindow.TypeError()|, even over Xrays, because we have to enter the content compartment at some point to create the TypeError object. But it _is_ a potential problem for DOMException.

Looking at the WebIDL though, it appears DOMError doesn't have a |stack| property, and the |stack| property on DOMException is [ChromeOnly]. So I guess this is ok.

Comment 24

[Bobby Holley (:bholley)]

Also, it looks like DOMException isn't constructable, which is double defense. Unless that will change someday?

Comment 25

[Bobby Holley (:bholley)]

Attachment: Ignore saved frame chains and contexts in JS::DescribeStack. v1

Comment 26

[Boris Zbarsky [:bzbarsky]]

DOMException is supposed to become constructible per spec, yes.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8386493 [details] [diff] [review]
Ignore saved frame chains and contexts in JS::DescribeStack. v1

r=me

Comment 28

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #26)
> DOMException is supposed to become constructible per spec, yes.

Ok. But it'll never have a .stack property?

Comment 29

[Boris Zbarsky [:bzbarsky]]

Per spec it should.

But note that it will stop being a WebIDL object in the process of getting there, and just become a JSObject with a custom class, like Error.  For whatever that's worth.

Comment 30

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #29)
> Per spec it should.
> 
> But note that it will stop being a WebIDL object in the process of getting
> there, and just become a JSObject with a custom class, like Error.  For
> whatever that's worth.

Hm. At that point presumably the .stack will come from the JS engine machinery, and this will not be a problem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8adacb553312

Comment 31

[David Teller [:Yoric] - still alive but not very active]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #22)
> punting both comment 20 and 21 to dteller who seems to have written much of
> it.
> 
> bholley, I can't think of a obvious way in which content code could use
> Promise.jsm, so no, unlikely to be an issue.

Agreed. Promise.jsm has not been hardened for use by content code and may already expose some internal information for the sake of debugging. If we ever find out that Promise.jsm is used by content code, that's a (possibly security) bug.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8adacb553312