Allow exception stacks to cross compartment boundaries

RESOLVED FIXED in mozilla14

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Followup from bug 729589 comment 7.
Created attachment 605952 [details] [diff] [review]
Allow exception stacks to cross compartment boundaries. v1

Attaching a patch - flagging luke for review.
Attachment #605952 - Flags: review?(luke)

Comment 2

5 years ago
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+

Comment 3

5 years ago
> >              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?

Comment 6

5 years ago
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?

Comment 8

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 736807
Depends on: 739694
You need to log in before you can comment on or make changes to this bug.