1219749 - Add a way to "throw from JSContext" on ErrorResult

Status: RESOLVED FIXED

Description

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

We have a somewhat-common pattern in our tree that looks like this:

  void SomeIDLMethod(JSContext* cx, ErroResult& rv) {
    if (!JS::DoSomething(cx)) {
      // DoSomething threw, exception already on cx
      rv.Throw(NS_ERROR_FAILURE);
      return;
    }
  }

This is all fine and dandy if DoSomething threw an actual exception.  But if DoSomething threw an _uncatchable_ exception (e.g. via the slow script dialog) then the above code will cause the bindings to throw an NS_ERROR_FAILURE XPConnect exception, which is catchable.  That's busted.

I see two ways of fixing this.  Either we add a ThrowFromJSContext method on ErrorResult which basically checks whether there is a pending exception and if so throws NS_ERROR_FAILURE, otherwise throws NS_ERROR_UNCATCHABLE_EXCEPTION.  Or we pick an nsresult value which explicitly means "don't put a pending exception on the JSContext; if there is one on there now, great, if not, also great".

Now that I write it down, I like the second option better.

Comment 1

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

Note that we have the existing an unused NS_ERROR_XPC_JS_THREW_EXCEPTION, but I'd rather not use it here because of the "XPC" bit...

Comment 2

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

bkelly had the excellent idea of having the API be rv.NoteJSContextException().

Comment 3

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

Attachment: Add a way to faithfully propagate the "exception is already on JSContext" state through an ErrorResult

Comment 4

[Peter Van der Beken [:peterv] - Away till Aug 17th 2019]

Comment on attachment 8680643 [details] [diff] [review]
Add a way to faithfully propagate the "exception is already on JSContext" state through an ErrorResult

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

::: xpcom/base/ErrorList.h
@@ +548,5 @@
>    ERROR(NS_ERROR_DOM_DOMEXCEPTION,                 FAILURE(1017)),
>  
> +  /* An nsresult value to use in ErrorResult to indicate that we
> +   * should just rethrow whatever is on the JSContext (which might be
> +   *  nothing if an uncatchable exception was thrown.

Looks like there's an extra space after the *.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7662e3d92a36

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7662e3d92a36