Followup from bug 729589 comment 7.

Attaching a patch - flagging luke for review.

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."

> > 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