1405285 - Add JS tests for some edge cases in jsarray/jsobj

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Christian Holler (:decoder)]

This bug is about adding some tests to jit-tests that cover the following areas of our engine:

http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/js/src/jsarray.cpp#440 This location is uncovered by fuzzing due to the only test in our codebase hitting this (by chance) being js1_8_5/extensions/file-mapped-arraybuffers.js and that test uses external files and a function disabled by default. I took the relevant parts out of this test.

http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/js/src/jsobj.cpp#939 This location isn't covered by any tests in either jstests or jit-tests. However, a test from the v8 test suite (mjsunit) covered this, so I decided to add both the original test (modified to work in our environment) and a reduced test that just hits this specific edge case.

http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/js/src/jsobj.cpp#2376 Again, this location isn't covered by any tests in either jstests or jit-tests. I found another test in the v8 test suite (mjsunit) that covers this and added both the original and a reduced testcase. It should be noted that the original test partly uses v8's native syntax, I wrote a script to rewrite all of these tests to remove this kind of syntax, potentially replacing it with something from our engine that is similar, if available.

Patch coming through mozreview.

Filing this bug s-s because it potentially points to a an area of our codebase that lacks the proper testing. We should only unhide this bug if the affected code is unlikely to be s-s or once we landed proper test support.

Comment 1

[Christian Holler (:decoder)]

Attachment: indexed-integer-exotics.js

So it turns out that I cannot land this because one of the test fails on our codebase. And the failure is not due to how I translated the test, but apparently rather a difference between our and v8's implementation.

Anba, can you help to figure out who is wrong here? The test (attached) or our implementation? It is probably a spec thing. You should be able to just run it with the JS shell. Thanks!

Comment 2

[Christian Holler (:decoder)]

Comment on attachment 8914730 [details]
indexed-integer-exotics.js

Nicely done Bugzilla. Anba, could you please check comment 1? Thanks!

Comment 3

[André Bargull [:anba]]

Comment on attachment 8914730 [details]
indexed-integer-exotics.js

(In reply to Christian Holler (:decoder) from comment #1)
> Anba, can you help to figure out who is wrong here? The test (attached) or
> our implementation? It is probably a spec thing. You should be able to just
> run it with the JS shell. Thanks!

The tests are correct from a spec POV, but they fail in SM, because we still don't implement CanonicalNumericIndexString for typed arrays (bug 1129202).

Comment 4

[Christian Holler (:decoder)]

Attachment: tests.patch

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8914788 [details] [diff] [review]
tests.patch

Review of attachment 8914788 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e406513e25d2