Differential Testing: Different output message involving Proxy

RESOLVED FIXED in Firefox 38

Status

()

--
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gkw, Assigned: bzbarsky)

Tracking

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

Trunk
mozilla40
x86_64
Mac OS X
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed, firefox40 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
__defineGetter__("x", decodeURI)
try {
    print(b = Proxy.createFunction(function() {
        return {
            get: function(r, z) {
                return x[z]
            }
        }
    }(), function() {}))
} catch (e) {};
try {
    function x() {}
    print(b)
} catch (e) {}

$ ./js-dbg-64-dm-nsprBuild-darwin-385840329d91 --fuzzing-safe --no-threads --ion-eager testcase.js

$ ./js-dbg-64-dm-nsprBuild-darwin-385840329d91 --fuzzing-safe --no-threads --baseline-eager testcase.js
function () {}

Tested this on m-c rev 385840329d91.

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -r 385840329d91

autoBisect is running.
(Reporter)

Comment 1

4 years ago
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/4247f2646a63
user:        Boris Zbarsky
date:        Tue Dec 09 14:44:38 2014 -0500
summary:     Bug 1100757. Don't emit a guard in testCommonGetterSetter when our getter/setter is is a non-configurable property on a native object.  r=efaust

Boris, is bug 1100757 a likely regressor?
Blocks: 1100757
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 2

4 years ago
[Tracking Requested - why for this release]:

Seems likely, yes.

So in that testcase, the first print() call ends up getting x as a string, so x[z] where z is "toString" is String.prototype.toString, and this is invoked on a proxy, which is not going to go well; so that throws.

When we get to the second print() call, what should have happened is that x is now a function, so x[z] is Function.prototype.toString so it should work.

With --no-threads --ion-eager in that testcase we get String.prototype.toString in both cases.

So what I see happening in that case is that we run the proxy get hook once in interp, _then_ we ion-compile it when we run it a second time.  At this point, "x" is not an accessor property on the global anymore.  But we're calling commonGetPropFunction which gets the information about whether we have a getter from the baseline IC, which of course has a getter.  So then we land in IonBuilder::testCommonGetterSetter which looks up the name, finds a non-configurable property, and doesn't add the shape guard.

The key here is that we need to be able to reshape the object after we've created a baseline IC but before we've ion-compiled it.  I'm not sure how to write a testcase for this, unfortunately.  Do we run part of jit-tests with --no-threads --ion-eager?
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 3

4 years ago
Created attachment 8585553 [details] [diff] [review]
When skipping shape guards in Ion common getter/setter code because the object has a non-configurable property, first verify that its current shape matches the shape we're using to compile our code

I tried to create a simpler testcase but couldn't seem to create one where we didn't fail some other guard, for some reason....
Attachment #8585553 - Flags: review?(jdemooij)
(Assignee)

Updated

4 years ago
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
(Assignee)

Comment 4

4 years ago
Created attachment 8585562 [details] [diff] [review]
When skipping shape guards in Ion common getter/setter code because the object has a non-configurable property, first verify that its current shape matches the shape we're using to compile our code

Finally figured out how to write the simpler test.
Attachment #8585562 - Flags: review?(jdemooij)
(Assignee)

Updated

4 years ago
Attachment #8585553 - Attachment is obsolete: true
Attachment #8585553 - Flags: review?(jdemooij)
(Assignee)

Comment 5

4 years ago
Oh, the other question is where we should backport this too.  Seems like at least beta and aurora... and we're about to ship 37 with this bug.  Not sure it's worth backporting to 37, though; the need to modify the object between the baseline run and the ion compile means it's not likely to be hit in practice...
Flags: needinfo?(jdemooij)
Comment on attachment 8585562 [details] [diff] [review]
When skipping shape guards in Ion common getter/setter code because the object has a non-configurable property, first verify that its current shape matches the shape we're using to compile our code

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

Good catch and nice asserts.
Attachment #8585562 - Flags: review?(jdemooij) → review+
(In reply to Not doing reviews right now from comment #5)
> Oh, the other question is where we should backport this too.  Seems like at
> least beta and aurora... and we're about to ship 37 with this bug.  Not sure
> it's worth backporting to 37, though; the need to modify the object between
> the baseline run and the ion compile means it's not likely to be hit in
> practice...

Agreed, too late for 37 but it probably doesn't matter.
Flags: needinfo?(jdemooij)
Bug 1144397, bug 1146333, and bug 1148973 backed out for widespread test bustage:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=501a8f687a91
(the Linux opt build failures were from glandium's prior push, the rest is bz)

   https://hg.mozilla.org/integration/mozilla-inbound/rev/623e6b3114de
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 9

4 years ago
The problem was bug 1146333, I'm pretty sure.
Comment on attachment 8585562 [details] [diff] [review]
When skipping shape guards in Ion common getter/setter code because the object has a non-configurable property, first verify that its current shape matches the shape we're using to compile our code

Approval Request Comment
[Feature/regressing bug #]: Bug 1100757 
[User impact if declined]: Some sites might in some very rare circumstances not
   work correctly due to getting incorrect values for properties in JS.
[Describe test coverage new/current, TreeHerder]: Reasonable-ish coverage.
[Risks and why]: I think this is very low risk.  It just turns off an
   optimization that we didn't make until Firefox 37 in some cases in which it's
   not safe to make.
[String/UUID change made/needed]: None.
Attachment #8585562 - Flags: approval-mozilla-beta?
Attachment #8585562 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/538a0b9c118c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment on attachment 8585562 [details] [diff] [review]
When skipping shape guards in Ion common getter/setter code because the object has a non-configurable property, first verify that its current shape matches the shape we're using to compile our code

should be part of 38b2
Attachment #8585562 - Flags: approval-mozilla-beta?
Attachment #8585562 - Flags: approval-mozilla-beta+
Attachment #8585562 - Flags: approval-mozilla-aurora?
Attachment #8585562 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.