1215072 - Assertion failure: !JS_IsExceptionPending(cx), at ./HTMLCanvasElementBinding.cpp:231

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Run the following in the web console (using a debug build):
document.createElement("canvas").getContext("webgl", { get stencil() {throw "bah"; } });
or
document.createElement("canvas").getContext("2d", { get alpha() {throw "bah"; } });

and you see 
Assertion failure: !JS_IsExceptionPending(cx), at ./HTMLCanvasElementBinding.cpp

As far as I see, the patch for bug 645792 was wrong, since it made us return NS_OK, even if there was an error.
In case the dictionary init fails, that should be propagated to the caller, since it effectively happens before the
getContext call.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Make sure we throw in case dictionary initialization fails.
A bit messy patch, but the setup is such.

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8674968 [details] [diff] [review]
patch

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

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +1605,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +CanvasRenderingContext2D::SetContextOptions(JSContext* aCx,

Can we make this void SetContextOptions(..., ErrorResult& aRv) ?

::: dom/canvas/CanvasRenderingContextHelper.cpp
@@ +172,5 @@
>  
>      mCurrentContext = context.forget();
>      mCurrentContextType = contextType;
>  
> +    nsresult rv = UpdateContext(aCx, aContextOptions, aRv);

This would be:
ErrorResult rv;
UpdateContext(aCx, aContextOptions, rv);
if (rv.Failed()) {
  return nullptr;
}

@@ +204,5 @@
>      mCurrentContext = nullptr;
>      return rv;
>    }
>  
> +  rv = currentContext->SetContextOptions(aCx, aNewContextOptions, aRv);

This really confuses me a lot. Why we use aRv somewhere, and rv somewhere else?

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8674968 [details] [diff] [review]
patch

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

I and smaug discussed this bug on IRC. We need a better name for aRv and a comment to explain why we have a double return error code.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrea Marchesini (:baku) from comment #2)
> > +CanvasRenderingContext2D::SetContextOptions(JSContext* aCx,
> 
> Can we make this void SetContextOptions(..., ErrorResult& aRv) ?
As I said on IRC, we really need two different rv values.
But I guess aRv could be called aRvForDictionaryInit


> This would be:
> ErrorResult rv;
> UpdateContext(aCx, aContextOptions, rv);
> if (rv.Failed()) {
>   return nullptr;
> }
And that would regress bug 645792

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: better naming for the variable + a comment

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09e059daabae

Comment 7

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3f4d60ce3418 - b2g would rather not be involved in running that test, https://treeherder.mozilla.org/logviewer.html#?job_id=15792279&repo=mozilla-inbound

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Ah, just a mistake in the test.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5e0aa4ec511

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b5e0aa4ec511