Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in html/dom/reflection.js around valueOf #44315

Closed
tbroyer opened this issue Jan 31, 2024 · 1 comment · Fixed by #44355
Closed

Bug in html/dom/reflection.js around valueOf #44315

tbroyer opened this issue Jan 31, 2024 · 1 comment · Fixed by #44355

Comments

@tbroyer
Copy link
Contributor

tbroyer commented Jan 31, 2024

In html/dom/reflection.js, some tests are conducted with objects like

{valueOf:function() {return 3;}}],

I believe these objects are missing a toString:null (like some others have BTW).
Without the the toString:null, the attribute is actually set to [object Object], not to 3 as expected; turning them identical in effect to the {"test": 6} tests.

The tests pass because they all but 2 have a domExpected function that will actually compute an "unexpected" expected value. Of the two tests that don't have, clamped unsigned long will actually re-compute the domExpected later on, and double will never exercise them as all (tested) reflected double attributes have a customGetter that neutralizes those tests.

Context: I'm doing some research around attribute reflection related to custom elements (blog post) and started using the WPT html/dom/reflection.js, slightly modified, to test the behavior of custom elements. My tests wouldn't pass with a double property and debugging (adding more precise logging on test failure) led me to this bug.

@tbroyer
Copy link
Contributor Author

tbroyer commented Jan 31, 2024

Fwiw, there's another bug in the double tests, this time around -0: the idlIdlExpected lists -0 as the expected value, but since the attribute value is "0", the getter can at no point parse it to -0; the expected value can only ever be 0, never -0.

wpt/html/dom/reflection.js

Lines 540 to 542 in adc1ac0

"idlTests": [ -10000000000, -1, -0, 0, 1, 10000000000],
"idlDomExpected": ["-10000000000", "-1", "0", "0", "1", "10000000000"],
"idlIdlExpected": [ -10000000000, -1, -0, 0, 1, 10000000000]

This is due to:

Again, this test is actually never run due to the customGetter (by the way, one might notice that the progress element is of type limited double, that isn't supported by html/dom/reflection.js).

As this is closely related (if only because it's never actually exercised) to the valueOf-related bug above, I don't think this deserved its own issue, hence reported here as an additional comment.

tbroyer added a commit to tbroyer/wpt that referenced this issue Feb 1, 2024
Without a toString:null in test values that are objects
with a valueOf method, the DOM attribute would actually
be set to "[object Object]" without ever calling the
valueOf method (effectively turning those tests into
behaving exactly the same as the {"test": 6} ones).

Most existing tests were passing because the implemented
algorithms would actually compute a WRONG expected value.
The only test that doesn't compute the expected value
("double") won't actually run because all reflected IDL
attributes of type "double" have custom getters.

Fixes web-platform-tests#44315
zcorpan pushed a commit that referenced this issue Feb 6, 2024
* Fix reflection tests wrt valueOf

Without a toString:null in test values that are objects
with a valueOf method, the DOM attribute would actually
be set to "[object Object]" without ever calling the
valueOf method (effectively turning those tests into
behaving exactly the same as the {"test": 6} ones).

Most existing tests were passing because the implemented
algorithms would actually compute a WRONG expected value.
The only test that doesn't compute the expected value
("double") won't actually run because all reflected IDL
attributes of type "double" have custom getters.

Fixes #44315

* Fix reflection tests wrt -0 for "double" attributes

Setting -0 to the IDL attribute will set the DOM
attribute to "0" (per ECMAScript's ToString, from the
rules for the the best representation of a number as
a floating point number), and the rules for parsing
flowting-point number values won't ever turn that "0"
into a -0 value.

This test value is never actually being used though,
as all the reflected attributes of type "double" have
custom getters.

* Add a few more reflection tests around whitespace for numeric types

Specifically leading non-ASCII whitespace following leading
ASCII whitespace (should be rejected), and trailing non-ASCII
whitespace (should be ignored)

* Add a few more numeric values to reflection tests

Specifically "+" and "-" to exercise all possible code
paths of failures.

* Add exponential notation to reflection tests of double values

* Implement "limited double" in reflection tests

* Implement the reflect algorithm for doubles

This should make it easier to extend tests with more values.
mbrodesser-Igalia pushed a commit to mbrodesser-Igalia/wpt that referenced this issue Feb 19, 2024
* Fix reflection tests wrt valueOf

Without a toString:null in test values that are objects
with a valueOf method, the DOM attribute would actually
be set to "[object Object]" without ever calling the
valueOf method (effectively turning those tests into
behaving exactly the same as the {"test": 6} ones).

Most existing tests were passing because the implemented
algorithms would actually compute a WRONG expected value.
The only test that doesn't compute the expected value
("double") won't actually run because all reflected IDL
attributes of type "double" have custom getters.

Fixes web-platform-tests#44315

* Fix reflection tests wrt -0 for "double" attributes

Setting -0 to the IDL attribute will set the DOM
attribute to "0" (per ECMAScript's ToString, from the
rules for the the best representation of a number as
a floating point number), and the rules for parsing
flowting-point number values won't ever turn that "0"
into a -0 value.

This test value is never actually being used though,
as all the reflected attributes of type "double" have
custom getters.

* Add a few more reflection tests around whitespace for numeric types

Specifically leading non-ASCII whitespace following leading
ASCII whitespace (should be rejected), and trailing non-ASCII
whitespace (should be ignored)

* Add a few more numeric values to reflection tests

Specifically "+" and "-" to exercise all possible code
paths of failures.

* Add exponential notation to reflection tests of double values

* Implement "limited double" in reflection tests

* Implement the reflect algorithm for doubles

This should make it easier to extend tests with more values.
marcoscaceres pushed a commit that referenced this issue Feb 23, 2024
* Fix reflection tests wrt valueOf

Without a toString:null in test values that are objects
with a valueOf method, the DOM attribute would actually
be set to "[object Object]" without ever calling the
valueOf method (effectively turning those tests into
behaving exactly the same as the {"test": 6} ones).

Most existing tests were passing because the implemented
algorithms would actually compute a WRONG expected value.
The only test that doesn't compute the expected value
("double") won't actually run because all reflected IDL
attributes of type "double" have custom getters.

Fixes #44315

* Fix reflection tests wrt -0 for "double" attributes

Setting -0 to the IDL attribute will set the DOM
attribute to "0" (per ECMAScript's ToString, from the
rules for the the best representation of a number as
a floating point number), and the rules for parsing
flowting-point number values won't ever turn that "0"
into a -0 value.

This test value is never actually being used though,
as all the reflected attributes of type "double" have
custom getters.

* Add a few more reflection tests around whitespace for numeric types

Specifically leading non-ASCII whitespace following leading
ASCII whitespace (should be rejected), and trailing non-ASCII
whitespace (should be ignored)

* Add a few more numeric values to reflection tests

Specifically "+" and "-" to exercise all possible code
paths of failures.

* Add exponential notation to reflection tests of double values

* Implement "limited double" in reflection tests

* Implement the reflect algorithm for doubles

This should make it easier to extend tests with more values.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant