1314545 - Differential Testing: Different output message involving Object.freeze

Status: RESOLVED FIXED

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

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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?

Comment 1

[Jan de Mooij [:jandem]]

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.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 3

[Jan de Mooij [:jandem]]

Regression from bug 1283334. We should backport this to 51.

Comment 4

[Nicolas B. Pierron [:nbp]]

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);

Comment 5

[Gerry Chang [:gchang]]

Per comment #3, track 51+ as regression.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c43332516cc
Fix Ion to handle stores to frozen elements correctly. r=nbp

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c43332516cc

Comment 8

[Jan de Mooij [:jandem]]

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.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8806783 [details] [diff] [review]
Patch

Test fixes for JS sandboxing, let's uplift to aurora (51)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/63b46b5db240