904315 - BananaBread regression on Firefox 24 with IonMonkey

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alon Zakai (:azakai)]

kripken.github.io/BananaBread/cube2/game.html?medium,medium

fails in Firefox 24 (aurora) and later when IonMonkey is enabled. In earlier versions (23 beta or ealier), or with ion disabled, it works ok (reaches start screen with buttons to start the game). Also works in Chrome.

When it fails it reports various

[18:06:14.597] uncaught exception: [object Object]

that should not be happening.

Comment 1

[Luke Wagner [:luke]]

This seems to be in the FFI call'd Ion code: the error reproduces with asmjs = false and there is no error with asmjs = true, ion.content = false.

Comment 2

[Hannes Verschore [:h4writer]]

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5976b9c673f8&tochange=0888e29c83a3

Comment 3

[Hannes Verschore [:h4writer]]

Previous regression range was only temporary fail.

Updated regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=af4e3ce8c487&tochange=bf73e10f5e54

Also I can only reproduce FF25+.

Looking further.

Comment 4

[Hannes Verschore [:h4writer]]

Bug 894447 is the regressor. I tested it with running m-i browser build. That revision doesn't have a linux build but surrounding revision does and don't touch IonMonkey. So I'm pretty sure this is the regressor.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Hannes Verschore [:h4writer] from comment #4)
> Bug 894447 is the regressor.

It seems to work if I revert the change to jsinferinlines.h there. But that just removed a temporary workaround and shouldn't have introduced any new bugs.

Type information is probably a bit better now triggering this bug somehow..

Comment 6

[Jan de Mooij [:jandem]]

So the uncaught exception is this:

"No such file or directory : FS.ErrnoError@http://kripken.github.io/BananaBread/cube2/bb.js:1478
MEMFS.node_ops.lookup@http://kripken.github.io/BananaBread/cube2/bb.js:1340
FS.lookup@http://kripken.github.io/BananaBread/cube2/bb.js:2193
FS.lookupNode@http://kripken.github.io/BananaBread/cube2/bb.js:1518
FS.lookupPath@http://kripken.github.io/BananaBread/cube2/bb.js:1597
FS.mknod@http://kripken.github.io/BananaBread/cube2/bb.js:2195
FS.create@http://kripken.github.io/BananaBread/cube2/bb.js:2209
FS.createDataFile@http://kripken.github.io/BananaBread/cube2/bb.js:1840
finish@http://kripken.github.io/BananaBread/cube2/bb.js:2064
processData@http://kripken.github.io/BananaBread/cube2/bb.js:2080
FS.createPreloadedFile@http://kripken.github.io/BananaBread/cube2/bb.js:2088
filePreload36.onload/<@http://kripken.github.io/BananaBread/cube2/game/preload_medium.js:790
decrunchWorker.onmessage@http://kripken.github.io/BananaBread/cube2/game/preload_medium.js:7

Alon, would it be possible to trigger this in the shell somehow?

Comment 7

[Jan de Mooij [:jandem]]

The problem seems to be with the lookupNode function, bb.js:1506 When we Ion-compiling it we hit this bug..

lookupNode:function (parent, name) {
        var err = FS.mayLookup(parent);
        if (err) {
          throw new FS.ErrnoError(err);
        }
        var hash = FS.hashName(parent.id, name);
        for (var node = FS.name_table[hash]; node; node = node.name_next) {
          if (node.parent.id === parent.id && node.name === name) {
            return node;
          }
        }
        // if we failed to find it in the cache, call into the VFS
        return FS.lookup(parent, name);
      }

Comment 8

[Alon Zakai (:azakai)]

(In reply to Jan de Mooij [:jandem] from comment #6)
> 
> Alon, would it be possible to trigger this in the shell somehow?

Looks like it happens in the shell benchmark too. STR:

1. git clone https://github.com/kripken/BananaBread.git
2. cd BananaBread/cube2
3. git checkout gh-pages
4. js js/game-setup.js

With --no-ion it gets much further but eventually fails on "window.addEventListener is not a function" (missing from our shell support i guess), so that is the expected behavior.

Comment 9

[Jan de Mooij [:jandem]]

Git was still cloning the BB repo but I got a shell testcase...

If an object has a property like -50, Ion will use MLoadElementHole for GETELEM's on this object if the index is int32, but this is wrong because it will return |undefined| for a property like -50..

function g(o, idx, exp) {
    for (var i=0; i<3000; i++) {
	assertEq(o[idx], exp);
    }
}
function f() {
    var o = [];
    for (var i=1; i<100; i++) {
	o[-i] = 1;
    }
    g(o, 50, undefined);
    g(o, -50, 1);
}
f();

Comment 10

[Jan de Mooij [:jandem]]

Brian, can you take this? I think it's a regression from bug 827490, though it was probably harder to trigger without bug 893897.

The shell testcase in comment 9 should fail without any flags and passes with --no-ion.

CodeGenerator::visitInArray emits a negative-index check in some cases, we will probably need that here too?

Comment 11

[Jan de Mooij [:jandem]]

Setting tracking flags. Would be good to get this fixed on beta; it's pretty easy to trigger and breaks real-world websites.

Comment 12

[Brian Hackett [Laid off!]]

Attachment: patch

Blech, I noted in bug 861165 this was probably a problem but didn't do anything about it.

Comment 13

[Hannes Verschore [:h4writer]]

(In reply to Brian Hackett (:bhackett) from comment #12)
> Created attachment 793297 [details] [diff] [review]
> patch
> 
> Blech, I noted in bug 861165 this was probably a problem but didn't do
> anything about it.

comment 6 right? I had to reread that bug at least twice before I saw it. Sorry I didn't take action to fix it than and overlooked it

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 793297 [details] [diff] [review]
patch

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

Thanks!

This patch adds a new VM function + MIR/LIR operand but doesn't use them. I think bailing out is the right thing to do here if we can't guard statically, but would be good to add a bit on the baseline IC when we see a negative index and check for that in IonBuilder::getElemTryDense, just to be a bit more resilient when code does this.

Does this have any effect on benchmarks? IIRC we initially added LoadElementHole for Kraken ai-astar..

Comment 15

[Brian Hackett [Laid off!]]

Attachment: v2

I haven't tested this to see its effect on kraken, but it should be negligible.

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 793476 [details] [diff] [review]
v2

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

::: js/src/jit/BaselineIC.h
@@ +2805,5 @@
>      bool hasNonNativeAccess() const {
> +        return extra_ & EXTRA_NON_NATIVE;
> +    }
> +
> +    void noteNegativeIndex() {

Oh almost forgot, we don't call this function anywhere. I think we want to call it after this comment in BaselineIC.cpp, if rhs < 0:

// Check for NativeObject[int] dense accesses.

Comment 17

[Brian Hackett [Laid off!]]

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

Comment 18

[Brian Hackett [Laid off!]]

Comment on attachment 793476 [details] [diff] [review]
v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 827490
User impact if declined: potential incorrect behavior
Testing completed (on m-c, etc.): on m-i
Risk to taking this patch (and alternatives if risky): low

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

This has some non-trivial conflicts with Aurora.

Comment 21

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ad6bd7e6b791
https://hg.mozilla.org/releases/mozilla-beta/rev/785628a9603b

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Thanks!

Comment 23

[Tracy Walker [:tracy]]

verified with 24Beta10 and nightly builds from 20130913 on Nightly and Aurora.