336907 - Fatal JS assert during shutdown: "lock != NULL" (#ifdef GC_MARK_DEBUG)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: Trunk Linux Firefox build with MOZ_CO_DATE=="Fri May 5 22:30:11 CDT 2006"

STEPS TO REPRODUCE:
1) Build
2) Run
3) Crash

Stack:

#9  0xb7cf813d in PR_Assert (s=0xb7d22690 "lock != NULL", 
    file=0xb7d225c8 "../../../../../mozilla/nsprpub/pr/src/pthreads/ptsynch.c", ln=205)
    at ../../../../../mozilla/nsprpub/pr/src/io/prlog.c:538
#10 0xb7d0f8d4 in PR_Lock (lock=0x0)
    at ../../../../../mozilla/nsprpub/pr/src/pthreads/ptsynch.c:205
#11 0xb7f538b2 in js_GetStringBytes (rt=0x8117e28, str=0x818b478)
    at ../../../mozilla/js/src/jsstr.c:3152
#12 0xb7ebf7e0 in JS_GetStringBytes (str=0x818b478)
    at ../../../mozilla/js/src/jsapi.c:4503
#13 0xb7ef0c85 in js_MarkAtom (cx=0x834b380, atom=0x81de8c0)
    at ../../../mozilla/js/src/jsgc.c:1203
#14 0xb7f216da in js_Mark (cx=0x834b380, obj=0x85262f8, arg=0x0)
    at ../../../mozilla/js/src/jsobj.c:4213
#15 0xb7ef0e32 in MarkGCThingChildren (cx=0x834b380, thing=0x85262f8, 
    flagp=0x8527c4f "\020", shouldCheckRecursion=1)
    at ../../../mozilla/js/src/jsgc.c:1281
#16 0xb7ef1ad7 in js_MarkGCThing (cx=0x834b380, thing=0x85262f8)
    at ../../../mozilla/js/src/jsgc.c:1664
#17 0xb7ef0bbf in js_MarkNamedGCThing (cx=0x834b380, thing=0x85262f8, 
    name=0x83bbb00 "nsXPCWrappedJS::mJSObj[nsITimerCallback,0x83bbac8,0x85262f8]")
    at ../../../mozilla/js/src/jsgc.c:1172
#18 0xb7ef1c7d in gc_root_marker (table=0x8117ea4, hdr=0x83c8b40, num=0, arg=0x834b380)
    at ../../../mozilla/js/src/jsgc.c:1728
#19 0xb7ed41d3 in JS_DHashTableEnumerate (table=0x8117ea4, 
    etor=0xb7ef1b16 <gc_root_marker>, arg=0x834b380)
    at ../../../mozilla/js/src/jsdhash.c:674
#20 0xb7ef24d9 in js_GC (cx=0x834b380, gcflags=2) at ../../../mozilla/js/src/jsgc.c:2000
#21 0xb7ef1d33 in js_ForceGC (cx=0x834b380, gcflags=2)
    at ../../../mozilla/js/src/jsgc.c:1753
#22 0xb7ec819f in js_DestroyContext (cx=0x834b380, gcmode=JS_FORCE_GC)
    at ../../../mozilla/js/src/jscntxt.c:340
#23 0xb7eb6c87 in JS_DestroyContext (cx=0x834b380) at ../../../mozilla/js/src/jsapi.c:938
#24 0xb78f0e65 in XPCJSContextStack::~XPCJSContextStack$delete ()
    at ../../../../dist/include/xpcom/xptcall.h:116
#25 0xb78f24c8 in XPCPerThreadData::CleanupAllThreads ()
    at ../../../../../mozilla/js/src/xpconnect/src/xpcthreadcontext.cpp:597
#26 0xb78b940e in ~nsXPConnect (this=0x813fd50)
    at ../../../../../mozilla/js/src/xpconnect/src/nsXPConnect.cpp:140
#27 0xb78b8c74 in nsXPConnect::Release (this=0x813fd50)
    at ../../../../../mozilla/js/src/xpconnect/src/nsXPConnect.cpp:48
#28 0xb78c314e in ~XPCCallContext (this=0xbfffebe0)
    at ../../../../../mozilla/js/src/xpconnect/src/xpccallcontext.cpp:357
#29 0xb78b96ca in nsXPConnect::ReleaseXPConnectSingleton ()
    at ../../../../../mozilla/js/src/xpconnect/src/nsXPConnect.cpp:231
#30 0xb78ee496 in xpcModuleDtor (self=0x8098710)
    at ../../../../../mozilla/js/src/xpconnect/src/xpcmodule.cpp:132
#31 0xb7dbf5db in nsGenericModule::Shutdown (this=0x8098710) at nsGenericFactory.cpp:340
#32 0xb7dbf05e in ~nsGenericModule (this=0x8098710) at nsGenericFactory.cpp:237
#33 0xb7dbf192 in nsGenericModule::Release (this=0x8098710) at nsGenericFactory.cpp:245
#34 0xb7e319c9 in nsCOMPtr<nsIModule>::assign_assuming_AddRef (this=0x8103de8, 
    newPtr=0x0) at ../../dist/include/xpcom/nsCOMPtr.h:568
#35 0xb7e312b0 in nsCOMPtr<nsIModule>::assign_with_AddRef (this=0x8103de8, rawPtr=0x0)
    at ../../dist/include/xpcom/nsCOMPtr.h:1224
#36 0xb7e33937 in nsCOMPtr<nsIModule>::operator= (this=0x8103de8, rhs=0x0)
    at ../../dist/include/xpcom/nsCOMPtr.h:713
#37 0xb7e3365a in nsNativeModuleLoader::ReleaserFunc (aHashedFile=0x8098174, 
    aLoadData=@0x8103de8)
    at ../../../mozilla/xpcom/components/nsNativeComponentLoader.cpp:232
#38 0xb7e33b77 in nsBaseHashtable<nsHashableHashKey, nsNativeModuleLoader::NativeLoadData,
 nsNativeModuleLoader::NativeLoadData>::s_EnumStub (table=0x8072218, hdr=0x8103de0, 
    number=51, arg=0xbfffee28) at ../../dist/include/xpcom/nsBaseHashtable.h:346
#39 0xb7db4e28 in PL_DHashTableEnumerate (table=0x8072218, 
    etor=0xb7e33b34 <nsBaseHashtable<nsHashableHashKey, nsNativeModuleLoader::NativeLoadData, nsNativeModuleLoader::NativeLoadData>::s_EnumStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*)>, arg=0xbfffee28) at pldhash.c:684
#40 0xb7e339b5 in nsBaseHashtable<nsHashableHashKey, nsNativeModuleLoader::NativeLoadData, nsNativeModuleLoader::NativeLoadData>::Enumerate (this=0x8072218, 
    enumFunc=0xb7e3363a <nsNativeModuleLoader::ReleaserFunc(nsIHashable*, nsNativeModuleLoader::NativeLoadData&, void*)>, userArg=0x0)
    at ../../dist/include/xpcom/nsBaseHashtable.h:219
#41 0xb7e3378b in nsNativeModuleLoader::UnloadLibraries (this=0x8072214)
    at ../../../mozilla/xpcom/components/nsNativeComponentLoader.cpp:271
#42 0xb7e272b0 in nsComponentManagerImpl::Shutdown (this=0x80721a8)
    at ../../../mozilla/xpcom/components/nsComponentManager.cpp:797
#43 0xb7dc3427 in NS_ShutdownXPCOM_P (servMgr=0x0)
    at ../../../mozilla/xpcom/build/nsXPComInit.cpp:784
#44 0xb7fb04e0 in ~ScopedXPCOMStartup (this=0xbffff110)
    at ../../../mozilla/toolkit/xre/nsAppRunner.cpp:556
#45 0xb7fb8949 in XRE_main (argc=1, argv=0xbffff374, aAppData=0x8049740)
    at ../../../mozilla/toolkit/xre/nsAppRunner.cpp:2399
#46 0x080485be in main (argc=1, argv=0xbffff374)
    at ../../../mozilla/browser/app/nsBrowserApp.cpp:61

rt->deflatedStringCacheLock is null, of course.

I don't know how to reproduce this EM restart stuff, so I'll keep this in a debugger until I hear otherwise, I guess.  :(

Comment 1

[Boris Zbarsky [:bzbarsky]]

So to be precise, what happens here is that #ifdef GC_MARK_DEBUG we get the string from an atom when marking it.  And apparently we're trying to do this too late in shutdown or something?

I'm seeing this on shutdown in all my GC_MARK_DEBUG builds, which makes leak debugging more or less impossible (since this dies before the refcount logging output gets written out).  We really need to get this fixed for alpha, esp. because there are some fun Places leaks that I can't debug due to this.

I still don't know how to reproduce this reliably, but it certainly happens any time we've leaked stuff entrained via JS...

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix

Apart from spacing and overlong-line cleanups, this patch attempts to reunify the APIs for js_Inflate- and js_DeflateStringToBuffer, so that callers may pass NULL for the next to last argument, and discover the needed "size" (number of chars or bytes, resp.) for the operation.  Also, if null cx is passed, the operation is done up to the passed-in *dstlenp or equivalent (*charsLength, *bytesLength) parameter value.

After this fixing, which I thought I would need for a jsgc.c-only patch to solve the problem bz ran into (GC_MARK_DEBUG calling JS_GetStringBytes during the last GC, after js_FinishRuntimeStringState has been called), bz pointed out calls in other files than jsgc.c during shutdown.  So I just made js_GetStringBytes test for null rt->deflatedStringCache #ifdef GC_MARK_DEBUG, and leakily return the result of a js_DeflateString call.

mrbkap, feel free to r+ too.

Boris, please let me know over IRC or here if this is not a complete fix.

/be

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yeah, with that patch I can shut down all the way without crashing.

Comment 4

[Michael Daumling]

Comment on attachment 221232 [details] [diff] [review]
proposed fix

Looks good to me. No nits ;)

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 221232 [details] [diff] [review]
proposed fix

sr=shaver

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could someone check this in if it hasn't been already, or mark the bug fixed if it has?

Comment 7

[Mike Schroepfer]

Bkap - can you check to see if the patch is still relevant?  If so check it in - either way close the bug out?  Thanks :-)

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

It appears that some of the cleanup has landed since the patch was attached to this bug, but the actual fix for the bug has not. I'll land the fix (updated patch forthcoming) and file a new bug for the other cleanup that has yet to land.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Minimal fix for this bug

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Without the old ifdef

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

I noticed that the other cleanup had happened in a slightly different way. I'll leave the code as-is and won't bother with the new bug.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

The condition added here wasn't correct, see the dependency.