Add support for uncatchable exceptions to Paris bindings

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
8 years ago
5 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks 1 bug)

Trunk
mozilla38
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(2 attachments)

Apparently workers need this.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
I guess I added the support to xpconnect too, just for consistency.  ;)
Comment on attachment 8566069 [details] [diff] [review]
part 1.  Add support for throwing uncatchable exceptions to Web IDL bindings.  People keep asking for this

Review of attachment 8566069 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Exceptions.cpp
@@ +89,5 @@
>  Throw(JSContext* aCx, nsresult aRv, const char* aMessage)
>  {
> +  if (aRv == NS_ERROR_UNCATCHABLE_EXCEPTION) {
> +    // Nuke any existing exception on aCx, to make sure we're uncatchable.
> +    JS_ClearPendingException(aCx);

Seems like some sort of screwup if we have an exception + this result code. Would it make more sense to |MOZ_ASSERT(!JS_IsExceptionPending(aCx))| here? (And in the other spot)
I would be more willing to do that if I trusted xpconnect's and SpiderMonkey's exception handling at all.

Specifically, say I have some API implemented in C++ that's called from JS (but doesn't take a JSContext or anything).  The API implementation calls some nsIFoo method.  The nsIFoo happens to be implemented in JS via XPCWrappedJS and throws, and xpconnect decides to leave an exception on cx and return an error nsresult.

One could argue that the correct behavior here is for the thing that called the nsIFoo method to just propagate an error nsresult out, but I wouldn't be surprised if we had a situation in which it wanted to throw an uncatchable exception in this case or something.  And then the assert would fail.

Once we get our exception throwing/reporting story more sorted out, such that a human can sanely reason about it, adding such an assertion would be more ok with me.
Yeah, makes sense. We also have NS_ERROR_OUT_OF_MEMORY to worry about...
Comment on attachment 8566070 [details] [diff] [review]
part 2.  Use the new uncatchable exception machinery in worker XHR code

Review of attachment 8566070 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/XMLHttpRequest.cpp
@@ +2231,1 @@
>    }

Shouldn't there be a |return| here? (preexisting)
https://hg.mozilla.org/mozilla-central/rev/160607e021a9
https://hg.mozilla.org/mozilla-central/rev/bab7f4c2c67c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
> Shouldn't there be a |return| here? (preexisting)

In Abort?  Yes, there should.  Filed bug 1135427.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.