Throwing an exception when failed to convert the settings argument of HTMLCanvasElement.getContext to dictionary is not web compatible

[arai-a]

(originally reported to https://bugzilla.mozilla.org/show_bug.cgi?id=1244480 )

https://html.spec.whatwg.org/multipage/scripting.html#coerce-context-arguments-for-2d

The spec says that we should propagate an exception thrown while converting the passed argument to a dictionary:

1. Let dict be the result of converting jsval to the dictionary type CanvasRenderingContext2DSettings. If this throws an exception, then propagate the exception and abort these steps.

and WebIDL spec [1] says that converting a non-object value to a dictionary should throw an exception:

1. If Type(V) is not Undefined, Null or Object, then throw a TypeError.

Firefox 44 implemented the error propagation, and it causes the web compat issue for following code:

canvas.getContext("2d", false)

Firefox 43 and older didn't throw exception, and other browsers (Chrome, Safari, Edge) don't throw exception.

Can we change the spec to silently ignore the invalid argument?

[1] https://heycam.github.io/webidl/#es-dictionary

[zcorpan]

So from the bug it appears there is one known broken site because of this.

In Chrome it appears that new Event('foo', false) does throw (so it's not a general webidl impl bug for dictionaries).

In httparchive I can't find anything that passes a non-object to the second argument:

SELECT page, url FROM (
  SELECT
    page,
    url,
    JSON_EXTRACT(payload, '$._body') AS hasBody,
    JSON_EXTRACT(payload, '$.response.content.text') AS content
  FROM [httparchive:har.chrome_jan_1_2016_requests]
)
WHERE hasBody = 'true'
AND REGEXP_MATCH(content, r'getContext\s*\(\s*["\'](2d|webgl)["\']\s*,\s*[a-zA-Z0-9_\.]+\s*\)')

Row	page	url
1	http://www.maketank.it/	http://www.maketank.it/js/threejs/three.js
2	http://www.sandmaennchen.de/	http://www.sandmann.de/basis/js/sandmann/initialize.js
3	http://www.iamondemand.com/	http://iamondemand.com/wp-content/plugins/iamondemand-plugin/includes/three/three.js?ver=4.0
4	http://www.smilegate.com/	http://www.smilegate.com/ko/js/build/three.js
5	http://www.cabbi.bo/	http://cabbi.bo/lib/three.js
6	http://www.livefyre.com/	http://web.livefyre.com/assets/js/vendor/three/three.js
7	http://www.imarine-project.jp/	http://www.imarine-project.jp/js/pixi.js
8	http://www.guns-project.jp/	http://www.guns-project.jp/assets/js/lib/three.js
9	http://www.redpen.uz/	http://redpen.uz/wp-content/plugins/q2w3-fixed-widget/js/q2w3-fixed-widget.min.js?ver=4.0.6
10	http://www.uploadvr.com/	http://alpha.vrchive.com/up/js/deps/three.js
11	http://www.mullen.com/	http://www.mullenloweus.com/wp-content/themes/mullen/libs/pixi.js
12	http://www.rustylake.com/	http://www.rustylake.com/assets/RustyLakeHeader/lib/pixi.dev.js
13	http://www.thepostfamily.com/	http://thepostfamily.com/build/build.js
14	http://www.mintees.com/	http://www.mintees.com/js/egg.js
15	http://www.patatap.com/	http://www.patatap.com/third-party/two.js

I looked at each of these (but skipped subsequent three.js and pixi.js assuming they'd be the same) and they all appear to be doing some variant of options = options || {} or explicitly setting it to an object.

It seems to me this should be possible to evang without changing the spec. It's certainly possible to change getContext with a compat hack (I suppose a union type), but I think we should try to fix this in non-Gecko browsers first and only do the compat hack if we learn that this is a more wide-spread problem than what the data so far suggests.

[zcorpan]

Possibly we could change the spec'd IDL from any... arguments to optional ContextSettings settings where ContextSettings is an empty dictionary and there's a 2d context settings dictionary that extends it, and also a webgl context settings dictionary that extends the same base dictionary. It would mean that different contexts have to make sure they don't use the same names with different types. (alpha currently exists for both 2d and webgl but they're both boolean and both default to true.)

This wouldn't help with passing in false as argument, but maybe the correct behavior falls out more easily if it's all specified as IDL (I haven't investigated what Chrome does but from the moz bug comments it appears it doesn't match the current spec).

[arai-a]

thank you for investigation :) will look into how many websites will be affected.

[arai-a]

update from original bug.

The problematic code comes from LiveCode's experimental feature, and it's already reported to the developers. So, I guess that case could be fixed on their side.

Apart from that, web-platform-test contains a testcase that does not match to the spec. https://github.com/w3c/web-platform-tests/blob/master/html/semantics/embedded-content/the-canvas-element/2d.getcontext.extraargs.html#L22

<p class="desc">The 2D context ignores extra getContext arguments</p> ... _assertDifferent(canvas.getContext('2d', false, {}, [], 1, "2"), null, "canvas.getContext('2d', false, {}, [], 1, \"2\")", "null");

[zcorpan]

OK, updating the test shouldn't be a problem. Filed https://github.com/w3c/web-platform-tests/issues/2540

[zcorpan]

So in Chromium I see this

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/html/HTMLCanvasElement.idl&q=htmlcanvaselement.idl&sq=package:chromium&l=35-49 https://chromium.googlesource.com/chromium/src/+/d1551f0663ddb783db3d309d79145fd0df2221bd

It was deliberately changed in the IDL but was believed to be equivalent, except it apparently is not?

[foolip]

In Blink's IDL a non-standard [PermissiveDictionaryConversion] is used, added while eliminating custom bindings for this.

@kenrussell, do you know more about this? Is the intention to drop [PermissiveDictionaryConversion]?

Using prose in the spec to invoke WebIDL's type conversion isn't great, in terms of implementation we're already past the WebIDL layer. In Blink there's a joint CanvasContextCreationAttributes dictionary, maybe the spec could do the same?

[smaug----]

This wouldn't help with passing in false as argument,

How so? If the param was a dictionary, we'd get the right 'false' handling automatically. though, perhaps you meant that with "maybe the correct behavior falls out more easily if it's all specified as IDL". It is all specified in IDL. Anyhow, I like the approach to have a base ContextSettings as the optional second argument.

Do we know why we have invalid test? Why was it originally added? Has the spec changed since that?

[kenrussell]

When writing https://codereview.chromium.org/795833004 the intent was to maintain maximum compatibility with the previous code, to avoid breaking web pages. I have no plans to remove the PermissiveDictionaryConversion extended attribute from Chrome's Canvas.getContext() IDL. /cc @junov

[domenic]

I tend to think that this is one of the cases where we have interop and should just spec it, as crazy as it is.

[smaug----]

Currently there is no interop between browsers and dictionary handling in this specific case. Gecko follows the current spec, Blink has its own merge-all-the-dictionaries-to-one-and-throw-even-in-case-one-shouldn't (see my comments to https://codereview.chromium.org/795833004) and Edge doesn't seem to throw ever.

[domenic]

Oh, thanks for the correction! I got the wrong impression from some upthread messages.

If Gecko follows the spec then I think the spec should stay the same and we should try to fix other browsers...

[smaug----]

I should have been more exact.

There is one part which is compatible between Edge and Blink, passing not-convertable-to-dictionary value as the second parameter doesn't throw any exception. That is what https://bugzilla.mozilla.org/show_bug.cgi?id=1244480 is about.

But there is also that dictionary handling in general which is very incompatible between browsers here.

[zcorpan]

@smaug----:

How so? If the param was a dictionary, we'd get the right 'false' handling automatically.

Sure, but I meant it would still throw. If we want false to not throw, we'd need to use a union type or something.

though, perhaps you meant that with "maybe the correct behavior falls out more easily if it's all specified as IDL". It is all specified in IDL.

Right.

Anyhow, I like the approach to have a base ContextSettings as the optional second argument.

OK, great.

@kenrussell:

I have no plans to remove the PermissiveDictionaryConversion extended attribute from Chrome's Canvas.getContext() IDL.

OK. So what we should do here, AFAICT, is two things:

• Switch to using a base dictionary for the second argument and have 2d and webgl extend the base dictionary, all using WebIDL syntax instead of prose.
• Make sure that passing a boolean does not throw. Not clear to me what we want to happen if you pass a number, string, etc. Maybe if we want nothing to throw then adding a new extended attribute like the one in Chromium makes sense?

[zcorpan]

Do we know why we have invalid test? Why was it originally added? Has the spec changed since that?

I think the test is relatively old, it seems likely that both HTML and WebIDL have changed since it was written. It seems to me it only wants to check that extra arguments are ignored, and not invoke "can't convert to dictionary" machinery.

[foolip]

How does the base dictionary idea work? Presumably only the empty base dictionary would be checked by WebIDL (generated bindings), so would the rest be in prose? Why not a single merged dictionary?

[zcorpan]

My thinking was that the rest would also be in WebIDL; much like

https://dom.spec.whatwg.org/#dictdef-eventinit https://dom.spec.whatwg.org/#dictdef-customeventinit

Though I realize now that my idea doesn't quite work, since we only have one getContext method while there Event and CustomEvent are different constructors.

So I think there needs to be a single dictionary type, but different specs can extend it using partial dictionary. So:

dictionary ContextAttributes {
    boolean alpha = true;
};
// WebGL would then do:
partial dictionary ContextAttributes {
    // skip alpha
    GLboolean depth = true;
    GLboolean stencil = false;
    GLboolean antialias = true;
    GLboolean premultipliedAlpha = true;
    GLboolean preserveDrawingBuffer = false;
    GLboolean preferLowPowerToHighPerformance = false;
    GLboolean failIfMajorPerformanceCaveat = false;
};

And any new spec that makes up a new rendering context can extend the same dictionary, and be careful not to conflict with existing ones.

[kenrussell]

A single merged dictionary is probably the only feasible IDL to write. It's not feasible to instantiate a specific dictionary subclass depending on which context type is being looked up (not without writing custom bindings, which we wanted to get rid of in Chromium).

It'll be an invariant that all context types (2d, webgl, webgl2, and any subsequent ones) must not define context creation attributes that would be in conflict with each other. For example, it would have been a real problem if the 2d context defined the "alpha" attribute as some other type than the WebGL spec did.

Essentially this means that a central place is needed which defines all of the legal context creation attributes, and other specs like 2d canvas and webgl will need to refer to it, and they'll need to be kept in sync. May I suggest that https://wiki.whatwg.org/wiki/CanvasContexts be this location, and that we reference that wiki page in the spec?

[zcorpan]

@kenrussell yeah I came to the same conclusion (see above) :-) Using the wiki to avoid collisions seems OK to me.

[kenrussell]

Yeah, your comment came in just as I was pressing "Comment" on mine. :) Great.

[smaug----]

OK. So what we should do here, AFAICT, is two things:

Switch to using a base dictionary for the second argument and have 2d and webgl extend the base dictionary, all using WebIDL syntax instead of prose.
Make sure that passing a boolean does not throw. Not clear to me what we want to happen if you pass a number, string, etc. Maybe if we want nothing to throw then adding a new extended attribute like the one in Chromium makes sense?

Oh, I don't want boolean to not throw. It should throw as it throws elsewhere. Consistency in the platform is super important. So only if throwing is causing issues in several sites, we should have this special case to not throw, IMO.

And merging all the dictionaries together feels just wrong. The properties for webgl have nothing to do with 2d, so passing a dictionary for 2d should not start to throw if there is some getter throwing for a webgl related property. I guess this means, that if having a base dictionary doesn't quite work, I'd prefer what we have in the current spec.

[peter-b]

As upstream for the problematic canvas.getContext("2d", false) expression in LiveCode, I have no objection to changing our JS and telling our users to redeploy their apps.

[zcorpan]

Thank you @peter-b!

@smaug---- I agree on a theoretical level that the current spec makes more sense, however conforming to it amounts to custom code for blink, which they wanted to move away from. I don't see that it is a problem in practice to merge the dictionaries.

In the end what matters more is interop, in my opinion. A merged dictionary expressed in IDL syntax seems to me it has the best path to avoid impl differences/bugs.

For throwing or not, I suggest we require throwing until it is shown that browsers have to not throw for web compat (with use counter or trying to ship or so).

[annevk]

It seems this is still wrong in the specification and the solution would be to ignore some exceptions, but it seems we cannot ignore all per se:

document.createElement("canvas").getContext("2d", {get alpha() { throw 1 }})

The above throws in Chrome and Firefox (though not Safari).

Looking at https://bug1244480.bmoattachments.org/attachment.cgi?id=8716020 I guess the solution here is that we check if the argument passed is an object and otherwise ignore the argument passed. That works.