235145 - JS script_filename_table must be per-runtime

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

Blargh.  Fix next.

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: proposed fix

Edward, please use this in good health!  Sorry again for the trouble.

/be

Comment 2

[Brendan Eich [:brendan]]

Fixed.

Phil, we should probably do an RC6a that picks up this patch.

/be

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think this patch is causing tinderboxen to crash during test.

One thing I noticed is that when you moved the lock, you changed the type from
JSLock to PRLock.

Comment 4

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

I backed this out due to tinderbox orange.

Comment 5

[Edward Chan]

Hi guys, I'm not sure if this is the correct way to do this since I've never 
been involved in any bug before.  But one problem I saw with the fix was that 
js_FinishRuntimeScriptState() that is called in JS_DestroyContext is called 
before the final gc.  This nulls out rt->scriptFilenameTable.  But when you 
call js_GC() for the last time, js_SweepScriptFilenames() will pass a null ptr 
to JS_HashTableEnumerateEntries().

I moved js_FinishRuntimeScriptState() to after the js_GC call in 
JS_DestroyContext(), and all seems ok.  I've also added a JS_ASSERT to 
js_FinishRuntimeScriptState() to make sure the table is empty when we destroy 
it.  Otherwise, we'd have a memory leak, right?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The stack trace that I got agrees with that.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Just in case it's useful, here's the stack:

#5  <signal handler called>
#6  0x00cf2372 in JS_HashTableEnumerateEntries (ht=0x0, f=0xd3b939
<js_script_filename_sweeper>, arg=0x8c4f380) at jshash.c:366
#7  0x00d3b998 in js_SweepScriptFilenames (rt=0x8c4f380) at jsscript.c:1034
#8  0x00cf1686 in js_GC (cx=0xbec0fea8, gcflags=2) at jsgc.c:1303
#9  0x00cf0cab in js_ForceGC (cx=0xbec0fea8, gcflags=2) at jsgc.c:1000
#10 0x00ccc055 in js_DestroyContext (cx=0xbec0fea8, gcmode=JS_FORCE_GC) at
jscntxt.c:234
#11 0x00cbdbec in JS_DestroyContext (cx=0xbec0fea8) at jsapi.c:911
#12 0x005a00dd in ~XPCJSContextStack (this=0x8c67bc8) at xpcthreadcontext.cpp:60
#13 0x005a155c in XPCPerThreadData::CleanupAllThreads() () at
xpcthreadcontext.cpp:544
#14 0x00572996 in ~nsXPConnect (this=0x8b6c598) at nsXPConnect.cpp:145
#15 0x00572119 in nsXPConnect::Release() (this=0x8b6c598) at nsXPConnect.cpp:46
#16 0x00bf0d36 in nsScriptSecurityManager::Shutdown() () at
nsScriptSecurityManager.cpp:2695
#17 0x00bfab3c in CapsModuleDtor(nsIModule*) (thisModules=0x8c46748) at
nsSecurityManagerFactory.cpp:430
#18 0x0050e8a3 in nsGenericModule::Shutdown() (this=0x8c46748) at
nsGenericFactory.cpp:344
#19 0x0050e34a in ~nsGenericModule (this=0x8c46748) at nsGenericFactory.cpp:241
#20 0x0050e475 in nsGenericModule::Release() (this=0x8c46748) at
nsGenericFactory.cpp:249
#21 0x004cd184 in nsDll::Shutdown() (this=0x8c46cb0) at xcDll.cpp:380
#22 0x004c8b0f in nsFreeLibrary (dll=0x8c46cb0, serviceMgr=0x0, when=3) at
nsNativeComponentLoader.cpp:271
#23 0x004c8d42 in nsFreeLibraryEnum (aKey=0x8b0ca80, aData=0x8c46cb0,
closure=0xbff5ccb8) at nsNativeComponentLoader.cpp:342
#24 0x00469522 in hashEnumerate (table=0x8b030f0, hdr=0x8b03c90, i=67,
arg=0xbff5cc80) at nsHashtable.cpp:115
#25 0x0046216c in PL_DHashTableEnumerate (table=0x8b030f0, etor=0x4694f4
<hashEnumerate>, arg=0xbff5cc80) at pldhash.c:619
#26 0x00469e4d in nsHashtable::Enumerate(int (*)(nsHashKey*, void*, void*),
void*) (this=0x8b030e8, aEnumFunc=0x4c8cf0 <nsFreeLibraryEnum>,
aClosure=0xbff5ccb8) at nsHashtable.cpp:303
#27 0x004caf34 in nsNativeComponentLoader::UnloadAll(int) (this=0x8b030a8,
aWhen=3) at nsNativeComponentLoader.cpp:960
#28 0x004c39f7 in nsComponentManagerImpl::UnloadLibraries(nsIServiceManager*,
int) (this=0x8afcc28, serviceMgr=0x0, aWhen=3) at nsComponentManager.cpp:3135
#29 0x004bdd34 in nsComponentManagerImpl::Shutdown() (this=0x8afcc28) at
nsComponentManager.cpp:875
#30 0x0045dc47 in NS_ShutdownXPCOM (servMgr=0x0) at nsXPComInit.cpp:773
#31 0x0807e737 in NS_ShutdownXPCOM (servMgr=0x0) at nsXPCOMGlue.cpp:184
#32 0x0807ef71 in GRE_Shutdown () at nsXPCOMGlue.cpp:452
#33 0x08063e7c in main (argc=3, argv=0xbff5cf14) at nsAppRunner.cpp:1686

Comment 8

[Brendan Eich [:brendan]]

Double-blargh.  I should test quitting the app instead of quitting gdb. 
Everything looked good.

Fixing again, thanks for the back-out; sorry for the trouble.

/be

Comment 9

[Brendan Eich [:brendan]]

Re: comment 5: Edward, we don't leak in any case, because JS_HashTableDestroy
calls the entry free function, which frees any unremoved entries.  But since the
patch I landed, and your patch to your source that anticipated it, both move the
destruction of the hash table down till after the last GC, the assertion you
added is ok.  It's just not necessary to flag a leak, though.

/be