Add a way to "throw from JSContext" on ErrorResult

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
5 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected, firefox45 fixed)

Details

Attachments

(1 attachment)

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.
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...
bkelly had the excellent idea of having the API be rv.NoteJSContextException().
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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 *.
Attachment #8680643 - Flags: review?(peterv) → review+
https://hg.mozilla.org/mozilla-central/rev/7662e3d92a36
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.