Closed Bug 1219749 Opened 5 years ago Closed 5 years ago

Add a way to "throw from JSContext" on ErrorResult

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

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: 5 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.