Closed Bug 735544 Opened 9 years ago Closed 9 years ago

Allow exception stacks to cross compartment boundaries

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(1 file)

Followup from bug 729589 comment 7.
Attaching a patch - flagging luke for review.
Attachment #605952 - Flags: review?(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."
Attachment #605952 - Flags: review?(luke) → review+
> >              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.
Updated and pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=b4298bf99678
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?
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.
(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?
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.
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
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
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/1d61262c243c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 736807
Depends on: 739694
You need to log in before you can comment on or make changes to this bug.