Differential Testing: Different output message involving Object.freeze

RESOLVED FIXED in Firefox 51

Status

()

Core
JavaScript Engine: JIT
P1
major
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: gkw, Assigned: jandem)

Tracking

(Blocks: 2 bugs, {testcase})

Trunk
mozilla52
x86_64
All
testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox49 unaffected, firefox-esr45 unaffected, firefox50 unaffected, firefox51+ fixed, firefox52+ fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
(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

a year 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

a year ago
Created attachment 8806783 [details] [diff] [review]
Patch

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

a year 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 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.
tracking-firefox51: ? → +
tracking-firefox52: ? → +

Comment 6

a year ago
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3c43332516cc
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Comment 8

a year 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 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

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/63b46b5db240
status-firefox51: affected → fixed
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.