816187 - Pass context-lost.html WebGL 1.0.1 conformance test

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

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

Attachment: let getContextAttributes's retval be nullable

The spec around getContextAttributes is still in flux, has changed recently to specify that it should return null when the context is lost, so we are no longer conformant; and fixing that is blocked on the issue that the current spec says that its return value is a nullable WebGLContextAttributes dictionary but the WebIDL spec says dictionaries can't be nullable and accordingly our WebIDL parser rejects that.

Here's a patch that we could take in a parallel universe where nullable dictionaries would be allowed as return values. If anyone has some antimatter to spare, we might be able to make this parallel universe meet ours.

See public_webgl discussion:
https://www.khronos.org/webgl/public-mailing-list/archives/1211/msg00201.html

Comment 1

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

Attachment: let getContextAttributes's retval be nullable; make codegen accept nullable dictionary retvals

This builds.... and even runs... and even passes the conformance test! But it's just pure luck. Is there a path from here to a landable patch?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 686388 [details] [diff] [review]
let getContextAttributes's retval be nullable; make codegen accept nullable dictionary retvals

This is generally the right approach.  A few issues:

1)  The check for isPrimitive() that came before the nested getWrapTemplateForType call in the nullable case should stay there and just check for isPrimitive() or isDictionary().

2)  We have to keep doing the Initializer thing.  That's what makes sure the dictionary is initialized properly so that callees only have to set the non-default values in it.  It doesn't make me happy, because Nullable<WebGLContextAttributesInitializer> is annoying... the other option is to rejigger things in codegen so that we can actually initialize the return value in some way before calling the method, not just declare it.  Maybe that's the way to go; let me know if you want me to give that a stab.  Note that we could live with the ugly for now and do the "initialize the return value" as a followup.

3)  The test for nullable unions containing a dictionary went away.  Also, and IDLArgument is never an IDLNullableType (though argument.type could be).

4)  The right place to do the "nullable dictionary arg" check is probably in IDLMethod.validate, where we can just walk through all the argument types and check them, and not have to worry about them not being fully resolved or whatever.

5)  We probably want to add some parser unit tests for this (both for allowing the nullable dictionary return value, and for still disallowing the arguments, if we don't have tests for that part already).  You can run the unit tests with "make -C $objdir/dom/bindings/test check-interactive".

Comment 3

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

Attachment: let getContextAttributes's retval be nullable; make codegen accept nullable dictionary retvals

Thanks!

Applied your comments but:
 - have no time (due to b2g) to do the nice things to remove Initializer
 - wrote a test for what should succeed, but don't know how to test something that should fail
 - I also get a test failure:

$ make -C obj-firefox-debug/dom/bindings/test check-interactive
make: Entering directory `/hack/mozilla-central/obj-firefox-debug/dom/bindings/test'
PYTHONDONTWRITEBYTECODE=1 /hack/mozilla-central/obj-firefox-debug/_virtualenv/bin/python /hack/mozilla-central/config/pythonpath.py \
          -I/hack/mozilla-central/other-licenses/ply /hack/mozilla-central/dom/bindings/test/../parser/runtests.py -q
Starting test /hack/mozilla-central/dom/bindings/test/../parser/tests/test_dictionary.py
TEST-UNEXPECTED-FAIL | Dictionary arg must not be nullable
Finished test /hack/mozilla-central/dom/bindings/test/../parser/tests/test_dictionary.py
make: Leaving directory `/hack/mozilla-central/obj-firefox-debug/dom/bindings/test'

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 686849 [details] [diff] [review]
let getContextAttributes's retval be nullable; make codegen accept nullable dictionary retvals

>+        if nullable:
>+            returnType = returnType.inner

I don't think you need this part, now that I think about it.  The makeDictionaryName call is already doing returnType.unroll(), which will strip off IDLNullableType.

The checks for dictionaries that you left in IDLNullableType doesn't look right to me.  It should only happen for arguments.

You shouldn't really need TestNullDictionaryInterface.  Just throw the new getter on TestInterface and TestExampleInterface?  If the latter makes "make -C $objdir/dom/bindings/test" not work, comment it out for now and file a followup to make it work on me.

But also, what I was looking for were additional unit tests (see dom/bindings/parser/tests/test_dictionary.py where you can just add some more tests; it has examples of how to test that something fails, so you may not need to add any more tests for stuff that should fail, I think).  Those are what you're apparently failing.  That's happening beacause you added your code at a point that's only reached for overloaded methods; see the len(possibleOverloads) == 1 test earlier in that function.  Now that I look at this code more closely, the best place to do it is probably either earlier in validate() or actually in finish() where there's an existing argument.type.isDictionary() test.  You could just test for argument.type.nullable() there, and move the union check there too.

Comment 5

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

Attachment: let getContextAttributes's retval be nullable; make codegen accept nullable dictionary retvals

Did I get this right?

Also, this python code is full of

   # Foo may not be a Bar
   if (Foo is a Bar)
     error("Foo may not be a Bar")

Maybe comments are not useful when they say the same thing as the string below?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 687197 [details] [diff] [review]
let getContextAttributes's retval be nullable; make codegen accept nullable dictionary retvals

>+                if argument.type.isUnion():
>+                    if argument.type.nullable():

Use a single if with "or"?

>+                                raise WebIDLError("An argument cannot be a nullable union "
>+                                                  "containing a dictionary",
>+                                                  [self.location, memberType.location])

Probably better to use [argument.location, memberType.location] as the second arg here.

With that, looks great.  r=me

Please do file a followup on me to stop having to expose this "Initializer" thing to consumers, ok?

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b83a6003fad

Comment 8

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

(In reply to Boris Zbarsky (:bz) from comment #6)
> Please do file a followup on me to stop having to expose this "Initializer"
> thing to consumers, ok?

Filed bug 817194.

Comment 9

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

Comment on attachment 687197 [details] [diff] [review]
let getContextAttributes's retval be nullable; make codegen accept nullable dictionary retvals

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 798187 / Firefox 19
User impact if declined: WebGL 1.0.1 conformance failure. That is most unfortunate as we were finally about to officialize WebGL 1.0.1 conformance.
Testing completed (on m-c, etc.): just landed on m-i
Risk to taking this patch (and alternatives if risky): This is a small patch to our WebIDL parser and codegen, and most of it is validation so a mistake would give either a build error or a failure in the unit test covering this; the rest of the patch, on the WebGL impl, is just boilerplate, so again a mistake there should give a compilation error. Also, we are in early aurora stages.
String or UUID changes made by this patch: none

Comment 10

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

Also, the good news is that the regression had not propagated to beta yet!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5b83a6003fad

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0c97e6c40f6c

Comment 13

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

Ryan, I can't tell you enough how much I appreciate these branch landings.