1137910 - Make C++ SavedFrame accessors enter the object compartment if the cx compartment subsumes it

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This came up in bug 1122238.

Consider a situation in which chrome code is working with a C++ object that has a stack stashed inside (a DOMException, or nsIStackFrame directly) and accesses it via the C++ API from bug 1031152.  What should happen when it examines that stack?

Right now what happens is exactly what would happen if the caller were touching the SavedFrame object from JS: the context compartment is whatever the caller is, the object compartment of the SavedFrame is ignored, and we get the "xray" view of the stack, which is the whole stack.

But that's not actually desirable in various cases (e.g. console.trace() starts showing chrome frames if they happened to be on the stack when trace() got called!).  And unlike working with SavedFrame in JS, where you can waive Xrays to get the unprivileged view, in this case waiving Xrays means you might not even be calling the right .stack getter.

Nick suggested that the right behavior here is to show the full stack if devtools.chrome.enabled but to show the content view of the stack otherwise.  My proposed implementation of the latter is that the C++ accessors in SavedFrame should enter the compartment of the object (whatever object was passed in, not the underlying SavedFrame) if that's subsumed by the context compartment.  This won't affect access from JS, because in those cases the object and context are already same-compartment.

Comment 1

[Bobby Holley (:bholley)]

(In reply to Not doing reviews right now from comment #0)
> But that's not actually desirable in various cases (e.g. console.trace()
> starts showing chrome frames if they happened to be on the stack when
> trace() got called!).  And unlike working with SavedFrame in JS, where you
> can waive Xrays to get the unprivileged view, in this case waiving Xrays
> means you might not even be calling the right .stack getter.

I don't understand this. When using the C++ API, we're not invoking JSFunctions at all right?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Sure.

Here's a concrete example.  Say you have chrome code that wants to get the stack of a content DOMException and show it to the web developer (i.e. devtools).  It clearly wants the stack as seen by content.

How should it get it?  Well, it doesn't want to waive Xrays on the DOMException, because then .stack will be under the control of the web page.  But if it calls .stack over Xrays, then we'll call the C++ API with cx in the chrome compartment but the HandleObject argument from the content compartment.  That's the situation that can't arise when calling JSFunctions on a StackFrame from script, and the situation in which I propose we enter the content compartment before operating on the StackFrame.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So one issue with basing this stuff on devtools.chrome.enabled is that afaict it's set on aurora (because of MOZ_DEV_EDITION).  So the tests that test the "normal" behavior will fail on aurora.  Worse yet, webdevs using the developer edition builds will see weird stacks...

Maybe we should just make the behavior unconditional.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Not doing reviews right now from comment #2)
> That's the situation that can't arise when calling JSFunctions
> on a StackFrame from script

Why not? Don't we have the same problem whereby waiving Xrays means that we can't trust the getters on the other side?

For DOMException, it seems like we could have a separate ChromeOnly getter to get the stack as-viewed by content. For the ES objects the story is a bit trickier, but we could manually stick something similar in on the Xray layer, or add something to Cu.

I'm kind of wary of the pref thing. It seems like using a feature-oriented pref to dictate basic platform behavior is crossing abstraction layers in a way that might bite us later.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Don't we have the same problem whereby waiving Xrays means that we can't trust the
> getters on the other side?

No, because SavedFrame objects are frozen after being created, as is the SavedFrame prototype.

This means you can't change their proto chain, can't remove properties from their proto, can't redefine properties on the proto, and can't add properties that would shadow the proto.

> For DOMException, it seems like we could have a separate ChromeOnly getter to get the
> stack as-viewed by content.

We could, and we could then make sure that everyone always used it (good luck!).

But that wouldn't help with console.trace() as things stand, right?  Though we could of course also duplicate all the accessors on nsIStackFrame or something...

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK.  There's one test failure: js/xpconnect/tests/unit/test_xray_SavedFrame.js which is claiming that xrays should be able to see the whole stack, but with my patch they do not.  Xray are affected because I misread what the accessors on SavedFrame do: they do THIS_SAVEDFRAME, which does checkThis(), which will go ahead and unwrap wrappers and do GetFirstSubsumedFrame.

Then, of course, the API getter they invoke tries to UnwrapSavedFrame, which would do the CheckedUnwrap and GetFirstSubsumedFrame all over. 

So I can fix the behavior by changing around what checkThis() does a bit to just check that our "this" is a SavedFrame or unwrappable wrapper for it without doing any unwrapping.   Nick, does that seem reasonable?

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Not doing reviews right now from comment #3)
> So one issue with basing this stuff on devtools.chrome.enabled is that
> afaict it's set on aurora (because of MOZ_DEV_EDITION).  So the tests that
> test the "normal" behavior will fail on aurora.  Worse yet, webdevs using
> the developer edition builds will see weird stacks...
> 
> Maybe we should just make the behavior unconditional.

I think maybe I switched my prefs around. There also exists `devtools.debugger.chrome-enabled` which might be what we are actually looking for. Trying to find someone who knows...

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

FWIW, +1 for just landing this behavior all the time. We can follow up with changing behavior based on a pref.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> There also exists `devtools.debugger.chrome-enabled` 

This is always true in Firefox, afaict, so I doubt it's what we want here.  ;)

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

/me finishes reading the other new comments

Ignore that other pref -- I think it is left over from when we didn't have a full chrome toolbox, but just a chrome debugger.

(In reply to Not doing reviews right now from comment #6)
> OK.  There's one test failure:
> js/xpconnect/tests/unit/test_xray_SavedFrame.js which is claiming that xrays
> should be able to see the whole stack, but with my patch they do not.  Xray
> are affected because I misread what the accessors on SavedFrame do: they do
> THIS_SAVEDFRAME, which does checkThis(), which will go ahead and unwrap
> wrappers and do GetFirstSubsumedFrame.
> 
> Then, of course, the API getter they invoke tries to UnwrapSavedFrame, which
> would do the CheckedUnwrap and GetFirstSubsumedFrame all over. 
> 
> So I can fix the behavior by changing around what checkThis() does a bit to
> just check that our "this" is a SavedFrame or unwrappable wrapper for it
> without doing any unwrapping.   Nick, does that seem reasonable?

So the SavedFrame accessors wouldn't unwrap, but let the JSAPI methods do the unwrapping? In this way, we don't mess up the subsumes checks that you're adding to decide whether to give the content view or not?

If I'm understanding correctly, this sounds good to me.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Don't lose track of the original 'this' object in THIS_SAVEDFRAME, so we can actually do things based on the principal of the object we're working with

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Give chrome callers that are indirectly (e.g. via nsIStackFrame) poking at content-captured stacks the content view of the stack

Comment 13

[Boris Zbarsky [:bzbarsky]]

If filed bug 1138593 on figuring out what to do with the pref bits.

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8571498 [details] [diff] [review]
part 1.  Don't lose track of the original 'this' object in THIS_SAVEDFRAME, so we can actually do things based on the principal of the object we're working with

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

Great!

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8571499 [details] [diff] [review]
part 2.  Give chrome callers that are indirectly (e.g. via nsIStackFrame) poking at content-captured stacks the content view of the stack

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

::: js/src/vm/SavedStacks.cpp
@@ +397,5 @@
> +namespace {
> +
> +// It's possible that our caller is privileged (and hence would see the
> +// entire stack) but we're working with an SavedFrame object that was
> +// captured in unprivileged code.  If so, drop privileges down to its level.

This explains the _what_ very well, but we might mention the _why_ as well. Something like "devtools code has chrome privileges, but wishes to display the stack to the web developer without Firefox implementation details, as they would see it in Error.prototype.stack" or something.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Fixed up the comments and:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d359ff807ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/5816dfd514af

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/5d359ff807ef
https://hg.mozilla.org/mozilla-central/rev/5816dfd514af