735544 - Allow exception stacks to cross compartment boundaries

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Followup from bug 729589 comment 7.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Allow exception stacks to cross compartment boundaries. v1

Attaching a patch - flagging luke for review.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 605952 [details] [diff] [review]
Allow exception stacks to cross compartment boundaries. v1

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

Nice.  I bet this fixes some existing annoyances for, say, jetpack devs.

::: js/src/jsexn.cpp
@@ +291,3 @@
>      Vector<Value> &values;
> +    AppendWrappedArg(JSContext *cx_, Vector<Value> &values) : cx(cx_)
> +                                                            , values(values) {}

SM style would be (trailing _ is for members):

  AppendWrappedArg(JSContext *cx, Vector<Value> &values)
    : cx(cx),
      values(values)
  {}

@@ +327,1 @@
>              StackFrame *fp = i.fp();

No \n before first line

@@ +327,5 @@
>              StackFrame *fp = i.fp();
> +
> +            /*
> +             * Ask the crystal CAPS ball whether we can see values across
> +             * compartment boundaries.

I would also add "NB: 'fp' may point to cross-compartment values that require wrapping."

Comment 3

[Luke Wagner [:luke]]

> >              StackFrame *fp = i.fp();
> 
> No \n before first line

Splinter doesn't make it clear, but I mean no \n *before* the "StackFrame* fp = " line.

Comment 4

[Bobby Holley (:bholley)]

Updated and pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=b4298bf99678

Comment 5

[Bobby Holley (:bholley)]

Ok, so it looks like my assertion that we didn't have pending exceptions wasn't always valid:

https://tbpl.mozilla.org/php/getParsedLog.php?id=10078640&tree=Try#error0

Luke, what do you suggest?

Comment 6

[Luke Wagner [:luke]]

I say remove JS_IsExceptionPending assert and JS_ClearPendingException.  InitExnPrivate is called in two cases: when constructing an exception object (in which case you shouldn't have a pending exception and you would want any exception generated by wrap to propagate) and js_ErrorToException which, as try has shown, can have an already-pending exception.  However, cx->generatingError state-twiddling in js_ErrorToException will prevent any error generated by wrap from actually being actually being thrown.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #6)
> I say remove JS_IsExceptionPending assert and JS_ClearPendingException. 
> InitExnPrivate is called in two cases: when constructing an exception object
> (in which case you shouldn't have a pending exception and you would want any
> exception generated by wrap to propagate) and js_ErrorToException which, as
> try has shown, can have an already-pending exception.  However,
> cx->generatingError state-twiddling in js_ErrorToException will prevent any
> error generated by wrap from actually being actually being thrown.

The ClearPendingException was necessary to avoid various infinite recursion cases. See this try push:

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

What do you think? Just replace the assert with Clear, and have 2 Clears?

Comment 8

[Luke Wagner [:luke]]

You might try again, I ran several of the failing tests with your patch with the assert and clear calls commented out in operator() and they all passed.

Comment 9

[Bobby Holley (:bholley)]

luke and I figured something out on IRC. Gave this another try push, linux only since those are fast. ;-)

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

Comment 10

[Bobby Holley (:bholley)]

Looks good, at least on release. Given that I'm no longer adding any assertions, that's probably good enough. Pushed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/1d61262c243c

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/1d61262c243c