742194 - Add support for uncatchable exceptions to Paris bindings

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Apparently workers need this.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add support for throwing uncatchable exceptions to Web IDL bindings. People keep asking for this

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Use the new uncatchable exception machinery in worker XHR code

Comment 3

[Boris Zbarsky [:bzbarsky]]

I guess I added the support to xpconnect too, just for consistency.  ;)

Comment 4

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/160607e021a9
   https://hg.mozilla.org/integration/mozilla-inbound/rev/bab7f4c2c67c

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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)

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yeah, makes sense. We also have NS_ERROR_OUT_OF_MEMORY to worry about...

Comment 8

[:Benjamin Peterson]

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)

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/160607e021a9
https://hg.mozilla.org/mozilla-central/rev/bab7f4c2c67c

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Shouldn't there be a |return| here? (preexisting)

In Abort?  Yes, there should.  Filed bug 1135427.