Closed
Bug 1137616
Opened 10 years ago
Closed 10 years ago
Differential Testing: Different output message involving __proto__
Categories
(Core :: JavaScript Engine: JIT, defect)
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)
Assignee | ||
Comment 1•10 years ago
|
||
This seems to be fixed. Bisecting to find what fixed it now.
Flags: needinfo?(jorendorff)
Assignee | ||
Comment 2•10 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
Assignee | ||
Comment 3•10 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)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8572275 -
Flags: review?(jdemooij)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(jorendorff)
Assignee | ||
Comment 5•10 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 6•10 years ago
|
||
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+
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 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>();
> }
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Comment 11•10 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");
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Comment 13•10 years ago
|
||
Assignee | ||
Comment 14•10 years ago
|
||
Comment 15•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Reporter | ||
Comment 16•10 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)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(jorendorff)
You need to log in
before you can comment on or make changes to this bug.
Description
•