923765 - Differential Testing: Unsound caching with Proxy and __noSuchMethod__

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

count = 0
try {
    f1 = (function() {
            count++
            print(count + '-FOO')
    })
    h2 = {}
    h2.getPropertyDescriptor = f1
    p1 = this
    m0 = Proxy.create(h2, 0)
    f2 = (function() {
        m0.g()
    })
    f1 = (function() {
        f2()
    })
    h2.getOwnPropertyNames = f1
    v =
    (1 instanceof m0)
} catch (e) {}
try {
    __iterator__ = (function() {})
    m0 + ''
} catch (e) {}
try {
    for (p in p1) {}
} catch (e) {}
try {
    this instanceof m0
} catch (e) {}


prints the following in js opt shell on m-c changeset b5d24ef1eb37 with --ion-eager:

1-FOO
2-FOO
3-FOO
4-FOO
5-FOO
6-FOO
7-FOO
8-FOO
9-FOO
10-FOO
11-FOO
12-FOO
13-FOO

but prints the following without any CLI arguments:

1-FOO
2-FOO
3-FOO
4-FOO
5-FOO
6-FOO
7-FOO
8-FOO
9-FOO
10-FOO
11-FOO
12-FOO
13-FOO
14-FOO  <-- note this extra line

Due to skipped revisions, the first bad revision could be any of:
changeset:   http://hg.mozilla.org/mozilla-central/rev/1a9a72fbdc59
user:        Jason Orendorff
date:        Fri Sep 06 21:41:26 2013 -0500
summary:     Bug 913445 - Print something less confusing than "null" for non-stringifiable values in the shell. r=luke.

changeset:   http://hg.mozilla.org/mozilla-central/rev/5c2a0f1510bc
user:        Jason Orendorff
date:        Fri Sep 06 21:41:30 2013 -0500
summary:     Bug 895223, part 1 - Change perf/jsperf.cpp to use JSNative getters rather than PropertyOps. r=jandem.

(not sure what bug caused this)

My configure flags are:

CC="clang -Qunused-arguments" AR=ar CXX="clang++ -Qunused-arguments" sh ./configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>

Comment 1

[Jason Orendorff [:jorendorff]]

Only triggers with --enable-more-deterministic. Ironically enough.

In the default, less-deterministic build you only get *two* messages. That's because the less-deterministic build uses the expression decompiler to generate error messages, where the more-determinstic build uses .toSource() which triggers more traps.

Minimized test:

-------
var count = 0;
function trap(t, name) {
    count++;
    print(count + '-FOO ' + name);
}
var m0 = new Proxy({}, {get: trap, invoke: trap});
function f() {
    m0.g();
}
for (var i = 0; i < 4; i++) {
    try {
        f();
    } catch (e) {}
}
-------

Ion's caching something about __noSuchMethod__. Is this a DUP?

Comment 2

[Jan de Mooij [:jandem]]

Eric this seems to be a problem with GetPropertyIC::tryAttachGenericProxy: the problem goes away if we don't attach a stub there. Can you take a look? Thanks!

Comment 3

[Eric Faust [:efaust]]

This is no fun indeed. The problem is hard to come across, but a general purpose solution is annoying to come up with.

So, the problem, as Jason has already pointed out, is that once we generate a generic proxy stub, all proxy accesses will go through there. If that lookup is a CALLPROP, and if that lookup should lookup noSuchMethod, and if that lookup would happen to trigger hooks on a proxy, like this one, then you would see observable mistakes.

What's worse, if the lookup actually found an object and there was such a thing as noSuchMethod found that should be called, that would not happen at all.

In the past, the ICs have not added stubs if noSuchMethod was involved, and so were saved from this, but since Proxy gets are very opaque, from an IC point of view, it's really hard to know whether that's true.

The obvious fix is to make a little stub that does Proxy::get and a check for the need to call "OnUnknownMethod" for the proxy case. This constitutes some constant perf penalty on all cases, though, but is probably the least heinous thing I have come up with.

We could also consider more nuclear solutions, like banning proxy stubs if the site is a CALLPROP. This solution sounds, at first glance and without any benchmarking, like performance suicide.

An important point to note here is that it does not matter about the state of __noSuchMethod__ on the proto chain. The differential here comes from not invoking the side effects incurred from looking for it, rather than calling it.

CC djvj, as he's the other IC guru.

Comment 4

[Kannan Vijayan [:djvj]]

Even bug 916949 landing wouldn't resolve the issue here.  This is so exasperating.

Eric and I discussed this for a bit yesterday, and I spent some time thinking about it.  I think we can get correct behaviour, if we bend over backward in the exact right way and hold each ear with the opposite hand.

Step 1: Decide that the presence of any __noSuchMethod__ binding on any object anywhere in the runtime is justification to scale back on any CALLPROP or CALLELEM optimizations in both baseline and in Ion.  The first time we define __noSuchMethod__, we go and clear out all these optimized stubs and invalidate Ion code, and set a flag on the runtime so we don't generate such optimizations again.

Step 2: Have proxies be introspectable as to whether property lookups on them are guaranteed to be effectless.  E.g. proxies for which the handler object is a plain object with a plain proto chain (i.e. no effects on lookup of |has| or for |getOwnPropertyDescriptor| on the handler), and does not bind |has| nor |getOwnPropertyDescriptor| either directly or on a prototype, and the underlying target object is plain with a plain proto chain (i.e. no effects on lookup of |__noSuchMethod__|), then optimized accesses to the proxy can be allowed.  This is to handle the issue that efaust mentions at the end of comment 3 - whereby the side-effects of looking for __noSuchMethod__ are present.


All this said and done, this is a bunch of finnicky, tedious work that will take valuable developer time away from THINGS THAT ACTUALLY MATTER.  We really need to ask ourselves to what extent we're going to continue bending over backwards for this ball and chain of a "feature".  It's not even spec, for goodness' sake!

As we're going to start fuzzing more proxy stuff, we're going to run into this type of differential constantly.  I propose that we simply stop fuzzing differentials on builds with JS_HAS_NO_SUCH_METHOD defined.

Gary, decoder, what do you guys think?

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Kannan Vijayan [:djvj] from comment #4)
> Step 1: Decide that the presence of any __noSuchMethod__ binding on any
> object anywhere in the runtime is justification to scale back on any
> CALLPROP or CALLELEM optimizations in both baseline and in Ion.  The first
> time we define __noSuchMethod__, we go and clear out all these optimized
> stubs and invalidate Ion code, and set a flag on the runtime so we don't
> generate such optimizations again.

http://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&redirect=true&q=__noSuchMethod__

I don't think we want to throw away everything because of an addon based on the addon-sdk, the scratchpad, or the Console.

Comment 6

[Jason Orendorff [:jorendorff]]

efaust and I are discussing it on IRC right now.

First impression: Changing the semantics of __noSuchMethod__ to whatever is convenient for the ICs to implement would be a lot easier to swallow than allowing ICs to be non-transparent in some cases.

Really really don't like unsound optimizations. They're bad for more than testing.

Comment 7

[Jason Orendorff [:jorendorff]]

I think this can be fixed by adding a second stub function, Proxy::getMethod, just like Proxy::get but adding the __noSuchMethod__ behavior. That will cost one predictable branch (and it's right after a virtual method call through the handler).

If that's really true, djvj is tentatively OK with it. efaust is investigating.

Comment 8

[Eric Faust [:efaust]]

Attachment: Fix

grumble grumble.

Here's the fix Mentioned above.

There's now one extra call of overhead on the path, which is a little annoying.

Better would be to do the branch in jitcode, but IonCaches.cpp is really nor prepared for such noble sacrifices.

Comment 9

[Kannan Vijayan [:djvj]]

Comment on attachment 818771 [details] [diff] [review]
Fix

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

::: js/src/jit/IonCaches.cpp
@@ +1302,5 @@
>      return linkAndAttachStub(cx, masm, attacher, ion, "typed array length");
>  }
>  
>  static bool
> +proxy_callprop(JSContext *cx, HandleObject proxy, HandleObject receiver, HandleId id,

Nit: This seems like it might belong next to Proxy::get, and be called Proxy::callProp.

Comment 10

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9b3eac5b674

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e9b3eac5b674

Comment 12

[Jason Orendorff [:jorendorff]]

The extra call could probably be avoided, but anyway all this will have to be reshuffled when Proxy::invoke lands (bug 878605).

Comment 13

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

(In reply to Kannan Vijayan [:djvj] from comment #4)
> As we're going to start fuzzing more proxy stuff, we're going to run into
> this type of differential constantly.  I propose that we simply stop fuzzing
> differentials on builds with JS_HAS_NO_SUCH_METHOD defined.

I only run differential testing on js shells with --enable-more-deterministic, so anything to be ignored can be added to these builds.