1397026 - Differential Testing: Different output message involving Object.defineProperty

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

function f(x) {
    Object.defineProperty(this, "setUint8", ({
        value: true,
        writable: true,
        configurable: true,
        enumerable: true
    }));
    if (x) {
        Object.seal(this);
    }
}
for (let z of [0, /x/, 0, 0]) {
    try {
        f(z);
    } catch (e) {
        print("FOO");
    }
}


$ ./js-dbg-64-dm-linux-0afabd3e5c27 --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js
FOO

$ ./js-dbg-64-dm-linux-0afabd3e5c27 --fuzzing-safe --no-threads --ion-eager testcase.js
FOO
FOO


Tested this on m-c rev 0afabd3e5c27.

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 0afabd3e5c27

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/a14fc8f5babc
user:        André Bargull
date:        Mon Aug 28 14:19:34 2017 +0200
summary:     Bug 1303335: Move parts of Object.getOwnProperty and Object.defineProperty to self-hosted code. r=till

Andre, is bug 1303335 a likely regressor?

Comment 1

[André Bargull [:anba]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> autoBisect shows this is probably related to the following changeset:
> 
> The first bad revision is:
> changeset:   https://hg.mozilla.org/mozilla-central/rev/a14fc8f5babc
> user:        André Bargull
> date:        Mon Aug 28 14:19:34 2017 +0200
> summary:     Bug 1303335: Move parts of Object.getOwnProperty and
> Object.defineProperty to self-hosted code. r=till
> 
> Andre, is bug 1303335 a likely regressor?

Yes.

Slightly reduced test case:
---
var o = {};
for (var i = 0; i < 4; ++i) {
    var desc = {
        value: i,
        writable: true,
        configurable: true,
        enumerable: true
    };
    try {
        Object.defineProperty(o, "p", desc);
    } catch (e) {
    }
    if (i === 1) {
        Object.defineProperty(o, "p", {configurable: false});
    }
    print(o.p);
}
---

Expected: Prints "0 1 1 1"
Actual: Prints "0 1 1 3"


Jan, it looks _DefineDataProperty, when optimized as INIT_ELEM, doesn't actually perform [[DefineOwnProperty]], but merely [[Set]]. Is this reasoning correct and how can we fix it to perform a proper [[DefineOwnProperty]]?

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

Good catch. This fixes it and is the only place that needs fixing AFAICS.

Comment 3

[André Bargull [:anba]]

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

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

There is still an issue where [[Enumerable]] is not set correctly.


Test case for [[Enumerable]], still not passing (with --baseline-eager).
Expected: Prints "true" four times.
Actual: Prints "true" three times and then "false"
---
var o = {};
for (var i = 0; i < 4; ++i) {
    var desc = {
        value: i,
        writable: true,
        configurable: true,
        enumerable: true
    };
    try {
        Object.defineProperty(o, "p", desc);
    } catch (e) {
    }
    print(Object.getOwnPropertyDescriptor(o, "p").enumerable);
    if (i > 0) {
        Object.defineProperty(o, "p", {enumerable: false});
    }
}
---



Test case which failed before bug 1303335 (passes with the attached patch):
---
var currentOptions;
var options = {
    get month() {
        currentOptions = this;
        currentOptions.year = "two-digit";
        if (i > 5) {
            Object.defineProperty(currentOptions, "year", {configurable: false});
        }
    }
};
var date = new Date();
for (var i = 0; i < 10; ++i) {
    try {
        date.toLocaleString(undefined, options);
    } catch (e) {}
    print(currentOptions.year);
}
---

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

How about this one? We check for configurable/writable/enumerable now so I think that should cover everything?

Comment 5

[André Bargull [:anba]]

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

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

Yes, that should work. Thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/509c46b010b6
Make sure JSOP_INIT* IC behavior matches [[DefineOwnProperty]] instead of [[Set]]. r=anba

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/509c46b010b6