All users were logged out of Bugzilla on October 13th, 2018

nsHTMLCanvasElement::GetContext is stupid

RESOLVED FIXED in mozilla24

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bjacob, Assigned: dzbarsky)

Tracking

unspecified
mozilla24
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
(Reporter)

Comment 1

6 years ago
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.
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.
(Assignee)

Comment 3

5 years ago
Created attachment 758290 [details] [diff] [review]
Use a dictionary for context options
Attachment #758290 - Flags: review?(bzbarsky)
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?
Yes. Converting ECMAScript values from/to WebIDL dictionaries is well speced.
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.
Attachment #758290 - Flags: review?(bzbarsky) → review-
Canvas2D doesn't have this dictionary argument in getContext, while WebGL may or may not.
(Reporter)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
(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
(Assignee)

Comment 10

5 years ago
Created attachment 758861 [details] [diff] [review]
Patch
Assignee: nobody → dzbarsky
Attachment #758290 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #758861 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Summary: nsHTMLCanvasElemet::GetContext is stupid → nsHTMLCanvasElement::GetContext is stupid
> 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...
At least we can make the code less messy by calling WebGLContextAttributes::Init without depending on WebIDL codegen.
Yes, that's what I mean.  Take "any" in the IDL and init the right dictionary explicitly in the C++.
Comment on attachment 758861 [details] [diff] [review]
Patch

Per comments.
Attachment #758861 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 15

5 years ago
Created attachment 759521 [details] [diff] [review]
Patch
Attachment #758861 - Attachment is obsolete: true
Attachment #759521 - Flags: review?(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;
Attachment #759521 - Flags: review?(bzbarsky) → review+
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.
(Assignee)

Comment 18

5 years ago
(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
https://hg.mozilla.org/mozilla-central/rev/52e83bf32c77
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 881261

Updated

5 years ago
Depends on: 881681
You need to log in before you can comment on or make changes to this bug.