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.

This bug could use a much better summary. ;)

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

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

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.

(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!

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 :)

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

(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?

(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.

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'.

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

(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?

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].

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.

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.

(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.)

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.

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.

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

DOMException is supposed to become constructible per spec, yes.

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

(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?

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.

(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

(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.