Closed
Bug 1242165
Opened 9 years ago
Closed 8 years ago
DataView#setXXX need to check the index parameter
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: evilpie, Assigned: evilpie)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
We currently just call ToUint32 on the index parameter. The spec however uses:
24.2.1.2 SetViewValue
3. Let numberIndex be ? ToNumber(requestIndex).
4. Let getIndex be ToInteger(numberIndex).
5. If numberIndex ≠ getIndex or getIndex < 0, throw a RangeError exception.
This currently causes us to fail a fair number of test262.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → evilpies
Assignee | ||
Comment 1•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e343314f633&selectedJob=23999698
Seems like a lot of failure come down to calling those functions without any arguments at all, which used to throw. The new behavior is probably more inline with how the rest of JS behaves, but still seems like a step backwards to.
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8774049 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 3•8 years ago
|
||
Hey! So I was wondering what to do about dom/canvas/test/webgl-conf/generated/test_conformance__typedarrays__data-view-test.html. Is disabling the whole test in mochitest-errata.ini correct? We fail one part of the test now, because view.getInt8() (no arguments) is allowed now.
Flags: needinfo?(jgilbert)
Comment 4•8 years ago
|
||
Comment on attachment 8774049 [details] [diff] [review]
Implement new DataView index behavior
Review of attachment 8774049 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/tests/ecma_6/DataView/get-set-index-range.js
@@ +29,5 @@
> +
> + assertThrowsInstanceOf(() => view.getInt8(0), TypeError);
> + assertThrowsInstanceOf(() => view.setInt8(0, 0), TypeError);
> + assertThrowsInstanceOf(() => view.getInt8(Math.pow(2, 53) - 1), TypeError);
> + assertThrowsInstanceOf(() => view.setInt8(Math.pow(2, 53) - 1, 0), TypeError);
These should be able to go up to Infinity, even to Number.MAX_VALUE, and still work, with the issue I noted fixed.
::: js/src/vm/TypedArrayObject.cpp
@@ +1845,5 @@
> + // 2. Step eliminates < 0, +0 == -0 with SameValueZero
> + // 3/4. Limit to <= 2^53-1, so everything above should fail.
> + if (integerIndex < 0 || integerIndex > 9007199254740991) {
> + JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_BAD_INDEX);
> + return false;
Am I misreading the spec algorithm? We have:
a. Let integerIndex be ? ToInteger(value).
b. If integerIndex < 0, throw a RangeError exception.
c. Let index be ! ToLength(integerIndex).
d. If SameValueZero(integerIndex, index) is false, throw a RangeError exception.
After (a) we have any IEEE-754 value that has no fractional component and isn't NaN -- so, say, 1e200. (b) eliminates all such values less than zero. (c) transforms Infinity into 2**53 - 1 and otherwise clamps values to [0, 2**53 - 1]. (d) throws *if* the original value isn't equal to the value after (c).
Throwing if |integerIndex < 0| matches (b). But throwing if |integerIndex > 2**53 - 1| *doesn't* map to what (c) does. Rather in that case |integerIndex| should be transformed into |min(integerIndex, 2**53 - 1)|.
So this latter bit looks wrong to me.
Attachment #8774049 -
Flags: review?(jwalden+bmo) → review-
Comment 5•8 years ago
|
||
Comment on attachment 8774049 [details] [diff] [review]
Implement new DataView index behavior
Review of attachment 8774049 [details] [diff] [review]:
-----------------------------------------------------------------
Er, wait. No, throwing if |integerIndex > 2**53 - 1| is akin to the if-!SameValueZero-then-throw bit. So this looks fine, sorry.
Attachment #8774049 -
Flags: review- → review+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/450787142f41
Update DataView index handling. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8deefd10b85
Disable conformance/typedarrays/data-view-test due to spec changes.
Assignee | ||
Comment 7•8 years ago
|
||
Thanks jgilbert for providing the patch to disable the failing test. He is also going to get this fixed upstream.
Flags: needinfo?(jgilbert)
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7fd5aed28bc
Disable another conformance/typedarrays webgl test due to spec changes to fix failing webgl tests (and requested by evilpie on IRC). r=me
Comment 9•8 years ago
|
||
Also disabled test_2_conformance__typedarrays__data-view-test.html
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7fd5aed28bc36991aa6904d4036043082ece224
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/450787142f41
https://hg.mozilla.org/mozilla-central/rev/b8deefd10b85
https://hg.mozilla.org/mozilla-central/rev/a7fd5aed28bc
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•