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)
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)
6.14 KB,
patch
|
nbp
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(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)
Assignee | ||
Comment 1•8 years ago
|
||
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.
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
Regression from bug 1283334. We should backport this to 51.
status-firefox49:
--- → unaffected
status-firefox50:
--- → unaffected
status-firefox51:
--- → affected
status-firefox-esr45:
--- → unaffected
tracking-firefox51:
--- → ?
tracking-firefox52:
--- → ?
Comment 4•8 years ago
|
||
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+
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
Comment 7•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee | ||
Comment 8•8 years ago
|
||
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 9•8 years ago
|
||
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+
Comment 10•8 years ago
|
||
bugherder uplift |
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•