798438 - nsHTMLCanvasElement::GetContext is stupid

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

The handling of context creation attributes (termed "options" there) is brain-dead: doing

    canvas.getContext("experimental-webgl", { ... some non-default attributes ... });

results in MULTIPLE WebGLContext::SetDimensions calls. This is because it first creates the drawing buffer with DEFAULT attributes (nsHTMLCanvasElement.cpp) and ONLY THEN reads the attributes and passes them to UpdateContext (line 722) which causes the drawing buffers to get re-allocated from scratch.

There's more. That GetContext function has a big while loop:

while (mCurrentContextId.IsEmpty()) {

that makes the whole logic there hard to read (i.e. hard to prove that SetDimensions won't happen more than once), and only seems to serve one purpose, which is to implement a forceThebes flag that seems only useful for the 2D context.

Finally, another issue is that the attributes are being read there as a property bag; IIUC the WebIDL bindings support functions taking dictionaries as arguments so we should just use that, but that part is blocked on the canvas element itself getting ported to WebIDL bindings.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Hm. While we _are_ calling SetDimensions multiple times on context creation (GDB shows it) the reason seems different from what I thought in comment 0. GetContextHelper does not actually seem to call SetDimensions. Need more debugging. Anyhow, the code around there is super convoluted and could use simplification.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The forceThebes stuff can go away now since we don't even create the canvas buffer in GetContext() ... it's created lazily and fallback happens internally by falling back to a cairo-based Azure DrawTarget. So just rip it out.

Comment 3

[David Zbarsky (:dzbarsky)]

Attachment: Use a dictionary for context options

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 758290 [details] [diff] [review]
Use a dictionary for context options

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

::: dom/webidl/HTMLCanvasElement.webidl
@@ +32,5 @@
>    [Pure, SetterThrows]
>             attribute unsigned long height;
>  
>    [Throws]
> +  nsISupports? getContext(DOMString contextId, optional ContextOptions arguments);

Does this maintain the ability to pass a user object that happens to have some relevant attributes attached to it?

Comment 5

[Masatoshi Kimura [:emk]]

Yes. Converting ECMAScript values from/to WebIDL dictionaries is well speced.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 758290 [details] [diff] [review]
Use a dictionary for context options

Shouldn't we end up using different dictionaries for 2d and webgl?

>+    ContextOptions options;
>+    rv = UpdateContext(options);

ContextOptionsInitializer?  Otherwise you're passing uninitialized data.

But I'd like to understand the 2d vs webgl thing first, since that part is web-observable.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Canvas2D doesn't have this dictionary argument in getContext, while WebGL may or may not.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Note sure if that is the question, but: WebGL and 2D definitely use separate, independent dictionary types. The WebGL one, at least, is specced in the WebGL spec, not in the HTML spec, so it could change at any time without consent from the 2D side.

Comment 9

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky (:bz) from comment #6)
> Comment on attachment 758290 [details] [diff] [review]
> Use a dictionary for context options
> 
> Shouldn't we end up using different dictionaries for 2d and webgl?
> 

SetContextOptions doesn't do anything for 2d so it should be ok.
http://mxr.mozilla.org/mozilla-central/ident?i=SetContextOptions

Comment 10

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

> SetContextOptions doesn't do anything for 2d so it should be ok.

The point is, the fact that you look at the incoming object is web-visible.  And right now it seems to be looked at in the "webgl way".

As long as we're messing with this code, we might as well do it right...

Comment 12

[Masatoshi Kimura [:emk]]

At least we can make the code less messy by calling WebGLContextAttributes::Init without depending on WebIDL codegen.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yes, that's what I mean.  Take "any" in the IDL and init the right dictionary explicitly in the C++.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 758861 [details] [diff] [review]
Patch

Per comments.

Comment 15

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 759521 [details] [diff] [review]
Patch

>+    WebGLContextAttributes attributes;

Hrm.  Ideally this would be:

    RootedDictionary<WebGLContextAttributes> attributes(aCx);

but your JSContext is sometimes null.  :(

I guess this is OK as long as no one adds gcthings to the dictionary....

>+    JS::Handle<JS::Value> options =
>+      aContextOptions.WasPassed() ? aContextOptions.Value() : 

If you make the second argument to getContext in HTMLCanvasElement.webidl be "optional any contextOptions = null", this code should all get much simpler...

>+  nsCOMPtr<nsISupports> context = do_QueryInterface(mCurrentContext);

Why do you need the QI here?

>+    //boolean alpha = true;

Add a comment that this is because we have a pref that causes us to have non-spec behavior here?

r=me with those fixed.

JS::NullHandleValue;

Comment 17

[Masatoshi Kimura [:emk]]

Why are you removing getContext() from xpidl? Now it is implementable because the corresponding WebIDL uses 'any'.
If you remove it anyway, please bump the iid.

Comment 18

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky (:bz) from comment #16)
> Comment on attachment 759521 [details] [diff] [review]

> If you make the second argument to getContext in HTMLCanvasElement.webidl be
> "optional any contextOptions = null", this code should all get much
> simpler...
> 

Nice.

> >+  nsCOMPtr<nsISupports> context = do_QueryInterface(mCurrentContext);
> 
> Why do you need the QI here?
> 

I don't if I change context to a nsCOMPtr<nsICanvasRenderingContextInternal>

> >+    //boolean alpha = true;
> 
> Add a comment that this is because we have a pref that causes us to have
> non-spec behavior here?
> 

Done

Comment 19

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52e83bf32c77

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/52e83bf32c77