942480 - Crash [@ js::gc::Cell::runtimeFromAnyThread] or Assertion failure: table, at dist/include/js/HashTable.h or Assertion failure: object->runtimeFromMainThread()->isHeapBusy(), at vm/Debugger.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stacks

x = ParallelArray([0]);
x.shape[0] = Infinity;
x.filter(function(){ new Debugger });

asserts 32-bit js debug shell on m-c changeset ad6589ed742c without any CLI arguments at Assertion failure: table, at dist/include/js/HashTable.h or Assertion failure: object->runtimeFromMainThread()->isHeapBusy(), at vm/Debugger.cpp

It also crashes 32-bit js opt shell at js::gc::Cell::runtimeFromAnyThread

64-bit seems unaffected.

This seems to be involving the Debugger object, probably an OOM, but due to the presence of GC and the varying assertions, I'm locking it s-s for the moment.

jimb, any idea what might be going on here?

My configure flags are:

Debug:

LD=ld CROSS_COMPILE=1 CXX="clang++ -Qunused-arguments -arch i386" RANLIB=ranlib CC="clang -Qunused-arguments -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments" HOST_CXX="clang++ -Qunused-arguments" sh ./configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --with-ccache --enable-threadsafe <other NSPR flags>

Opt:

LD=ld CROSS_COMPILE=1 CXX="clang++ -Qunused-arguments -arch i386" RANLIB=ranlib CC="clang -Qunused-arguments -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments" HOST_CXX="clang++ -Qunused-arguments" sh ./configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --with-ccache --enable-threadsafe <other NSPR flags>

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:   http://hg.mozilla.org/mozilla-central/rev/77bcbb164b45
user:        Dan Gohman
date:        Fri Aug 16 14:40:50 2013 -0700
summary:     Bug 894813 - IonMonkey: Eliminate ambiguity in pow calls in a CLOSED TREE

Actually, bug 894813 may be a regressor instead (not sure), so switching over to Dan.

Comment 2

[Dan Gohman [:sunfish]]

I can reproduce the "object->runtimeFromMainThread()->isHeapBusy()" assertion failure on 32-bit Linux.

I can reproduce it with --ion-range-analysis=off, so it's probably unrelated to bug 894813 or range analysis in general.

I poked around a little:

The "Assertion failure: table" abort is coming from the "JS_ASSERT(debuggees.empty())" in ~Debugger(). It should perhaps be "JS_ASSERT_IF(debuggees.initialized(), debuggees.empty());", because if the debugger's init function fails due to OOM, it may leave the debuggees table in its "uninitialized" state. Same thing for the similar assert in Debugger::finalize.

The "object->runtimeFromMainThread()->isHeapBusy()" assertion has a comment that says "This always happens in the GC thread, so no locking is required." However, when it fails, the debugger is being called from Debugger::construct because of an OOM failure, which presumably isn't the normal GC case. If that assertion is disabled, the program proceeds into what looks like a double free: the Debugger object which had its init fail due to OOM was deleted explicitly, and later it has its finalizer called from the GC.

Since this looks like a double free, security-sensitive looks appropriate, probably sec-critical. It's not clear when the bug was first introduced; I didn't see anything recent at a quick glance. Someone familiar with the Debugger and/or the GC should look at this.

Comment 3

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

> Since this looks like a double free, security-sensitive looks appropriate,
> probably sec-critical. It's not clear when the bug was first introduced; I
> didn't see anything recent at a quick glance. Someone familiar with the
> Debugger and/or the GC should look at this.

CC'ing some Debugger/GC folks, and needinfo'ing a handful.

Comment 4

[Terrence Cole [:terrence]]

It looks like creating a Debugger object from PJS fails (correctly), but leaves the debugger present in an invalid state. Niko, Shu, could one of you take a look?

Comment 5

[Niko Matsakis [:nmatsakis]]

For debugger, I think it has to be shu.

Comment 6

[Andrew McCreight [:mccr8]]

Marking sec-critical per comment 2.

Comment 7

[Shu-yu Guo [:shu]]

Attachment: Fix OOM handling in Debugger construction.

This doesn't really have anything to do with ParallelArray, this is just an OOM
bug.

~Debugger can be called from GC or during OOM, and it currently incorrectly
assumes it's always called from GC. Jim, I'm not sure what the comment above
isHeapBusy() is talking about in terms of racing. AFAICT it's not possible for
~Debugger to race on the runtime-wide linked lists.

Gary, with this patch I get an uncaught OOM exception, but OS X 10.9 also
starts barfing messages like:

js(37743,0xa19be1a8) malloc: *** mach_vm_map(size=8388608) failed (error code=3)
*** error: can't allocate region
*** set a breakpoint in malloc_error_break to debug

over and over and over and over. I suppose that means it's running of VM space
on 32bit. Probably intended behavior, since we are OOMing.

Comment 8

[Shu-yu Guo [:shu]]

That patch doesn't fix all the crashes. It's running out of VM space and it's still crashing in the HashTable destructors. Not sure why -- maybe heap and stack become overlapped?

Comment 9

[Shu-yu Guo [:shu]]

Some further investigation reveals that we basically allocate Debuggers forever until we run out of VM space, at which point we start unwinding, destructing the millions of Debuggers we've allocated.

We don't get very far in trying to destruct the millions of Debugger, though, until we hit a crash. I'm betting that we run out of stack space and bad things start to happen, since I got this in lldb:

  (lldb) p e
  error: Couldn't allocate space for the stack frame: Couldn't malloc: address space is full
  Errored out in Execute, couldn't PrepareToExecuteJITExpression

If that is the case (we're running out of stack while running in the destructor), I know how to fix this. Ideas, Jim?

Comment 10

[Shu-yu Guo [:shu]]

err, left out an operative word there: I *don't* know how to fix this.

Comment 11

[Shu-yu Guo [:shu]]

Attachment: Reduced testcase

Comment 12

[Shu-yu Guo [:shu]]

Attachment: Fix some OOM handling in Debugger construction.

Remove duplicate assert.

Comment 13

[Shu-yu Guo [:shu]]

Maybe make an upper limit on how many Debuggers can be made so we don't blow the C stack?

Comment 14

[Shu-yu Guo [:shu]]

Also, I have a hard time reproducing the OOM in an optimized build, but it happens readily on debug.

Comment 15

[Shu-yu Guo [:shu]]

(maybe something about inlined frames, less stack)

Comment 16

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Error: Failed to compile specified revision ad6589ed742c (maybe try another?)

Comment 17

[Jim Blandy :jimb]

That assertion in ~Debugger that the heap is busy is obviously incorrect in the OOM case; it should go.

The assertion in ~Debugger that the 'debuggees' table is empty should be protected by a 'debuggees.initialized()' check.

But we really should find out why those Debuggers can't be destroyed nicely. I have no idea.

Comment 18

[Jason Orendorff [:jorendorff]]

(In reply to Shu-yu Guo [:shu] from comment #9)
> millions of Debuggers

This bug is so awesome.

Comment 19

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #17)
> That assertion in ~Debugger that the heap is busy is obviously incorrect in
> the OOM case; it should go.
> 
> The assertion in ~Debugger that the 'debuggees' table is empty should be
> protected by a 'debuggees.initialized()' check.
> 
> But we really should find out why those Debuggers can't be destroyed nicely.
> I have no idea.

Yes, that is in fact what the patch does. :)

Comment 20

[Jim Blandy :jimb]

Comment on attachment 8341535 [details] [diff] [review]
Fix some OOM handling in Debugger construction.

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

Why, so it is!

Comment 21

[Jim Blandy :jimb]

(In reply to Shu-yu Guo [:shu] from comment #9)
> Some further investigation reveals that we basically allocate Debuggers
> forever until we run out of VM space, at which point we start unwinding,
> destructing the millions of Debuggers we've allocated.
>
> We don't get very far in trying to destruct the millions of Debugger,
> though, until we hit a crash. I'm betting that we run out of stack space and
> bad things start to happen, since I got this in lldb:
> 
>   (lldb) p e
>   error: Couldn't allocate space for the stack frame: Couldn't malloc:
> address space is full
>   Errored out in Execute, couldn't PrepareToExecuteJITExpression
> 
> If that is the case (we're running out of stack while running in the
> destructor), I know how to fix this. Ideas, Jim?

That error message, "couldn't PrepareToExecuteJITExpression", is an lldb error message, not a SpiderMonkey error message. So it's just saying that LLDB itself can't get the memory it wants.

Can you make it crash in an interesting way, with your patch applied, if you do something like this?

$ (ulimit -v 4000000; obj~/js -f ~/moz/cool-bug.js)

The parens make bash run the command in a subshell, so the ulimit doesn't affect your main shell, only the JS shell invocation.)

Comment 22

[Jim Blandy :jimb]

Ah, okay, I'm able to reproduce a double-free. The problem is that Debugger::construct stores the pointer to the js::Debugger in the JSObject's private slot, but then js_deletes the js::Debugger on OOM. Later, the GC runs the JSObject's finalizer, which double-frees the hash tables.

Comment 23

[Jim Blandy :jimb]

Attachment: Don't js_delete a freshly allocated js::Debugger, if we've stored it in the Debugger JSObject's private slot.

Comment 24

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #22)
> Ah, okay, I'm able to reproduce a double-free. The problem is that
> Debugger::construct stores the pointer to the js::Debugger in the JSObject's
> private slot, but then js_deletes the js::Debugger on OOM. Later, the GC
> runs the JSObject's finalizer, which double-frees the hash tables.

Ooh, nice catch.

Comment 25

[Jim Blandy :jimb]

(In reply to Shu-yu Guo [:shu] from comment #24)
> Ooh, nice catch.

Glibc's malloc gave me a backtrace for the double free, with hex PC values and no source locations, so I was typing stuff like "list *0x921971" into GDB until I figured out what was going on.

Comment 26

[Shu-yu Guo [:shu]]

Jim, your double free fix fixes the intermittent crashes in the test cases? Also, do you mind just rolling the 2 patches into 1 and pushing it?

Comment 27

[Jim Blandy :jimb]

Sure.

Try: https://tbpl.mozilla.org/?tree=Try&rev=a3299f20a6d8

Comment 28

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c6feff2a255
https://hg.mozilla.org/integration/mozilla-inbound/rev/521a84fc4f6f

(That's both patches.)

Comment 29

[Carsten Book [:Tomcat]]

landed on central
https://hg.mozilla.org/mozilla-central/rev/6c6feff2a255
https://hg.mozilla.org/mozilla-central/rev/521a84fc4f6f

Comment 30

[Christian Holler (:decoder)]

Is this really sec-critical, given that it's a double-free involving Debugger (which is not available in content)? Also, is bug 926448 a dup of this?

Comment 32

[Jim Blandy :jimb]

(In reply to Christian Holler (:decoder) from comment #30)
> Is this really sec-critical, given that it's a double-free involving
> Debugger (which is not available in content)?

Now that we know that that's the cause: no, I think this is not security-critical.

> Also, is bug 926448 a dup of
> this?

Sure looks like it. Marked as such.