1137616 - Differential Testing: Different output message involving __proto__

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

try {
    x = newGlobal();
    x.__proto__ = {};
    evaluate("s += ''", ({
        global: x
    }));
    print(x);
} catch (e) {}

$ ./js-32-prof-dm-nsprBuild-linux-5f1009731a97 --fuzzing-safe --no-threads --ion-eager testcase.js
$

$ ./js-32-prof-dm-nsprBuild-linux-5f1009731a97 --fuzzing-safe --no-threads --no-baseline --no-ion testcase.js
[object global]

Tested this on m-c rev 5f1009731a97.

My configure flags are:

CXX="g++ -m32 -msse2 -mfpmath=sse" CC="gcc -m32 -msse2 -mfpmath=sse" AR=ar PKG_CONFIG_LIBDIR=/usr/lib/pkgconfig sh ~/trees/mozilla-central/js/src/configure --target=i686-pc-linux --enable-profiling --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/fuzzing/js/compileShell.py -b "--32 --enable-profiling --enable-nspr-build --enable-more-deterministic -R ~/trees/mozilla-central" -r 5f1009731a97

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/615f118f2787
user:        Jason Orendorff
date:        Tue Dec 16 18:06:43 2014 -0600
summary:     Bug 914314, part 3 - Reimplement GetPropertyInline to match ES6 9.1.8. r=efaust.

Jason/Eric, is bug 914314 a likely regressor?

Comment 1

[Jason Orendorff [:jorendorff]]

This seems to be fixed. Bisecting to find what fixed it now.

Comment 2

[Jason Orendorff [:jorendorff]]

The first good revision is:
changeset:   231328:b83256f370d0
user:        Jan de Mooij <[e-mail]>
date:        Sat Feb 21 20:20:44 2015 +0100
summary:     Bug 1106982 - Stop doing script/pc lookup in GetNonexistentProperty if extra warnings are disabled. r=jorendorff

Comment 3

[Jason Orendorff [:jorendorff]]

This looks like a real bug I introduced; Jan just made the code behave the same in the jits. Still looking into it.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Restore ReferenceError when a proxy is on the global object's prototype chain, regressed by rev 615f118f2787

Comment 5

[Jason Orendorff [:jorendorff]]

This suggests a different approach:

1. Implement a GetPropertyIfPresent operation that's tuned for the case where the operand is a native object and the property can be gotten without any funny business (hooks, GC, getters). At the first sign of trouble, it simply falls back on HasProperty/GetProperty.

2. Use that instead of GetProperty where appropriate to implement stuff like JSOP_GETNAME.

3. Remove everything to do with name lookups from GetProperty, so that GetProperty is nothing more or less than ES6 [[Get]].

If I had an extra month in Q1 I'd do it. :-\

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8572275 [details] [diff] [review]
Restore ReferenceError when a proxy is on the global object's prototype chain, regressed by rev 615f118f2787

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

Nice refactoring.

I didn't realize bug 1106982 fixed a JIT correctness issue, good.

Comment 7

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=ae1fc7678109

Comment 8

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=ecdcd359b70d

Comment 9

[Jason Orendorff [:jorendorff]]

Argh, the patch attached here has a silly bug, caused by copy-and-paste of a single line of code. Here's the fix:

>         // Step 4.d. If the prototype is also native, this step is a
>         // recursive tail call, and we don't need to go through all the
>         // plumbing of JSObject::getGeneric; the top of the loop is where
>         // we're going to end up anyway. But if pobj is non-native,
>         // that optimization would be incorrect.
>-       if (obj->getOps()->getProperty)
>+       if (proto->getOps()->getProperty)
>            return GeneralizedGetProperty(cx, proto, id, receiver, nameLookup, vp);
> 
>         pobj = &proto->as<NativeObject>();
>     }

Comment 10

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=480d64067b30

Comment 11

[Jason Orendorff [:jorendorff]]

OK, one more funny change I have to make here: I posted a test case in comment 1106982 comment 2, with the comment "Current behavior is correct except I think ES6 says we have to call "has" twice before calling "set"; currently we only call it once."

Well, with this patch we actually *do* call the "has" trap twice, which causes my stupid test to error out. D'oh. Easily fixed.

>diff --git a/js/src/jit-test/tests/basic/bug1106982.js b/js/src/jit-test/tests/basic/bug1106982.js
>--- a/js/src/jit-test/tests/basic/bug1106982.js
>+++ b/js/src/jit-test/tests/basic/bug1106982.js
>@@ -1,13 +1,16 @@
> var x = "wrong";
> var t = {x: "x"};
>+var hits = 0;
> var p = new Proxy(t, {
>     has(t, id) {
>         var found = id in t;
>-        delete t[id];
>+        if (++hits == 2)
>+            delete t[id];
>         return found;
>     },
>     get(t, id) { return t[id]; }
> });
> with (p)
>     x += " x";
>+assertEq(hits, 2);
> assertEq(t.x, "undefined x");

Comment 12

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=537df066e1a3

Comment 13

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=a8fddfdf4bcd

Comment 14

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ca34e768b42

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ca34e768b42

Comment 16

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

Can we please backport this to 38? It is the next ESR and this will help fuzzing on that ESR branch if need be.