471373 - TM: OOM in imacro trips assert

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

function g() {
  var x = <x/>;
  for (var b = 0; b < 2; ++b) { 
    yield x; 
    for (var c = 0; c < 10;++c) { 
      x.r = x; 
    } 
  } 
}
for (let y in g()) { }

typein:7: out of memory
Assertion failure: (size_t)(regs.pc - script->code) < script->length, at ../jsinterp.cpp:6884

The OOM is expected, but the assertion failure is not.

Tested using Tracemonkey branch.  This assertion failure only occurs when the JIT is enabled, even though it is in jsinterp.cpp.

Comment 1

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

hg bisect reveals this changeset to have regressed the testcase in comment #0 :

The first bad revision is:
changeset:   21598:52536f3066ff
user:        Andreas Gal
date:        Thu Nov 13 15:58:58 2008 -0800
summary:     Don't flush JIT cache from within the recorder (464403, r=brendan).

Comment 2

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

This bug is a regression of (i.e. blocks) bug 464403, fixing wrong direction.

Comment 3

[Graydon Hoare :graydon]

I believe the cset implicated in comment #1 is a red herring (though it's true, the rev before doesn't fail in the same way on this case). I've applied its inverse locally to TM tip (with a little wiggling to make it stick) and am still tripping the assert. It also doesn't really look related: the assert involves falling off the end of a code segment in the interpreter. That's random memory corruption or an error in stack synthesis, I think.

I'll dig further.

Comment 4

[Graydon Hoare :graydon]

Minimality note: the following 1-line change is enough to provoke the crash, back at the pre-state of the implicated rev. Serves as a good jumping-off point for before/after comparison.

diff -r 5804f5597d3d js/src/jstracer.cpp
--- a/js/src/jstracer.cpp       Thu Nov 13 09:10:27 2008 -0800
+++ b/js/src/jstracer.cpp       Fri Mar 13 17:21:01 2009 -0700
@@ -2894,7 +2894,7 @@
 
     /* Make sure the global type map didn't change on us. */
     uint32 globalShape = OBJ_SHAPE(JS_GetGlobalForObject(cx, cx->fp->scopeChain));
-    if (tm->globalShape != globalShape) {
+    if (tm->globalSlots->length() && tm->globalShape != globalShape) {
         AUDIT(globalShapeMismatchAtEntry);
         debug_only_v(printf("Global shape mismatch (%u vs. %u) in RecordTree, flushing cache.\n",
                           globalShape, tm->globalShape);)

Comment 5

[Graydon Hoare :graydon]

I'm still going with "random memory corruption" though; we never even manage to *run* a trace in either case (neither working nor broken). They're all aborted no matter what.

Comment 6

[Graydon Hoare :graydon]

Attachment: Fix the bug, I think?

Ok, so thanks to the genius of gdb convenience vars and watchpoints (thank you jimb) I found the culprit; we're OOM'ing inside an imacro execution context, which of course makes pc be wildly off base.

The "simple" fix here is to include imacro-execution in the assert. I am not really sure if this is right. Possibly there is some further cleanup we have to do in the imacro case when exiting OOM? I assume the frame is not long for the world in such a condition, and we're basically just about to throw out everything related to the pc anyweay.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 367863 [details] [diff] [review]
Fix the bug, I think?

Sorry I missed this, it's so obvious from the if's condition just above. This is fine. Nit: you might use JS_ASSERT_IF(!fp->imacpc, ...). Or even

JS_ASSERT((fp->imacpc ? fp->imacpc : regs.pc) - script->code < script->length);

That would cover both cases.

/be

Comment 8

[Graydon Hoare :graydon]

http://hg.mozilla.org/tracemonkey/rev/81b61d9ca71c

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/81b61d9ca71c

Comment 10

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/5f59c245bbf1

/cvsroot/mozilla/js/tests/js1_8/regress/regress-471373.js,v  <--  regress-471373.js
initial revision: 1.1

Comment 11

[Robert Sayre]

fixed1.9.1 awhile back:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8ed2860342cb

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with testcase in comment 0 with the following debug builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090522 Minefield/3.6a1pre ID:20090522133810

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre)
Gecko/20090522 Shiretoko/3.5pre ID:20090522153422