Differential Testing: Different output message involving __proto__

RESOLVED FIXED in Firefox 39



JavaScript Engine: JIT
3 years ago
2 years ago


(Reporter: gkw, Assigned: jorendorff)


(Blocks: 2 bugs, {regression, testcase})

regression, testcase
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)



(1 attachment)



3 years ago
try {
    x = newGlobal();
    x.__proto__ = {};
    evaluate("s += ''", ({
        global: 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?
Flags: needinfo?(jorendorff)
Flags: needinfo?(efaustbmo)

Comment 1

3 years ago
This seems to be fixed. Bisecting to find what fixed it now.
Flags: needinfo?(jorendorff)

Comment 2

3 years ago
The first good revision is:
changeset:   231328:b83256f370d0
user:        Jan de Mooij <jdemooij@mozilla.com>
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

3 years ago
This looks like a real bug I introduced; Jan just made the code behave the same in the jits. Still looking into it.
Flags: needinfo?(efaustbmo) → needinfo?(jorendorff)

Comment 4

3 years ago
Created attachment 8572275 [details] [diff] [review]
Restore ReferenceError when a proxy is on the global object's prototype chain, regressed by rev 615f118f2787
Attachment #8572275 - Flags: review?(jdemooij)


3 years ago
Assignee: nobody → jorendorff


3 years ago
Flags: needinfo?(jorendorff)

Comment 5

3 years ago
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 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.
Attachment #8572275 - Flags: review?(jdemooij) → review+

Comment 9

3 years ago
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 11

3 years ago
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");
Last Resolved: 3 years ago
status-firefox39: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39

Comment 16

3 years ago
Can we please backport this to 38? It is the next ESR and this will help fuzzing on that ESR branch if need be.
Flags: needinfo?(jorendorff)


2 years ago
Flags: needinfo?(jorendorff)
You need to log in before you can comment on or make changes to this bug.