307289 - crash [@ JS_DHashTableOperate] when closing browser

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Adam Guthrie]

I've had this happen twice over the past two days for no apparent reason.

Incident ID: 9063326
Stack Signature	JS_DHashTableOperate() a639da82
Product ID	FirefoxTrunk
Build ID	2005090505
Trigger Time	2005-09-05 14:16:53.0
Platform	LinuxIntel
Operating System	Linux 2.6.12-1.1447_FC4
Module	libmozjs.so + (00024955)
URL visited	
User Comments	
Since Last Crash	2 sec
Total Uptime	2 sec
Trigger Reason	SIGSEGV: Segmentation Fault: (signal 11)
Source File, Line No.
/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/js/src/jsdhash.c,
line 491
Stack Trace 	
JS_DHashTableOperate() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/js/src/jsdhash.c,
line 491]
XPCWrappedNativeProto::JSProtoObjectFinalized() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/js/src/xpconnect/src/xpcwrappednativeproto.cpp,
line 407]
XPC_WN_Shared_Proto_Finalize() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,
line 1492]
js_FinalizeObject() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/js/src/jsobj.c,
line 2077]
js_GC() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/js/src/jsgc.c,
line 1839]
js_ForceGC() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/js/src/jsgc.c,
line 1511]
js_DestroyContext() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/js/src/jscntxt.c,
line 284]
JS_DestroyContext() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/js/src/jsapi.c,
line 953]
XPCJSContextStack::~XPCJSContextStack() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/js/src/xpconnect/src/xpcthreadcontext.cpp,
line 61]
XPCPerThreadData::CleanupAllThreads() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/js/src/xpconnect/src/xpcthreadcontext.cpp,
line 544]
nsXPConnect::~nsXPConnect() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/js/src/xpconnect/src/nsXPConnect.cpp,
line 150]
nsXPConnect::Release() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/js/src/xpconnect/src/nsXPConnect.cpp,
line 48]
nsScriptSecurityManager::Shutdown() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/caps/src/nsScriptSecurityManager.cpp,
line 2996]
nsGenericModule::Shutdown() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/xpcom/build/nsGenericFactory.cpp,
line 339]
nsGenericModule::~nsGenericModule() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/xpcom/build/nsGenericFactory.cpp,
line 236]
nsGenericModule::Release() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/xpcom/build/nsGenericFactory.cpp,
line 244]
nsCOMPtr_base::assign_with_AddRef() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/xpcom/build/nsCOMPtr.cpp,
line 531]
info_ClearEntry() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/xpcom/components/nsStaticComponentLoader.cpp,
line 101]
PL_DHashTableFinish() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/xpcom/build/pldhash.c,
line 345]
nsStaticComponentLoader::Release() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/xpcom/components/nsStaticComponentLoader.cpp,
line 82]
nsComponentManagerImpl::Shutdown() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/xpcom/components/nsComponentManager.cpp,
line 923]
NS_ShutdownXPCOM_P() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/xpcom/build/nsXPComInit.cpp,
line 831]
ScopedXPCOMStartup::~ScopedXPCOMStartup() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/toolkit/xre/nsAppRunner.cpp,
line 553]
XRE_main() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/toolkit/xre/nsAppRunner.cpp,
line 848]
main() 
[/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/browser/app/nsBrowserApp.cpp,
line 62]
libc.so.6 + 0x1549f (0x00df249f)

Comment 1

[Brendan Eich [:brendan]]

Reporter: when you see a crash in a general-purpose data structure
implementation of long standing, look up the stack to find the bug's initial
component.

jst: any chance this is fallout from bug 304423 ?  I've been expecting some,
just based on our luck lately!

/be

Comment 2

[Johnny Stenback (:jst)]

Yeah, I bet this is due to the fix for bug 304423 :(

I bet we somehow end up in a situation where we've got live
XPCWrappedNativeProto object's outliving XPConnect's shutdown code and for some
reason don't get torn down properly. There's tons of code in XPConnect to deal
with all this, but I bet these prototype objects for some reason don't end up in
the right maps etc and don't get cleaned up.

Comment 3

[Johnny Stenback (:jst)]

Attachment: Probable fix, make sure replaced protos get properly cleaned up.

When we're restoring an old proto into a scope we need to put the proto we're
replacing in the scope's classinfo to wrapped native proto map on the map of
detached prototypes so that it'll get properly cleaned up. W/o doing this we
can end up in this exact situation where we potentially have prototypes in
memory that don't end up getting told about shutdown so they'll get finalized
after shutdown, after their scopes are dead.

I haven't been able to reproduce this crash, so it's hard to tell for sure if
this is *the* fix for this, but this is IMO the right thing to do and really
likely to fix this exact crash.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 195202 [details] [diff] [review]
Probable fix, make sure replaced protos get properly cleaned up.

Looks good, but if jband can jump in late (just cc'ed him and it's been years
since he looked at this code, I bet), even better.

Obligatory nag to follow if(...)\n\t{...\t} style (\t for indentation, not hard
tab).

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 195202 [details] [diff] [review]
Probable fix, make sure replaced protos get properly cleaned up.

>Index: js/src/xpconnect/src/nsXPConnect.cpp
>@@ -1026,22 +1026,50 @@ nsXPConnect::RestoreWrappedNativePrototy
>+            // entry for aClassInfo in the map we haveto remove it to

Nit: 'have to'

r=mrbkap

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 195202 [details] [diff] [review]
Probable fix, make sure replaced protos get properly cleaned up.

restoring sr=brendan that got lost...

Comment 7

[John Bandhauer]

Wow, an xpconnect bug that isn't my fault! :)

I'm sure Johnny has a much better grasp of this code than I have these days.
But, this looks right to me.

I was going to make some noise about the patch not dealing with the case where
oldProto == proto. But, It looks to me like it ends up doing the right thing
anyway. I'm not sure if this is undocumented cleaverness or just blind luck :) I
think it might be good to note this in a comment though. Given all the other
extreme error checking, I wouldn't think you'd *expect* that the values must be
different at this level.

Also, I didn't see any mention in the bug text about the SetGlobal call you
added. Though, the comment makes this cound reasonable and I'm buying it.

Comment 8

[Bill Gianopoulos [:WG9s]]

I realize it is VERY late for 1.5 beta1, but if there is going to be a respin, I
would seriously consider taking this if it fixes the issue.  I get this crash
better than 50% of the time if I have a chatzilla window open when I am exiting.
 If I have the browser window covered by the chatzilla window and click the
close X on the chatzilla window then the X on the now visible browser window it
Will crash most of the time.  I have no idea if this would effect other
extensions or not, but since extensions developers will be using this build to
try to debug extension issues, it would be better NOT to confuse them and send
them down a rabbithole trying to chase down a crash that is really not their fault.

Comment 9

[Johnny Stenback (:jst)]

(In reply to comment #7)
> I was going to make some noise about the patch not dealing with the case where
> oldProto == proto. But, It looks to me like it ends up doing the right thing
> anyway. I'm not sure if this is undocumented cleaverness or just blind luck :) 

That's probably more of the latter :)

> think it might be good to note this in a comment though. Given all the other
> extreme error checking, I wouldn't think you'd *expect* that the values must be
> different at this level.

I just checked in a comment and a warning to catch any such callers.

> Also, I didn't see any mention in the bug text about the SetGlobal call you
> added. Though, the comment makes this cound reasonable and I'm buying it.

Yeah, forgot to mention that, even after mrbkap pointed it out to me in person
:( That change was something I just realized we should do based on reading code,
as the comment says, the scope can be left with a cached Object.prototype that's
not the current Object.prototype.

Thanks for having a look :)

Comment 10

[Bill Gianopoulos [:WG9s]]

Sorry for additiona bugsapm on this, but I am shamelessly lobbying again to try
to get this checked into the branch so that it can be included in the security
update for the IDN vulnerability.

The rationale for this is:

1.  According to Talkback, this is the top crasher for the Firefox 1.5 beta build.
2.  There have been 0 talkback incidents with this signature in trunk builds
containing this patch.
3.  There have been no reports from trunk testers of any regressions caused by
this check-in.
4.  From my personal experience I have gone from multiple crashes per day to
zero crashes since this patch was checked in.

So, this would seem to be a relatively safe patch that would fix a major issue
with the current Firefox beta.

Comment 11

[Johnny Stenback (:jst)]

Taking bug.

Comment 12

[Johnny Stenback (:jst)]

Attachment: 1.8 branch version of the fix, including comment and warning changes mentioned in the bug.

Comment 13

[Johnny Stenback (:jst)]

Fix landed on both the trunk and the 1.8 branch.

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

*** Bug 308363 has been marked as a duplicate of this bug. ***