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

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

(firefox45 fixed)

Details

Attachments

(7 attachments)

18.06 KB, patch
peterv
: review+
Details | Diff | Splinter Review
9.41 KB, patch
peterv
: review+
Details | Diff | Splinter Review
7.06 KB, patch
peterv
: review+
Details | Diff | Splinter Review
10.25 KB, patch
peterv
: review+
Details | Diff | Splinter Review
4.48 KB, patch
kaku
: review+
Details | Diff | Splinter Review
8.12 KB, patch
peterv
: review+
Details | Diff | Splinter Review
6.63 KB, patch
peterv
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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.
Attachment #8686306 - Flags: review?(peterv)
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.
Attachment #8686304 - Flags: review?(tkuo) → review+
Part 5 was blocking some other work I was trying to do, so I spun it off into bug 1224636.
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.
Attachment #8686299 - Flags: review?(peterv) → review+
Attachment #8686300 - Flags: review?(peterv) → review+
Attachment #8686301 - Flags: review?(peterv) → review+
Attachment #8686303 - Flags: review?(peterv) → review+
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.
Attachment #8686305 - Flags: review?(peterv) → review+
>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.
Attachment #8686306 - Flags: review?(peterv) → review+
Depends on: 1228707
Depends on: 1228708
Depends on: 1230692
Depends on: 1230700
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.