Canvas and OffscreenCanvas getContext should check if argument is an object before trying to convert it to a dictionary

• Component: Canvas
• Status: RESOLVED FIXED

[Chris Lord]

Since r277543, which introduced canvas settings handling, imported/w3c/web-platform-tests/html/canvas/offscreen/the-offscreen-canvas/2d.getcontext.extraargs.html and the worker equivalent have been failing. This is due to a TypeError when trying to convert the non-object arguments into a dictionary.

Reading the spec, both the section on getContext[1] and the section on context creation[2], it seems that arguments are expected to be converted to either an object or null before being converted to a settings dictionary. As we lack this step, there are more situations where we throw an exception when a compliant browser wouldn't.

[1] https://html.spec.whatwg.org/multipage/canvas.html#dom-offscreencanvas-getcontext
[2] https://html.spec.whatwg.org/multipage/canvas.html#offscreen-2d-context-creation-algorithm

Ms2ger is working on more tests for this in WPT, we should fix this bug and sync those tests here once they exist.

[Chris Lord]

Created attachment 433322 [details]
Patch

[Chris Lord]

Created attachment 433327 [details]
Patch

[Chris Lord]

Created attachment 433328 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/80625487>

[Sam Weinig]

Comment on attachment 433328 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433328&action=review

> Source/WebCore/html/HTMLCanvasElement.cpp:288
> +        auto settings = convert<IDLDictionary<CanvasRenderingContext2DSettings>>(state, (!arguments.isEmpty() && arguments[0].isObject()) ? arguments[0].get() : JSC::jsUndefined());

I don't think we need the isEmpty() check if we are checking isObject().  That said, I would perfer we make this a bit closer o the spec language (e.g. https://html.spec.whatwg.org/multipage/canvas.html#dom-canvas-getcontext) and use jsNull() if not an object).

[Chris Lord]

(In reply to Sam Weinig from comment #5)
> Comment on attachment 433328 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433328&action=review
> 
> > Source/WebCore/html/HTMLCanvasElement.cpp:288
> > +        auto settings = convert<IDLDictionary<CanvasRenderingContext2DSettings>>(state, (!arguments.isEmpty() && arguments[0].isObject()) ? arguments[0].get() : JSC::jsUndefined());
> 
> I don't think we need the isEmpty() check if we are checking isObject(). 
> That said, I would perfer we make this a bit closer o the spec language
> (e.g.
> https://html.spec.whatwg.org/multipage/canvas.html#dom-canvas-getcontext)
> and use jsNull() if not an object).

We still need the check as arguments can be empty, but from this I'm guessing what you're saying is it should be undefined (no arguments), null (argument that isn't an object) or dictionary (at least one object argument specified that converts without an exception).

I don't know that there's any observable side-effect from what you're suggesting, but I'll amend the patch.

[Chris Lord]

Created attachment 433862 [details]
Patch

[Sam Weinig]

(In reply to Chris Lord from comment #6)
> (In reply to Sam Weinig from comment #5)
> > Comment on attachment 433328 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=433328&action=review
> > 
> > > Source/WebCore/html/HTMLCanvasElement.cpp:288
> > > +        auto settings = convert<IDLDictionary<CanvasRenderingContext2DSettings>>(state, (!arguments.isEmpty() && arguments[0].isObject()) ? arguments[0].get() : JSC::jsUndefined());
> > 
> > I don't think we need the isEmpty() check if we are checking isObject(). 
> > That said, I would perfer we make this a bit closer o the spec language
> > (e.g.
> > https://html.spec.whatwg.org/multipage/canvas.html#dom-canvas-getcontext)
> > and use jsNull() if not an object).
> 
> We still need the check as arguments can be empty, but from this I'm
> guessing what you're saying is it should be undefined (no arguments), null
> (argument that isn't an object) or dictionary (at least one object argument
> specified that converts without an exception).
> 
> I don't know that there's any observable side-effect from what you're
> suggesting, but I'll amend the patch.

Apologies, I totally read that wrong. I read it as arguments[0].isEmpty() when that's not what it is at all :(. I think you can keep that part the same, and just replace the jsUndefined() with jsNull(), but either will result in the same thing so r=me.

[Chris Lord]

Comment on attachment 433862 [details]
Patch

I guess this doesn't hurt and it does read closer to the spec, even if it doesn't make a difference, so let's go for it :)

[EWS]

Committed r280084 (239809@main): <https://commits.webkit.org/239809@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433862 [details].