Closed
Bug 735544
Opened 9 years ago
Closed 9 years ago
Allow exception stacks to cross compartment boundaries
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(1 file)
9.24 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
Followup from bug 729589 comment 7.
Assignee | ||
Comment 1•9 years ago
|
||
Attaching a patch - flagging luke for review.
Attachment #605952 -
Flags: review?(luke)
![]() |
||
Comment 2•9 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•9 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.
Assignee | ||
Comment 4•9 years ago
|
||
Updated and pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=b4298bf99678
Assignee | ||
Comment 5•9 years ago
|
||
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•9 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.
Assignee | ||
Comment 7•9 years ago
|
||
(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•9 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.
Assignee | ||
Comment 9•9 years ago
|
||
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
Assignee | ||
Comment 10•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Target Milestone: --- → mozilla14
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1d61262c243c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•