Closed Bug 1405285 Opened 2 years ago Closed 2 years ago

Add JS tests for some edge cases in jsarray/jsobj

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: decoder, Assigned: decoder)

References

(Blocks 1 open bug)

Details

(Keywords: sec-want, Whiteboard: [adv-main58-][post-critsmash-triage])

Attachments

(1 file, 1 obsolete file)

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.
Attached file indexed-integer-exotics.js (obsolete) —
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!
Attachment #8914730 - Flags: feedback?
Comment on attachment 8914730 [details]
indexed-integer-exotics.js

Nicely done Bugzilla. Anba, could you please check comment 1? Thanks!
Attachment #8914730 - Flags: feedback? → feedback?(andrebargull)
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).
Attachment #8914730 - Flags: feedback?(andrebargull)
Attached patch tests.patchSplinter Review
Assignee: nobody → choller
Attachment #8914730 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8914788 - Flags: review?(jdemooij)
Comment on attachment 8914788 [details] [diff] [review]
tests.patch

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

LGTM.
Attachment #8914788 - Flags: review?(jdemooij) → review+
Group: core-security → javascript-core-security
https://hg.mozilla.org/mozilla-central/rev/e406513e25d2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Group: javascript-core-security → core-security-release
Whiteboard: [adv-main58-]
Flags: qe-verify-
Whiteboard: [adv-main58-] → [adv-main58-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.