Closed
Bug 904315
Opened 11 years ago
Closed 11 years ago
BananaBread regression on Firefox 24 with IonMonkey
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla26
People
(Reporter: azakai, Assigned: bhackett1024)
References
()
Details
(Keywords: verifyme)
Attachments
(1 file, 1 obsolete file)
9.20 KB,
patch
|
jandem
:
review+
bajaj
:
approval-mozilla-aurora+
bajaj
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•11 years ago
|
Reporter | ||
Updated•11 years ago
|
![]() |
||
Comment 1•11 years ago
|
||
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•11 years ago
|
||
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5976b9c673f8&tochange=0888e29c83a3
Comment 3•11 years ago
|
||
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.
Version: 24 Branch → 25 Branch
Comment 4•11 years ago
|
||
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.
Depends on: 894447
Updated•11 years ago
|
Flags: needinfo?(jdemooij)
Comment 5•11 years ago
|
||
(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•11 years ago
|
||
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?
Flags: needinfo?(jdemooij)
Updated•11 years ago
|
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Comment 7•11 years ago
|
||
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); }
Reporter | ||
Comment 8•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
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?
Assignee: jdemooij → bhackett1024
Flags: needinfo?(bhackett1024)
Comment 11•11 years ago
|
||
Setting tracking flags. Would be good to get this fixed on beta; it's pretty easy to trigger and breaks real-world websites.
status-firefox24:
--- → affected
status-firefox25:
--- → affected
status-firefox26:
--- → affected
tracking-firefox24:
--- → ?
tracking-firefox25:
--- → ?
tracking-firefox26:
--- → ?
Updated•11 years ago
|
status-firefox23:
--- → unaffected
Assignee | ||
Comment 12•11 years ago
|
||
Blech, I noted in bug 861165 this was probably a problem but didn't do anything about it.
Attachment #793297 -
Flags: review?(jdemooij)
Flags: needinfo?(bhackett1024)
Comment 13•11 years ago
|
||
(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•11 years ago
|
||
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..
Attachment #793297 -
Flags: review?(jdemooij)
Assignee | ||
Comment 15•11 years ago
|
||
I haven't tested this to see its effect on kraken, but it should be negligible.
Attachment #793297 -
Attachment is obsolete: true
Attachment #793476 -
Flags: review?(jdemooij)
Updated•11 years ago
|
Attachment #793476 -
Flags: review?(jdemooij) → review+
Comment 16•11 years ago
|
||
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.
Assignee | ||
Comment 17•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbf11d4d27e6
Assignee | ||
Comment 18•11 years ago
|
||
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
Attachment #793476 -
Flags: approval-mozilla-beta?
Attachment #793476 -
Flags: approval-mozilla-aurora?
Comment 19•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fbf11d4d27e6
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Updated•11 years ago
|
Attachment #793476 -
Flags: approval-mozilla-beta?
Attachment #793476 -
Flags: approval-mozilla-beta+
Attachment #793476 -
Flags: approval-mozilla-aurora?
Attachment #793476 -
Flags: approval-mozilla-aurora+
Updated•11 years ago
|
Keywords: checkin-needed
Comment 20•11 years ago
|
||
This has some non-trivial conflicts with Aurora.
Flags: needinfo?(bhackett1024)
Keywords: checkin-needed → branch-patch-needed
Assignee | ||
Comment 21•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/ad6bd7e6b791 https://hg.mozilla.org/releases/mozilla-beta/rev/785628a9603b
Flags: needinfo?(bhackett1024)
Comment 23•11 years ago
|
||
verified with 24Beta10 and nightly builds from 20130913 on Nightly and Aurora.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•