7698 - CRASH in js_Enumerate after running some XPConnect code

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Alec Flett]

I've gotten this same crash multiple times on different linux boxes (haven't
gotten a chance to try windows yet)

Here's what I'm trying to do:
given an XPConnect object, I'm running this function on it:
function describe(object, name) {
  for (var i in object) {
    var value = object[i];
    if (typeof(object[i]) == "function")
      value = "[function]";

    dump(name + "." + i + " = " + value + "\n");
  }
}

When I run this on some XPConnected object (for instance nsIMsgIncomingServer)
more than once (i.e. first time is fine) I get this crash, with what looks like
the same stacktrace every time:
#0  js_Enumerate (cx=0x8527588, obj=0x853bb58, enum_op=JSENUMERATE_DESTROY,
    statep=0xbfffee38, idp=0x0) at jsobj.c:2290
#1  0x4038e141 in prop_iterator_finalize (cx=0x8527588, obj=0x853bb58)
    at jsinterp.c:104
#2  0x4039d9ae in js_FinalizeObject (cx=0x8527588, obj=0x853bb58)
    at jsobj.c:1259
#3  0x4038d58c in js_GC (cx=0x8527588) at jsgc.c:835
#4  0x4038cff0 in js_ForceGC (cx=0x8527588) at jsgc.c:618
#5  0x40374fde in JS_GC (cx=0x8527588) at jsapi.c:867
#6  0x402f7b03 in nsJSContext::GC (this=0x85270a8) at nsJSEnvironment.cpp:328
#7  0x402fd181 in GlobalWindowImpl::SetNewDocument (this=0x8527528,
    aDocument=0x0) at nsGlobalWindow.cpp:257
#8  0x40d25493 in DocumentViewerImpl::~DocumentViewerImpl (this=0x8329840,
    __in_chrg=3) at nsDocumentViewer.cpp:225
#9  0x40d25307 in DocumentViewerImpl::Release (this=0x8329840)
    at nsDocumentViewer.cpp:184
#10 0x40279d0b in nsWebShell::Embed (this=0x84df340, aContentViewer=0x85c3f28,
    aCommand=0x85e0720 "view", aExtraInfo=0x0) at nsWebShell.cpp:755
#11 0x40277cf1 in nsDocumentBindInfo::OnStartBinding (this=0x84e05d0,
    aURL=0x85c2210, aContentType=0x85c3b38 "text/html") at nsDocLoader.cpp:1432
#12 0x4026272e in NET_NGLayoutConverter (format_out=38, converter_obj=0x0,
    URL_s=0x85c3580, context=0x85c22b0) at nsStubContext.cpp:949
#13 0x4024154b in NET_StreamBuilder (format_out=38, URL_s=0x85c3580,
    context=0x85c22b0) at mkstream.c:237
#14 0x4020f4f1 in NET_CacheConverter (format_out=38, converter_obj=0x0,
    URL_s=0x85c3580, window_id=0x85c22b0) at mkcache.c:1689
#15 0x4024154b in NET_StreamBuilder (format_out=102, URL_s=0x85c3580,
    context=0x85c22b0) at mkstream.c:237
#16 0x40180016 in net_finish_setup_http_stream (ce=0x85c23e8) at mkhttp.c:2877
#17 0x4017fb78 in net_setup_http_stream (ce=0x85c23e8) at mkhttp.c:2645
#18 0x40180c16 in net_ProcessHTTP (ce=0x85c23e8) at mkhttp.c:3558
#19 0x4023ac8b in NET_ProcessNet (ready_fd=0x85c25a0, fd_type=2)
    at mkgeturl.c:3355
#20 0x4024086f in NET_PollSockets () at mkselect.c:298
#21 0x4025d105 in nsNetlibService::NetPollSocketsCallback (aTimer=0x8527500,
    aClosure=0x80bde18) at nsNetService.cpp:1244
#22 0x400fa404 in TimerImpl::FireTimeout (this=0x8527500) at nsTimer.cpp:76
#23 0x400fa7d4 in nsTimerExpired (aCallData=0x8527500) at nsTimer.cpp:196
#24 0x405c1c81 in g_timeout_dispatch ()
#25 0x405c0e12 in g_main_dispatch ()
#26 0x405c140b in g_main_iterate ()
#27 0x405c15c1 in g_main_run ()
#28 0x404e6c4b in gtk_main ()
#29 0x400b3211 in nsAppShell::Run (this=0x809d9d0) at nsAppShell.cpp:197
#30 0x40017d72 in nsAppShellService::Run (this=0x8078d48)
    at nsAppShellService.cpp:402
#31 0x804a84f in main (argc=2, argv=0xbffff6b4) at nsAppRunner.cpp:514

The call to js_Enumerate looks like

	/* Fall through ... */

    case JSENUMERATE_DESTROY:
	state = JSVAL_TO_PRIVATE(*statep);
      =>JS_DestroyIdArray(cx, state->ida);
	JS_free(cx, state);
	*statep = JSVAL_NULL;
	return JS_TRUE;

and at this point "state" is "0xc" which is really wierd, but explains why
dereferencing it dumps core.

Comment 1

[leger]

Updating Component

Comment 2

[John Bandhauer]

This is really a JS Engine bug.

The problem is that the iteration state holder code in the forinloop stuff in
jsinterp.c only works right for objects that use the default JSObject
enumeration code.

The jsinterp.c code builds a propobj object of class prop_iterator_class to hold
the iteration state. The idea is that if the forinloop terminates before the
enumeration is done then the finalize of the propobj will call the enumerator
callback with JSENUMERATE_DESTROY to have it clean up.

But, this is broken. prop_iterator_finalize does...
  OBJ_ENUMERATE(cx, obj, JSENUMERATE_DESTROY, &iter_state, NULL);
... where 'obj' is the propobj itself. So, the default enumerator code gets
called and *it* assumes that iter_state is an object of a particular shape
(JSNativeIteratorState). Unfortunately, for xpconnect (and others?) who use
their own enumeration code and their own state struct this causes havoc.

So what should we do?

The propobj has a reference to the 'iteratee' in a slot, but there is is no
guarantee that it won't be finalized first (right?).

We don't want to make the iteratee track its own iteration state(s) (to be
free'd upon its own finalization).

We don't want to leak these object's iteration states.

I'm short on ideas that will cover all the bases and not be absurdly complex.

Ideas?

Comment 3

[Scott Furman]

The object being iterated over is stored in the propobj (in JSSLOT_PARENT).  It
seems like we could just change the critical line in prop_iterator_finalize:

OBJ_ENUMERATE(cx, obj->slots[JSSLOT_PARENT], JSENUMERATE_DESTROY, &iter_state,
NULL);

Oh, but wait, that won't work:  What if the object being iterated over has since
become unreachable and has already been finalized ?

+ We don't want to make the iteratee track its own iteration state(s) (to be
+ free'd upon its own finalization).

I suspect that this is exactly what we will need to do.

Comment 4

[John Bandhauer]

Well, we could root the iteratee slot in the propobj and then remove the root
upon propobj finalization.

Comment 5

[John Bandhauer]

Attachment: This works. Too much overhead on for-in?

Comment 6

[brendan-obsolete]

Testing iteratee != JSVAL_NULL isn't quite right -- if the object is stillborn
(due to error around line 1333 in jsinterp.c), the slot will be JSVAL_VOID, not
JSVAL_NULL.  I don't see code that would set JSSLOT_PARENT to JSVAL_NULL, but if
you want to defend, use JSVAL_IS_PRIMITIVE(iteratee) instead to rule out
non-object-references.

The rooting is annoying, but it's a sufficient fix.  Given the JSIdArray malloc
and iter_state malloc entailed by native for/in, I don't think that yet another
malloc for a root hashtable entry will kill us.  We should use double hashing
anyway to avoid malloc per entry.

/be

Comment 7

[Scott Furman]

I think that brendan is correct and that a test for JSVAL_IS_VOID() is
sufficient since I don't think iteratee can become NULL.  JSVAL_IS_PRIMITIVE()
is paranoia, but harmless.

Other than that, I think this is a good fix.

Comment 8

[John Bandhauer]

checked in the fix on trunk and SpiderMonkey140_BRANCH

Comment 9

[cbegle]

verified cmb. 8/31/99. i have tests for this, will check in soon.