402653 - xpcshell core dump when shutdown

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Solar Flare]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007110515 Minefield/3.0a9pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) 

back trace:

Core was generated by `./xpcshell'.
Program terminated with signal 5, Trace/breakpoint trap.
#0  JS_Assert (s=0xb7ebe097 "!rt->deflatedStringCacheLock", 
    file=0xb7ebdb80 "/home/solar/mozilla/js/src/jsstr.c", ln=2349)
    at /home/solar/mozilla/js/src/jsutil.c:63
63          abort();
(gdb) bt
#0  JS_Assert (s=0xb7ebe097 "!rt->deflatedStringCacheLock", 
    file=0xb7ebdb80 "/home/solar/mozilla/js/src/jsstr.c", ln=2349)
    at /home/solar/mozilla/js/src/jsutil.c:63
#1  0xb7e8b631 in js_InitRuntimeStringState (cx=0x8094710)
    at /home/solar/mozilla/js/src/jsstr.c:2349
#2  0xb7ddd541 in js_NewContext (rt=0x808eec0, stackChunkSize=8192)
    at /home/solar/mozilla/js/src/jscntxt.c:297
#3  0xb7dc8024 in JS_NewContext (rt=0x808eec0, stackChunkSize=8192)
    at /home/solar/mozilla/js/src/jsapi.c:990
#4  0xb789b82a in XPCJSContextStack::GetSafeJSContext (this=0x808eb88, 
    aSafeJSContext=0xbfb87110)
    at /home/solar/mozilla/js/src/xpconnect/src/xpcthreadcontext.cpp:206
#5  0xb786b407 in XPCCallContext (this=0xbfb870f4, 
    callerLanguage=XPCContext::LANG_NATIVE, cx=0x0, obj=0x0, funobj=0x0, 
    name=0, argc=4294967295, argv=0x0, rval=0x0)
    at /home/solar/mozilla/js/src/xpconnect/src/xpccallcontext.cpp:93
#6  0xb78659af in nsXPConnect::Collect (this=0x8095160)
    at /home/solar/mozilla/js/src/xpconnect/src/nsXPConnect.cpp:490
#7  0xb7d614b7 in nsCycleCollector::Collect (this=0x806e8b8, aTryCollections=5)
    at /home/solar/mozilla/xpcom/base/nsCycleCollector.cpp:2094
#8  0xb7d61589 in nsCycleCollector::Shutdown (this=0x806e8b8)
    at /home/solar/mozilla/xpcom/base/nsCycleCollector.cpp:2249
#9  0xb7d615c0 in nsCycleCollector_shutdown ()
---Type <return> to continue, or q <return> to quit---
    at /home/solar/mozilla/xpcom/base/nsCycleCollector.cpp:2667
#10 0xb7cd71ea in NS_ShutdownXPCOM_P (servMgr=0x0)
    at /home/solar/mozilla/xpcom/build/nsXPComInit.cpp:785
#11 0xb7dab7c7 in NS_ShutdownXPCOM (svcMgr=0x0)
    at /home/solar/mozilla/xpcom/stub/nsXPComStub.cpp:175
#12 0x0804c234 in main (argc=0, argv=0xbfb873c8, envp=0xbfb873cc)
    at /home/solar/mozilla/js/src/xpconnect/shell/xpcshell.cpp:1459
Current language:  auto; currently c
(gdb)

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Solar Flare]

Attachment: change the initialization of string hash

Per igor's suggestion, the string hash(and its lock) should be initialized in JS_NewRuntime rather than JS_NewContext. (Comment #51 in bug 401687)
I had updated the patch.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

How common is it to hit this? This sounds like something we really don't want to ship beta with.

Comment 3

[Igor Bukanov]

Comment on attachment 287498 [details] [diff] [review]
change the initialization of string hash

>+++ js/src/jsstr.c	5 Nov 2007 10:19:06 -0000
> JSBool
> js_InitRuntimeStringState(JSContext *cx)
> {
>+    rt->deflatedStringCache->keyHash=js_hash_string_pointer;
>     rt->emptyString = ATOM_TO_STRING(rt->atomState.emptyAtom);
>     return JS_TRUE;
> }
>-void
>-js_FinishDeflatedStringCache(JSRuntime *rt)
>-{
>-    if (rt->deflatedStringCache) {
>-        JS_HashTableDestroy(rt->deflatedStringCache);
>-        rt->deflatedStringCache = NULL;
>-    }
>-#ifdef JS_THREADSAFE
>-    if (rt->deflatedStringCacheLock) {
>-        JS_DESTROY_LOCK(rt->deflatedStringCacheLock);
>-        rt->deflatedStringCacheLock = NULL;
>-    }
>-#endif
>-}

To minimize the patch and avoid the ugliness of rt->deflatedStringCache->keyHash=js_hash_string_pointer I suggest to keep js_FinishDeflatedStringCache and just add a parallel 

JSBool 
js_InitDeflatedStringCache(JSRuntime *rt);

to be called from the runtime initialization code. 

r+ with these changes.

Comment 4

[Solar Flare]

(In reply to comment #2)
> How common is it to hit this? This sounds like something we really don't want
> to ship beta with.
> 

xpcshell will crash when components/compreg.dat exist.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 287498 [details] [diff] [review]
change the initialization of string hash

Yeah, and space on each side of = and other operators.

/be

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, does this only affect xpcshell? If so I don't think this is a beta blocker.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Not blocking M9, I'll leave it to component owners to decide overall blocking status.

Comment 8

[Solar Flare]

Attachment: add js_InitDeflatedStringCache

I had updated the patch. This time, I add a new js_InitDeflatedStringCache function in jsstr.c. It will be called when create a new JSRuntime. This correspond with the js_FinishDeflatedStringCache function which will be called when destroy JSRuntime.

This bug currently affects xpcshell only. But it's a potential issue of firefox I think.

Comment 9

[Brian Crowder]

Though the patch needs review, it looks good or close, and this bug very likely -does- affect more than just xpcshell.  Not sure why Fx is getting lucky right now.

Comment 10

[Igor Bukanov]

Comment on attachment 287651 [details] [diff] [review]
add js_InitDeflatedStringCache

>Index: js/src/jsstr.c
> void
> js_PurgeDeflatedStringCache(JSRuntime *rt, JSString *str)
> {
>     JSHashNumber hash;
>     JSHashEntry *he, **hep;
> 
>     if (!rt->deflatedStringCache)
>         return;

This is useless check as with the patch js_PurgeDeflatedStringCache should never be called with not initialized cache. r+ with that fixed.

Comment 11

[Brian Crowder]

instead, JS_ASSERT(rt->deflatedStringCache) ?

Comment 12

[Igor Bukanov]

(In reply to comment #11)
> instead, JS_ASSERT(rt->deflatedStringCache) ?

The assert is not necessary as the CPU will do the check when dereferencing the null pointer. I.e. SpiderMonkey's style is not to assert that a pointer is not null if the pointer is dereferenced on all code paths. 

Comment 13

[Solar Flare]

Attachment: Remove the useless check

Remove the useless check in js_PurgeDeflatedStringCache.

Comment 14

[Reed Loden [:reed]]

Thanks for the patch!

Checking in js/src/jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.370; previous revision: 3.369
done
Checking in js/src/jsstr.c;
/cvsroot/mozilla/js/src/jsstr.c,v  <--  jsstr.c
new revision: 3.175; previous revision: 3.174
done
Checking in js/src/jsstr.h;
/cvsroot/mozilla/js/src/jsstr.h,v  <--  jsstr.h
new revision: 3.54; previous revision: 3.53
done