Bug 130775 – [Bindings] constants are always typed to 'int'

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Antonio Gomes]

that makes the following IDL return -1 (overflow) instead of the actual result.

] interface NodeFilter {
...
    // Constants for whatToShow
    const unsigned long       SHOW_ALL                       = 0xFFFFFFFF;
...
}

Generated code from this IDL looks like:

JSValue jsNodeFilterSHOW_ALL(ExecState* exec, JSValue, PropertyName)
{
    UNUSED_PARAM(exec);
    return jsNumber(static_cast<int>(0xFFFFFFFF)); <--- OVERFLOW
}


Firefox, for instance, returns 4294967295 as expected.

[Darin Adler]

We should try just removing "static_cast<int>" from the generated code. I don’t think that cast is doing us any good.

Actually parsing the type of the constant (“unsigned long”) would be OK, but likely is not needed.

I suggest we make tests for all these enum values. They would be very easy to write:

    shouldBe("NodeFilter.SHOW_ALL", "0xFFFFFFFF");

[Antonio Gomes]

(In reply to comment #1)
> We should try just removing "static_cast<int>" from the generated code. I don’t think that cast is doing us any good.

It might be important to static_cast it, so the proper jsNumber constructor can be called?
 
> Actually parsing the type of the constant (“unsigned long”) would be OK, but likely is not needed.

We already here this information:

-                push(@implContent, "    return JSValue::encode(jsNumber(static_cast<int>(" . $constant->value . ")));\n");
+                push(@implContent, "    return JSValue::encode(jsNumber(static_cast<" . $constant->type . ">(" . $constant->value . ")));\n");

> I suggest we make tests for all these enum values. They would be very easy to write:
> 
>     shouldBe("NodeFilter.SHOW_ALL", "0xFFFFFFFF");

Sounds good. There are actually compile-times tests dumped into the generated C++ code, but an IDL might have a "DoNotCheckConstants" extended attribute entry, that bypasses it.

[Darin Adler]

(In reply to comment #2)
> (In reply to comment #1)
> > We should try just removing "static_cast<int>" from the generated code. I don’t think that cast is doing us any good.
> 
> It might be important to static_cast it, so the proper jsNumber constructor can be called?

No, I don’t believe it is important.

Generally speaking, the jsNumber constructor should correctly convert a number passed in of most any C++ type. It’s overloaded for the various types. But of course we need to test to make sure it works.

> > Actually parsing the type of the constant (“unsigned long”) would be OK, but likely is not needed.
> 
> We already here this information:
> 
> -                push(@implContent, "    return JSValue::encode(jsNumber(static_cast<int>(" . $constant->value . ")));\n");
> +                push(@implContent, "    return JSValue::encode(jsNumber(static_cast<" . $constant->type . ">(" . $constant->value . ")));\n");

If $constant->type is going to be “unsigned long” then the code above is going to be incorrect. The type “unsigned long” in IDL needs to turn into “unsigned” in C++. That’s one of the reasons I made my suggestion above. Casting to unsigned long will convert to 64-bit on a 64-bit system, which is something we don’t want to do.

> There are actually compile-times tests dumped into the generated C++ code, but an IDL might have a "DoNotCheckConstants" extended attribute entry, that bypasses it.

Tests in C++ code would not be sufficient. We need to test this in JavaScript, since it’s a JavaScript binding we are testing.

[Antonio Gomes]

Created attachment 227885 [details]
patch - for EWS

[Build Bot]

Comment on attachment 227885 [details]
patch - for EWS

Attachment 227885 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6078183057129472

New failing tests:
fast/dom/constants.html

[Build Bot]

Created attachment 227893 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 227885 [details]
patch - for EWS

Attachment 227885 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5987782585483264

New failing tests:
fast/dom/constants.html

[Build Bot]

Created attachment 227897 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 227885 [details]
patch - for EWS

Attachment 227885 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4716566755147776

New failing tests:
fast/dom/constants.html

[Build Bot]

Created attachment 227899 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 227885 [details]
patch - for EWS

Attachment 227885 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4573937333698560

New failing tests:
fast/dom/constants.html

[Build Bot]

Created attachment 227901 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Antonio Gomes]

Created attachment 227918 [details]
patch - for review

[Build Bot]

Comment on attachment 227918 [details]
patch - for review

Attachment 227918 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5823585985757184

New failing tests:
media/W3C/audio/canPlayType/canPlayType_application_octet_stream_with_codecs_1.html

[Build Bot]

Created attachment 227927 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Antonio Gomes]

Darin, could you review the patch?

[Darin Adler]

Comment on attachment 227918 [details]
patch - for review

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

Fix is good, but LayoutTests part of patch is wrong.

> LayoutTests/fast/dom/constants-expected.txt:30
> -PASS: nodeFilter.SHOW_ALL should be -1 and is.
> +FAIL: nodeFilter.SHOW_ALL should be -1 but instead is 4294967295.

This is wrong.

We should fix the fast/dom/constants.html test itself, which we now believe was expecting the wrong thing.

We should not instead update the expected results to show failure!

> Source/WebCore/ChangeLog:16
> +        NodeFilter.SHOW_ALL (0xFFFFFF).

Should have 8 Fs there, not 6.

[Antonio Gomes]

Created attachment 227998 [details]
patch  - as per Darin's review

[Antonio Gomes]

Created attachment 227999 [details]
(actual) patch - as per Darin's review

[Darin Adler]

Comment on attachment 227999 [details]
(actual) patch - as per Darin's review

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

> LayoutTests/fast/dom/constants.html:83
> +    shouldBe("nodeFilter.SHOW_ALL", 4294967295);

I would have written this as 0xFFFFFFFF.

> LayoutTests/fast/dom/constants.html:100
> +    shouldBe("window.NodeFilter.SHOW_ALL", 4294967295);

Ditto.

[Antonio Gomes]

(In reply to comment #20)
> (From update of attachment 227999 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227999&action=review
> 
> > LayoutTests/fast/dom/constants.html:83
> > +    shouldBe("nodeFilter.SHOW_ALL", 4294967295);
> 
> I would have written this as 0xFFFFFFFF.
> 
> > LayoutTests/fast/dom/constants.html:100
> > +    shouldBe("window.NodeFilter.SHOW_ALL", 4294967295);
> 
> Ditto.

Done: <https://trac.webkit.org/r166413>