1122238 - Remove the DOMException-cloning hackery we do right now

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Right now, we clone DOMExceptions in various cases when chrome created the DOMException and we're returning it to content.  See bug 1107592 and bug 1107953 comment 17 and the discussion that ensued in bug 1107953.

This cloning is basically working around bug 1117242 and the fact that DOMException will cache stuff.  I believe once that's fixed we can rip all the cloning bits out.  That includes DOMException::Sanitize, probably StackFrame/JSStackFrame::CallerSubsumes, the compartment-entering bits we do in Promise code right now that were added in attachment 8543478 [details] [diff] [review], the comments about the compartment of the cx being meaningful in StealJSException.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1. Pass in a JSContext to various DOMException and nsIStackFrame getters

Bobby, I actually have this locally as a stack of five patches, doing a property or two at a time.  Let me know if you'd prefer to review it that way.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Stop caching things in JSStackFrame when we're called over Xrays

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Drop all the DOMException-cloning gunk we added in bug 1107592 and bug 1107953

I've verified that parts 0-2 from bug 1117242 plus these patches still pass our tests for not exposing the JS-impleemnted webidl stack bits to content

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 even building on Linux

Comment 5

[Boris Zbarsky [:bzbarsky]]

A try run with the above patches ran into an exciting problem.

TCPSocket is still on XPConnect bindings, not Web IDL ones.  Which means the errors it throws are turned into Exception, not DOMException.  

These Exceptions are allocated via the CheckForException call in nsXPCWrappedJSClass::CallMethod while we're in the compartment of the method being called (in this case system code).  So the stack they capture will have its StackFrame objects in the chrome compartment (even though the stack it's capturing no longer includes the chrome code that was just called).  Then the Exception object gets a content-side reflector.

The upshot is that without these patches if you set dom.mozTCPSocket.enabled to true and do:

  navigator.mozTCPSocket.open(1, 2)

you get an exception whose various methods/getters (toString, filename, lineNumber, etc) try to get the filename and line number from the  the (chrome-compartment) stack.  This succeeds, because we currently enter the mStack compartment before touching it.

With the above patches, what happens instead is that the attempt to touch the chrome-compartment stack throws (but we don't propagate the exception up to the caller to hit fatal assertions about pending exceptions when they should not be there, etc).

Anyway, the question is which behavior we want for this case.  Obvious options:

1)  For the specific case of Exception, not DOMException, enter the compartment of the
    captured stack.
2)  For the specific case of Exceptions allocated from CheckForException, enter the
    compartment of the captured stack.
3)  Throw from the various methods/getters on the Exception in this case.  This sucks for
    authors who have to deal with these untouchable exceptions.
4)  Suppress the exception and return empty string or 0 or whatnot from whatever functions
    would otherwise end up throwing.  Still kinda sucks.
5)  Do something to change which compartment the stack is captured in (e.g. exit the
    autocompartment in CallMethod before calling CheckForException, assuming that
    CheckForException can deal with the cx not being in any compartment in the cases when
    we have no JS on the stack above us).

I'm tempted by #5 if we can make it work.  Thoughts?

Comment 6

[Boris Zbarsky [:bzbarsky]]

So I kinda tried #5, and the problem is that this cx is not the cx the content is running on, so even if we managed to exit our JSAutoCompartment and the JSAutoNullableCompartment our AutoEntryScript sets up, we still wouldn't end up in the page compartment.  :(

I guess there's also this option:

6)  Make JSStackFrame Xrays work from content to chrome and do the right thing in that case.

Comment 7

[Boris Zbarsky [:bzbarsky]]

And I guess:

7)  Wait for XPConnect to not be exposed to content.  Except extensions....

Comment 8

[Boris Zbarsky [:bzbarsky]]

And another issue.  We have XPCWrappedJS implementing nsIException, because of the bits in XPCConvert::JSValToXPCException that do that.  And XPCWrappedJS can't implement [implicit_jscontext] methods, so ends up throwing from toString() and the like with the patches above...  I'm not quite sure where to go with that yet.  :(

Comment 9

[Bobby Holley (:bholley)]

Sorry again for the really long lag. :-(

(In reply to Boris Zbarsky [:bz] from comment #6)
> So I kinda tried #5, and the problem is that this cx is not the cx the
> content is running on, so even if we managed to exit our JSAutoCompartment
> and the JSAutoNullableCompartment our AutoEntryScript sets up, we still
> wouldn't end up in the page compartment.  :(

This is solvable by just doing another nested enter of the original compartment. But the question is _what_ that original compartment is. In the case of an nsRunnable invoking C++ which invokes a method on an XPCWrappedJS, there's no notion of a JS caller. The fact that this concept is undefined in major cases makes me suspect that it's not the right one.
 
> I guess there's also this option:
> 
> 6)  Make JSStackFrame Xrays work from content to chrome and do the right
> thing in that case.

This _would_ be the right solution, I think, except for the fact that it would add a really nasty special case to the security wrapper mechanism for an edge-case benefit.

Fundamentally, there's no reason why this should be as hard as it is. Exception has both the entire captured stack and the principal of its caller, and the only reason we're running into trouble is that we're trying to route our question through the MOP, and get an impedence mismatch between the two sets of invariants.

I met with fitzgen in SF on Wednesday, and we worked out a plan whereby he's going to make the underlying SavedStack accessors accept the principal of the caller as an argument, rather than inferring it from the cx. Given that, it seems like we can just add some jsfriendapi shims to those accessors, and extract the information like that.

Does that seem reasonable to everyone?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> But the question is _what_ that original compartment is.

That is precisely what we do not know, which is why I wasn't trying to enter it...

Really, the only case in which we need to do something special here is the very special case of an XPCWrappedJS inside an XPCWrappedNative with the latter exposed to the web.  And this case is going away, per comment 7... again except extensions.

Maybe we could just stash a compartment somewhere on the XPCCallContext or the like in that special situation.  Or something.  Anyway, this is all moot given your proposal.

> Does that seem reasonable to everyone?

Yes!  It might even give me that non-painful-to-use C++ SavedStacks API I've been asking for and had more or less given up on.

Thoughts on the issue in comment 7?

Comment 11

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #8)
> And another issue.  We have XPCWrappedJS implementing nsIException, because
> of the bits in XPCConvert::JSValToXPCException that do that.  And
> XPCWrappedJS can't implement [implicit_jscontext] methods, so ends up
> throwing from toString() and the like with the patches above...  I'm not
> quite sure where to go with that yet.  :(

We can just nsContentUtils::GetCurrentJSContext/AutoJSContext our way around that issue.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> We can just nsContentUtils::GetCurrentJSContext/AutoJSContext our way around that issue.

Ok, as long as you're happy with that.  It makes things less explicit, but I can add comments to make it clear what's going on.

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8550113 [details] [diff] [review]
part 2.  Stop caching things in JSStackFrame when we're called over Xrays

If I understand correctly these patches are going to change slightly on top of fitzgen's patches, and I can review them then.

Comment 14

[Boris Zbarsky [:bzbarsky]]

OK.  Is there a bug for the proposed API changes from comment 9?

Comment 15

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #14)
> OK.  Is there a bug for the proposed API changes from comment 9?

NI fitzgen for this piece as well.

Comment 16

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

Sorry for the delay, I've been out of commission with the flu.

Bug 1117242 will have the SavedFrame + Xray changes.

Comment 17

[Boris Zbarsky [:bzbarsky]]

OK, so I have patches for this stuff, using the APIs added in bug 1031152.  Unfortunately, using those APIs leads to test failures.  See https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0f8423814fc 

The failures seem to be about browser-chrome or devtools tests... presumably they're chrome code poking at a content-side exception and seeing the chrome parts of the stack too.

I'm not sure exactly what the expected behavior here is, actually.  What should chrome code using .stack on a content exception see?  Does it matter whether it does that on an Xray for the content exception or on a waiver?  What behavior did we settle on for chrome accessing content-side SavedFrames?

Comment 18

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

(In reply to Not doing reviews right now from comment #17)
> OK, so I have patches for this stuff, using the APIs added in bug 1031152. 
> Unfortunately, using those APIs leads to test failures.  See
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0f8423814fc 
> 
> The failures seem to be about browser-chrome or devtools tests... presumably
> they're chrome code poking at a content-side exception and seeing the chrome
> parts of the stack too.

Yes, I believe that is what they are doing.

> I'm not sure exactly what the expected behavior here is, actually.  What
> should chrome code using .stack on a content exception see?  Does it matter
> whether it does that on an Xray for the content exception or on a waiver? 
> What behavior did we settle on for chrome accessing content-side SavedFrames?

Ideally, if the `devtools.debugger.chrome-enabled` pref is set, we would include the chrome frames and otherwise not.

This was why in bug 1117242, my initial impulse was to swap the behavior of Xrays and CCWs with SavedFrame: so that the default (Xrays) would give you the stack as the content sees it (what devtools usually wants), but CCWs (that you have to explicitly opt into with waiveXrays) would give you the full stack.

So what needs to happen here to make these tests pass is waive Xrays before getting the .stack property. However, (DOM)Error objects aren't frozen (right?), like SavedFrame objects are, so I don't know if that is safe...

Can we expose the internal SavedFrame object to chrome code? Then devtools could just operate on that object.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> So what needs to happen here to make these tests pass is waive Xrays before getting the
> .stack property.

No, that's no good.  Then you have no guarantee that you're calling the right .stack getter, even.

> Can we expose the internal SavedFrame object to chrome code?

We can, for Exception/DOMException, because we can do chromeonly things in bindings.

For Error that will be a bit more complicated, right?

I can do that if that's easiest for devtools, though.

Another plausible option is that the JSAPI getters could have the following behavior: If the compartments of cx and the SavedFrame object are different (so we're not in the "I am accessing my own stuff" case) and the compartment of cx subsumes the compartment of the SavedFrame (so either we're same-origin or we _would_ have an Xray if we were actually operating on the SavedFrame directly, I believe), then enter the compartment of the SavedFrame (corresponding to waiving the Xray in the Xray case and not much of anything interesting in the cross-compartment same-origin case) before starting to filter things based on the compartment principal.  We could even make this behavior controlled by the `devtools.debugger.chrome-enabled` pref if really desired...

Comment 20

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

(In reply to Not doing reviews right now from comment #19)
> Another plausible option is that the JSAPI getters could have the following
> behavior: If the compartments of cx and the SavedFrame object are different
> (so we're not in the "I am accessing my own stuff" case) and the compartment
> of cx subsumes the compartment of the SavedFrame (so either we're
> same-origin or we _would_ have an Xray if we were actually operating on the
> SavedFrame directly, I believe), then enter the compartment of the
> SavedFrame (corresponding to waiving the Xray in the Xray case and not much
> of anything interesting in the cross-compartment same-origin case) before
> starting to filter things based on the compartment principal.  We could even
> make this behavior controlled by the `devtools.debugger.chrome-enabled` pref
> if really desired...

This sounds best to me, if we make it controlled by the pref.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Switch to using the new stackframe APIs in JSStackFrame

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Stop caching things in JSStackFrame when we're called over Xrays

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 diff -w

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Drop all the DOMException-cloning and sanitization gunk we added in bug 1107592 and bug 1107953 and bug 1117242

Comment 25

[Bobby Holley (:bholley)]

Comment on attachment 8571506 [details] [diff] [review]
part 2.  Stop caching things in JSStackFrame when we're called over Xrays

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

::: dom/bindings/Exceptions.cpp
@@ +414,5 @@
> +                    ReturnType aValue)
> +{
> +  if (!aStack) {
> +    // Nothing much we can do here past returning our cached value.
> +    *aUseCachedValue = true;

Should we really be doing this? I don't think we should do the potentially-security-sensitive "wrong thing" in a corner failure case. Can we assert against it or just fail?

@@ +660,5 @@
> +  // returns bool, not JS::SavedFrameResult.  Maybe it's possible to
> +  // make the templates more complicated to deal, but in the meantime
> +  // let's just inline GetValueIfNotCached here.
> +  if (!mStack) {
> +    aStack = mFormattedStack;

Same concern here.

Comment 26

[Bobby Holley (:bholley)]

Comment on attachment 8571508 [details] [diff] [review]
part 3.  Drop all the DOMException-cloning and sanitization gunk we added in bug 1107592 and bug 1107953 and bug 1117242

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

\o/

Comment 27

[Boris Zbarsky [:bzbarsky]]

> Should we really be doing this? 

Yeah, that's fair.  I guess what we can do instead is just throw.  I'll make GetValueIfNotCached return nsresult, I guess, and just do that.  In practice people shouldn't be calling stuff on an unlinked nsIStackFrame anyway.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Oh, and I guess mark all this stuff as maybe-throwing in the webidl.  :(

Comment 29

[Boris Zbarsky [:bzbarsky]]

Or actually, just leave the webidl bits doing what they do now on failure; that should be ok.

Comment 30

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c444283fa15
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef27e001d45e
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb099aee1067

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7c444283fa15
https://hg.mozilla.org/mozilla-central/rev/ef27e001d45e
https://hg.mozilla.org/mozilla-central/rev/eb099aee1067