Last Comment Bug 307289 - crash [@ JS_DHashTableOperate] when closing browser
: crash [@ JS_DHashTableOperate] when closing browser
Status: RESOLVED FIXED
: crash, fixed1.8
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- critical with 1 vote (vote)
: ---
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
: Phil Schwartau
:
Mentors:
: 308363 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-09-06 17:41 PDT by Adam Guthrie
Modified: 2007-01-29 03:36 PST (History)
12 users (show)
asa: blocking1.8b4-
brendan: blocking1.8b5+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Probable fix, make sure replaced protos get properly cleaned up. (2.51 KB, patch)
2005-09-07 17:08 PDT, Johnny Stenback (:jst, jst@mozilla.com)
mrbkap: review+
jst: superreview+
asa: approval1.8b5+
Details | Diff | Splinter Review
1.8 branch version of the fix, including comment and warning changes mentioned in the bug. (2.76 KB, patch)
2005-09-14 14:14 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review

Description Adam Guthrie 2005-09-06 17:41:16 PDT
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] 2005-09-06 18:03:17 PDT
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, jst@mozilla.com) 2005-09-07 16:10:09 PDT
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, jst@mozilla.com) 2005-09-07 17:08:57 PDT
Created attachment 195202 [details] [diff] [review]
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] 2005-09-07 17:14:51 PDT
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) 2005-09-07 17:15:59 PDT
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, jst@mozilla.com) 2005-09-07 17:30:09 PDT
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 2005-09-07 19:19:28 PDT
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] 2005-09-08 04:51:04 PDT
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, jst@mozilla.com) 2005-09-08 09:34:48 PDT
(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] 2005-09-10 06:51:35 PDT
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, jst@mozilla.com) 2005-09-14 14:09:33 PDT
Taking bug.
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2005-09-14 14:14:47 PDT
Created attachment 196077 [details] [diff] [review]
1.8 branch version of the fix, including comment and warning changes mentioned in the bug.
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2005-09-14 14:15:50 PDT
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) 2005-09-22 21:38:21 PDT
*** Bug 308363 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.