Closed Bug 1215072 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Canvas: WebGL, defect)

36 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Assignee: nobody → bugs
Attached patch patch (obsolete) — Splinter Review
Make sure we throw in case dictionary initialization fails.
A bit messy patch, but the setup is such.
Attachment #8674968 - Flags: review?(amarchesini)
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?
Attachment #8674968 - Flags: review?(amarchesini) → review-
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.
Attachment #8674968 - Flags: review- → review+
(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
Attachment #8674968 - Attachment is obsolete: true
Ah, just a mistake in the test.
Attached patch v3Splinter Review
https://hg.mozilla.org/mozilla-central/rev/b5e0aa4ec511
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1244480
You need to log in before you can comment on or make changes to this bug.