Closed Bug 1314545 Opened 8 years ago Closed 8 years ago

Differential Testing: Different output message involving Object.freeze

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

x86_64
All
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- unaffected
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 + fixed
firefox52 + fixed

People

(Reporter: gkw, Assigned: jandem)

References

Details

(Keywords: testcase)

Attachments

(1 file)

(function() { for (let y of[1, , 0]) { this[0] = y; Object.freeze(this); for (var z in this) {}; } })() print(uneval(this)); $ ./js-dbg-64-dm-linux-969c3295d3aa --fuzzing-safe --no-threads --ion-eager testcase.js /snip }, wasmTextToBinary:function wasmTextToBinary() { [native code] }, 0:0}) $ ./js-dbg-64-dm-linux-969c3295d3aa/js-dbg-64-dm-linux-969c3295d3aa --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js /snip }, wasmTextToBinary:function wasmTextToBinary() { [native code] }, 0:1}) Tested this on m-c rev 969c3295d3aa. My configure flags are: AR=ar sh ./configure --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic" -r 969c3295d3aa autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/f95b25ae990d user: Leo Gaspard date: Mon Aug 29 15:00:35 2016 -0700 summary: Bug 1283334 - Part 1: Do not sparsify dense arrays when freezing - Interpreter. r=jandem Jan, is bug 1283334 a likely regressor?
Flags: needinfo?(jdemooij)
Here's a simpler testcase that doesn't require uneval or a more-deterministic build: for (let y of [1, , 0]) { this[0] = y; Object.freeze(this); for (var i=0; i<10; i++) {} } assertEq(this[0], 1); Fails with --ion-eager --no-threads.
Attached patch PatchSplinter Review
The code in IonBuilder::jsop_setelem_dense is wrong for frozen objects when there are indexed properties on the prototype chain: we just fall back to an unchecked StoreElement! The patch adds a check for this and makes us fall back to an IC for now. It's a rare case and we will have to backport this.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8806783 - Flags: review?(nicolas.b.pierron)
Regression from bug 1283334. We should backport this to 51.
Comment on attachment 8806783 [details] [diff] [review] Patch Review of attachment 8806783 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonBuilder.cpp @@ +10394,5 @@ > + if (!jsop_setelem_dense(conversion, object, index, value, unboxedType, writeHole, emitted)) > + return false; > + > + if (!*emitted) > + return true; nit: Add trackOptimizationOutcome(TrackedOutcome::NonWritableProperty);
Attachment #8806783 - Flags: review?(nicolas.b.pierron) → review+
Per comment #3, track 51+ as regression.
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3c43332516cc Fix Ion to handle stores to frozen elements correctly. r=nbp
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8806783 [details] [diff] [review] Patch Approval Request Comment [Feature/regressing bug #]: Bug 1283334. [User impact if declined]: Correctness issue, might also affect security of JS sandboxes. [Describe test coverage new/current, TreeHerder]: Fixes tests, on m-c for a few days. [Risks and why]: Low risk, this just adds an extra check. [String/UUID change made/needed]: None.
Attachment #8806783 - Flags: approval-mozilla-aurora?
Comment on attachment 8806783 [details] [diff] [review] Patch Test fixes for JS sandboxing, let's uplift to aurora (51)
Attachment #8806783 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: