1224007 - Make ErrorResult complain if it comes off the stack without being handled in some way

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

I'd like to avoid situations where we have a pending exception on an ErrorResult and then just silently lose it.  This is sort of related to bug 933378, but not quite the same thing.

I considered trying to do a setup like we have for JS exceptions right now, but ran into problems with the typical StealNSResult consumer, which looks like this:

  if (rv.Failed()) {
    return rv.StealNSResult();
  }

Such consumers would have to add a WouldReportException() or some such call before that if block, which is rather annoying.  We could perhaps handle it by changing the StealNSResult() signature so the code above would become:

  nsresult res;
  if (rv.MaybeStealNSResult(&res)) {
    // True return means failure.
    return res;
  }

But that's a rather ugly API...

In any case, for now I've done something a bit weaker: just assert in ~ErrorResult that !Failed() and make sure that all our ways of hadling an ErrorResult: StealNSResult, SuppressException, ThrowMethodFailed (in today's tree; these patches make it a member named MaybeSetPendingException), and ToJSValue (which calls MaybeSetPendingException internally) reset the ErrorResult to NS_OK.  That should get us closer to doing the above "ugly API" bit in the future if we want to.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Rename ThrowMethodFailed to MaybeSetPendingException and make it an ErrorResult instance method

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Make the various ErrorResult::Report* methods private, so consumers all go through MaybeSetPendingException and rename them to more clearly indicate what they're actually doing

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Push down WouldReportJSException into MaybeSetPendingException, since anyone calling the latter will propagate the JS exception as needed

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Fix some cases in which ErrorResult instances are destroyed without doing anything useful with exceptions on them

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Fix some code in ImageBitmap that rejects a promise with an ErrorResult, then keeps trying to use that ErrorResult

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Get rid of ErrorResult::StealJSException

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Change MaybeSetPendingException to set the ErrorResult state to "not failed", just like SuppressException and StealNSResult already do, and assert in the destructor that the ErrorResult is not Failed()

This is not quite as strong as being able to assert that all codepaths that might
lead to failure call one of the above methods, but being able to assert that
would involve a lot of extra noise at callsites.  Or at least changing the
signature of StealNSResult to use an outparam and return a boolean indicating
whether the ErrorResult was failure or not, or something, so StealNSResult
can be usefully called on successful ErrorResults too.

Comment 8

[Tzuhao Kuo [:kaku]]

Comment on attachment 8686304 [details] [diff] [review]
part 5.  Fix some code in ImageBitmap that rejects a promise with an ErrorResult, then keeps trying to use that ErrorResult

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

Thank you for simply the logic. LGTM.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Part 5 was blocking some other work I was trying to do, so I spun it off into bug 1224636.

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 8686299 [details] [diff] [review]
part 1.  Rename ThrowMethodFailed to MaybeSetPendingException and make it an ErrorResult instance method

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

::: dom/bindings/BindingUtils.cpp
@@ +447,5 @@
> +void
> +ErrorResult::SetPendingException(JSContext* cx)
> +{
> +  if (IsUncatchableException()) {
> +    // Nuke any existing exception on aCx, to make sure we're uncatchable.

s/aCx/cx/

::: dom/bindings/ErrorResult.h
@@ +133,5 @@
> +  // exception on the given JSContext.  This is the normal "throw an exception"
> +  // codepath.
> +  //
> +  // The return value is false if the ErrorResult represents success, true
> +  // otherwise.  This does mean that in JSAPI method implementatitions you can't

s/implementatitions/implementations/

@@ +134,5 @@
> +  // codepath.
> +  //
> +  // The return value is false if the ErrorResult represents success, true
> +  // otherwise.  This does mean that in JSAPI method implementatitions you can't
> +  // just use this as |return rv.MaybeSetPendingException(cx)|, but in practice

But we could do |return !rv.MaybeSetPendingException(cx)|, right?

::: dom/bindings/ToJSValue.cpp
@@ +55,5 @@
>    MOZ_ASSERT(aArgument.Failed());
>    MOZ_ASSERT(!aArgument.IsUncatchableException(),
>               "Doesn't make sense to convert uncatchable exception to a JS value!");
>    AutoForceSetExceptionOnContext forceExn(aCx);
> +  DebugOnly<bool> throwResult = aArgument.MaybeSetPendingException(aCx);

Why not just aArgument.SetPendingException? We already assert aArgument.Failed() above.

Comment 11

[Peter Van der Beken [:peterv]]

Comment on attachment 8686305 [details] [diff] [review]
part 6.  Get rid of ErrorResult::StealJSException

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

::: dom/promise/PromiseCallback.cpp
@@ +225,5 @@
>    // PromiseReactionTask step 7
>    if (rv.Failed()) {
>      JS::Rooted<JS::Value> value(aCx);
> +    { // Scope for JSAutoCompartment
> +      

Whitespace.

Comment 12

[Boris Zbarsky [:bzbarsky]]

>s/aCx/cx/
>s/implementatitions/implementations/

Done (and ouch on the second one).

> But we could do |return !rv.MaybeSetPendingException(cx)|, right?

Indeed, you could.  Documented.

> Why not just aArgument.SetPendingException? We already assert aArgument.Failed() above.

Two reasons: (1) SetPendingException() is not public (which begs the question of why not, of course) and (2) That would not work with part 3; we want that WouldReport... call.

> Whitespace.

You mean remove the blank line?  OK, done.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97c1d36af517
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7ffa99e2ac9
https://hg.mozilla.org/integration/mozilla-inbound/rev/f38491274f56
https://hg.mozilla.org/integration/mozilla-inbound/rev/169d9adca23f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e597563f32ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecb3051bba08

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/97c1d36af517
https://hg.mozilla.org/mozilla-central/rev/e7ffa99e2ac9
https://hg.mozilla.org/mozilla-central/rev/f38491274f56
https://hg.mozilla.org/mozilla-central/rev/169d9adca23f
https://hg.mozilla.org/mozilla-central/rev/e597563f32ff
https://hg.mozilla.org/mozilla-central/rev/ecb3051bba08