Closed Bug 1137616 Opened 10 years ago Closed 10 years ago

Differential Testing: Different output message involving __proto__

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: gkw, Assigned: jorendorff)

References

Details

(Keywords: regression, testcase)

Attachments

(1 file)

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?
Flags: needinfo?(jorendorff)
Flags: needinfo?(efaustbmo)
This seems to be fixed. Bisecting to find what fixed it now.
Flags: needinfo?(jorendorff)
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
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)
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Flags: needinfo?(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 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+
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>(); > }
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");
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
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)
Flags: needinfo?(jorendorff)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: