278725 - Assertion failure: obj->slots [@ js_FinalizeObject] finalizing object that is being constructed

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[James Ross]

It appears, from the details I have extracted from the engine shown at
http://twpol.dyndns.org/temp/js_c_1.txt, that the following is happening:

- A new object is wanted; js_NewObject called.
- New object needs slots; AllocSlots called.
- This needs a new GC-able object; js_NewGCThing called.
- There's a lack of space for the new object, last-ditch GC is used; js_GC called.
- GC finds an object in the free list; finalizer (js_FinalizeObject) called.
- Uh oh! The object has no slots! (because it's the one we're creating)

If I continue from this assert, I get another about GC running when it's not
expected ("Assertion failure: !rt->gcRunning, at mozilla/js/src/jsgc.c:569"),
and it all collapses and dies. I believe this assert is the first indication of
what's gone wrong.

Generally this crash happens when idling in ChatZilla. I have not used any other
bits of trunk CVS Mozilla to experience it elsewhere, though it may be limited
to ChatZilla for some reason.

Re URL: more details are available (it's sitting on the assert in the debugger
still), but I can't keep it like this forever - it's locking my normal profile, too.

Comment 1

[James Ross]

FWIW, I've also (earlier today) crashed after hitting this assert:

  http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsgc.c&mark=1090#1075

It appears to make the flawed assumption that no slots means no map, which is
not true during object creation (that crash also had its stack go through the
same line in js_NewObject, the AllocSlots call, IIRC).

Comment 2

[Alex Vincent [:WeirdAl]]

Attachment: xpcshell crash testcase

Comment 3

[James Ross]

Woah. *That* crashes here as well? why is the gc() inside an if(0), though?

Comment 4

[Alex Vincent [:WeirdAl]]

Because timeless wanted to include code that would indicate conditions under
which it didn't crash.

Comment 5

[Brendan Eich [:brendan]]

Sorry, fixing....

/be

Comment 6

[Brendan Eich [:brendan]]

From comment 0:

> There's a lack of space for the new object, last-ditch GC is used; js_GC called.
> GC finds an object in the free list; finalizer (js_FinalizeObject) called.
> Uh oh! The object has no slots! (because it's the one we're creating)

This doesn't make sense, btw -- the object that we're creating can't have been
allocated, and things on the freelist have already been finalized.

Comment 1 is right-on, OTOH! ;-)  Patch soon.

/be

Comment 7

[Brendan Eich [:brendan]]

So why didn't cx->newborn[GCX_OBJECT] protect the object in Silver's stack shown
in attachment 171560 [details], called obj in the js_NewObject frame?  Silver, if you can
reproduce this crash, what is cx->newborn[0] if not obj?

Silver: also, what is the stack when you get the JS_ASSERT(!rt->gcRunning)
botch? That usually means a finalizer is attempting to allocate new objects or
other GC-things, which is Not Allowed.

The other assertion that ajvincent et al. see is a bogus assert in jsgc.c and
not relevant to the one Silver sees, I think.

/be

Comment 8

[James Ross]

(In reply to comment #6)
> From comment 0:
> 
> > There's a lack of space for the new object, last-ditch GC is used; js_GC called.
> > GC finds an object in the free list; finalizer (js_FinalizeObject) called.
> > Uh oh! The object has no slots! (because it's the one we're creating)
> 
> This doesn't make sense, btw -- the object that we're creating can't have been
> allocated, and things on the freelist have already been finalized.

The technicalities are probably off - I only attempted to describe roughly what
the code was up to at each point without just pasting it in. :)

When this thing crashes again (why doesn't it crash when you want it to?), I'll
take a look at cx->newborn, and get the stack for the GC running assert.

Comment 9

[James Ross]

I just got the comment #1 assert again, and that also complains about the GC
running moments later. Stack: http://twpol.dyndns.org/temp/js_c_2.txt

It looks like, due to the dialog box for the assertion (which hasn't yet become
visible), the NSPR event loop has been re-entered, and a nsInputStreamPump has
gone off and had an event.

Comment 10

[Brendan Eich [:brendan]]

Attachment: fix attempt

I'd still like to understand how cx->newborn[GCX_OBJECT] was cleared after obj
was allocated, but before its slots were allocated and before the last ditch GC
failed to mark obj via the newborn root.  It could be that another last ditch
GC nested under GetClassPrototype, but I don't see how given all the looping --
we should have bootstrapped the standard classes, or at least Object, right
away.

Still, the order in js_NewObject was wrong, and this patch fixes that very old
bug, so if people try this patch and beat on it, and see no assert botches,
then I'm going to check it in.

/be

Comment 11

[James Ross]

Ok. I've got the obj->slots assertion back in the debugger.

cx->newborn[0] is 0x00000000 (as is the entire newborn array).

Any other information, just ask.

Comment 12

[Mike Shaver (:shaver emeritus)]

Are we bumping into
http://lxr.mozilla.org/mozilla/source/dom/src/base/nsJSEnvironment.cpp#1964
running at a bad time?  That line makes me nervous, I must say.

Comment 13

[James Ross]

cx->lastAtom is a valid item (0x01011a80 struct JSAtom *), and that would have
been cleared by JS_ClearNewbornRoots (I assume that was the line that worried
you?), but I can't say I know what might have put something into cx->lastAtom.

If you want, I can printf in the FireGCTimer code (and elsewhere), and see if it
is running nearby, but obviously I'd need to kill my current paused instance and
wait for it to crash again.

Comment 14

[Brendan Eich [:brendan]]

Does venkman run its code on the same cx as the debugged script runs on?

Silver, did you get the assertion back in the debugger (comment 11) with or
without the patch in this bug applied and built?

Thinking about it more, the jsobj.c part of this patch is a good fix for
branches as well as trunk.  It ought to go in bug 268535.  I'll do that shortly

/be

Comment 15

[James Ross]

I'm currently running without the patch (i.e. still the same code as when I
filed the bug). I can of course try the patch and see if it helps this
assert/crash, if you think it will.

Comment 16

[Brendan Eich [:brendan]]

Without the patch, the thing to try to determine (using a debugger data
breakpoint or watchpoint, if you can) is who nulled cx->newborn[0] just before
the GC that calls js_FinalizeObject where the assert botches.

With the patch, you should see no assertbotches and no crashes to-do with JS.

/be

Comment 17

[Brendan Eich [:brendan]]

Patch checked in, leaving bug open for confirmation of goodness.

/be

Comment 18

[James Ross]

I think Brendan has got this one nailed perfectly.

Last night I added a bunch of code to print a warning every time
JS_ClearNewbornRoots was called and the item in cx->newborn[0] was currently
still "under construction".

I got at least 45 of these warnings just starting Navigator. If anyone's
interested, I got stacks for a random collection of them (some were almost
identicaly, and they all share a few bits, in particular stack items 00 to 08
are all the same) here: http://twpol.dyndns.org/temp/js_c_3.txt

On the good news side of things, I've not seen a single one of my warnings since
starting my build with the patch in, and certainly no assertion failures from
Spidermonkey. :-)

Thanks Brendan - I'll mark this FIXED if this keeps up (as I believe it will)
for the rest of the day.

Comment 19

[James Ross]

Confirmation of goodness.

Thanks!

Comment 20

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-278725.js

Silver, with your ok this will go into the javascript test library.

Comment 21

[James Ross]

I didn't write that code, either Alex and/or timeless did, but it looks ok.

Comment 22

[Bob Clary [:bc] (inactive)]

ok, i'll add them to the contributor list. I assume you guys will give the "ok". ;-)

Comment 23

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-278725.js checked in.

Comment 24

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.0.5, 1.8.1, 1.9a1 win/macppc/linux.
note to self:  
js1_5/GC/regress-278725.js: result: TIMED OUT opt-1.9a1_2006070603-peach