1148973 - Differential Testing: Different output message involving Proxy

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

__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.

Comment 1

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

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?

Comment 2

[Boris Zbarsky [:bzbarsky]]

[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?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: 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....

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: 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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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...

Comment 6

[Jan de Mooij [:jandem]]

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.

Comment 7

[Jan de Mooij [:jandem]]

(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.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

The problem was bug 1146333, I'm pretty sure.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/538a0b9c118c

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/538a0b9c118c

Comment 13

[Sylvestre Ledru [:Sylvestre]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5869c83448f0

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d6ec30c02b8d