Last Comment Bug 735544 - Allow exception stacks to cross compartment boundaries
: Allow exception stacks to cross compartment boundaries
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Bobby Holley (PTO through June 13)
:
Mentors:
Depends on: 736807 739694
Blocks: cpg
  Show dependency treegraph
 
Reported: 2012-03-13 18:21 PDT by Bobby Holley (PTO through June 13)
Modified: 2012-03-29 17:58 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Allow exception stacks to cross compartment boundaries. v1 (9.24 KB, patch)
2012-03-14 14:33 PDT, Bobby Holley (PTO through June 13)
luke: review+
Details | Diff | Review

Description Bobby Holley (PTO through June 13) 2012-03-13 18:21:15 PDT
Followup from bug 729589 comment 7.
Comment 1 Bobby Holley (PTO through June 13) 2012-03-14 14:33:40 PDT
Created attachment 605952 [details] [diff] [review]
Allow exception stacks to cross compartment boundaries. v1

Attaching a patch - flagging luke for review.
Comment 2 Luke Wagner [:luke] 2012-03-14 14:44:38 PDT
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] 2012-03-14 14:46:14 PDT
> >              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 (PTO through June 13) 2012-03-14 15:02:03 PDT
Updated and pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=b4298bf99678
Comment 5 Bobby Holley (PTO through June 13) 2012-03-14 23:33:46 PDT
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] 2012-03-15 10:23:54 PDT
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 (PTO through June 13) 2012-03-15 10:38:39 PDT
(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] 2012-03-15 11:09:56 PDT
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 (PTO through June 13) 2012-03-15 13:18:18 PDT
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 (PTO through June 13) 2012-03-15 15:21:15 PDT
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] 2012-03-16 06:26:21 PDT
https://hg.mozilla.org/mozilla-central/rev/1d61262c243c

Note You need to log in before you can comment on or make changes to this bug.