912316 - Differential Testing: Different output message involving Proxy and __proto__

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

count = 0;
try {
    f2 = (function(a, a1, a) {
        count++;
        x = 1 % print('foo' + count);
        var r = a1 / 4
    });
    i1 = Iterator(1, true)
    a0 = []
    this.f2()
    e2 = x
    Object.defineProperty(a0, 9, {
        value: this.e2
    })
    i1.toSource = (function() {
        Array.prototype.sort.call(a0, (function() {}))
    })
    Object.defineProperty(a0, 15, {
        writable: 7,
    })
    Array.prototype.push.call(a0, i1)
    v = (this.v instanceof i1)
} catch (e) {}
try {
    Array.prototype.reverse.call(a0);
} catch (e) {}
try {
    i1.valueOf = (function() {
        f2(4 == 0)
    });
    Array(a0());
} catch (e) {}
try {
    Array.prototype.sort.call(a0, f2)
} catch (e) {}

prints "foo" 9 times on js opt shell on m-c changeset e3785e299ab6 without any CLI arguments, when the testcase is passed in as a CLI argument. However it prints "foo" only 8 times with --ion-eager.

My configure flags are:

--enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --enable-threadsafe <more NSPR compilation flags>

Comment 1

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

o1 = new Object
h1 = {}
f1 = Proxy.create(h1)
o2 = {}
v0 = new Number()
try {
    (function() {
        count = 0;
        h1.has = (function() {
            count++;
            print("foo" + count);
        })
    })()
    v0.valueOf = (function() {
        for (var j = 0; j < 9; j++) {
            o1.f()
        }
    })
    o2.__proto__ = f1
    o1.__proto__ = o2; + v0
} catch (e) {}

(this may be a related testcase)

$ js-opt-32-dm-ts-windows-e3785e299ab6.exe --fuzzing-safe 33450.js
foo1
foo2

$ js-opt-32-dm-ts-windows-e3785e299ab6.exe --fuzzing-safe --ion-eager 33450.js
foo1
foo2
foo3

Comment 2

[Jan de Mooij [:jandem]]

Gary I can't reproduce this with the testcase in comment 0; it only prints "foo1" and "foo2". It *looks* like it's a duplicate of bug 911369, but I can't reproduce it even without the patch for that bug.

I see the problem with the test in comment 1 though, I reduced it to:

o1 = {};
h1 = {};
proxy = Proxy.create(h1);
count = 0;
h1.has = (function() {
    print(count++);
});
function foo() {
    for (var j = 0; j < 9; j++) {
        o1.f();
    }
}
o2 = {};
o2.__proto__ = proxy;
o1.__proto__ = o2;
try {
    foo();
} catch (e) {}

Eric, would you mind taking this? It looks like it's a proxy issue; we're somehow calling h1.has 3 times with --ion-eager instead of twice.

Comment 3

[Eric Faust [:efaust]]

OK, I have a bead on this. The problem is that we have two stubs that independently call lookupGeneric() when trying to decide if they should attach, so if the first fails, and the second succeeds, we call twice when we should have only called once. Patch hopefully forthcoming.

Comment 4

[Eric Faust [:efaust]]

Attachment: Fix

OK, so the explanation above was almost right. I can't read GDB backtraces, apparently. The problem was that with the proxy on the proto chain, we weren't ensuring that a lookupProperty() call meant to be idempotent and diagnostic wouldn't call hooks in the proper fashion.

The attached patch moves away from ever calling JSObject::lookupProperty() in this case, and calls LookupPropertyPure() instead.

This also affected GetElementIC in the same way, and this patch also fixes that deficiency.

Comment 5

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

Since the bug morphed to the testcase in comment 1, I'm setting a needinfo on myself to retest the testcase in comment 0 after all these differential testing patches have landed.

Comment 6

[Kannan Vijayan [:djvj]]

Comment on attachment 799676 [details] [diff] [review]
Fix

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

Nice fix.

Comment 7

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97ee6915d2f4

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/97ee6915d2f4

Comment 9

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

(In reply to Jan de Mooij [:jandem] from comment #2)
> Gary I can't reproduce this with the testcase in comment 0; it only prints
> "foo1" and "foo2". It *looks* like it's a duplicate of bug 911369, but I
> can't reproduce it even without the patch for that bug.

I've tried to reproduce on m-c rev tip 34f431863037 on Windows 7, it seems to be fixed now. ("FOO" appears 9 times both with and without --ion-eager.) I've verified that the testcase in comment 2 is also fixed.