513315 - crash [@ nsScriptSecurityManager::GetCurrentJSContext()]

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Wayne Mery (:wsmwk)]

[@ nsScriptSecurityManager::GetCurrentJSContext()]

#18 crash for 3.0b3, but hard to say it's a topcrash given
bp-efccf75b-c216-489f-adec-9b6b02090813 "I've been having trouble all day"
nsScriptSecurityManager::GetCurrentJSContext	 caps/src/nsScriptSecurityManager.cpp:327
nsScriptSecurityManager::IsCapabilityEnabled	caps/src/nsScriptSecurityManager.cpp:2442
nsScriptSecurityManager::CheckXPCPermissions	caps/src/nsScriptSecurityManager.cpp:3082
nsScriptSecurityManager::CanCreateWrapper	caps/src/nsScriptSecurityManager.cpp:2889
XPCWrappedNative::InitTearOff	js/src/xpconnect/src/xpcwrappednative.cpp:1803
XPCWrappedNative::FindTearOff	js/src/xpconnect/src/xpcwrappednative.cpp:1629
XPCCallContext::CanCallNow	js/src/xpconnect/src/xpccallcontext.cpp:270
XPCWrappedNative::CallMethod	js/src/xpconnect/src/xpcwrappednative.cpp:1960
XPC_WN_GetterSetter	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1622
js_Invoke	js/src/jsinterp.cpp:1386
js_InternalInvoke	js/src/jsinterp.cpp:1447
js_InternalGetOrSet	js/src/jsinterp.cpp:1510
js_GetSprop	js/src/jsscope.h:367
js_NativeGet	js/src/jsobj.cpp:4167
js_GetPropertyHelper	js/src/jsobj.cpp:4333
js_Interpret	js/src/jsinterp.cpp:4451
js_Invoke	js/src/jsinterp.cpp:1394
nsXPCWrappedJSClass::CallMethod	js/src/xpconnect/src/xpcwrappedjsclass.cpp:1697
nsXPCWrappedJS::CallMethod	js/src/xpconnect/src/xpcwrappedjs.cpp:561
PrepareAndDispatch	xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp:114
SharedStub	xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp:141
nsMsgDBView::OnAnnouncerGoingAway	mailnews/base/src/nsMsgDBView.cpp:5663
nsMsgDatabase::NotifyAnnouncerGoingAway	mailnews/db/msgdb/src/nsMsgDatabase.cpp:729
nsMsgDatabase::ForceClosed	mailnews/db/msgdb/src/nsMsgDatabase.cpp:1224
nsMailDatabase::ForceClosed	mailnews/db/msgdb/src/nsMailDatabase.cpp:118
nsImapMailDatabase::ForceClosed	mailnews/db/msgdb/src/nsImapMailDatabase.cpp:129
nsMsgDatabase::CleanupCache	mailnews/db/msgdb/src/nsMsgDatabase.cpp:763
nsGenericModule::Shutdown	nsGenericFactory.cpp:340
nsGenericModule::`scalar deleting destructor'	
nsGenericModule::Release	nsGenericFactory.cpp:245
nsRefPtr<nsThread>::assign_assuming_AddRef	nsAutoPtr.h:944
nsCOMPtr_base::assign_with_AddRef	nsCOMPtr.cpp:89
info_ClearEntry	xpcom/components/nsStaticComponentLoader.cpp:70
PL_DHashTableFinish	pldhash.c:383
nsComponentManagerImpl::Shutdown	xpcom/components/nsComponentManager.cpp:745
NS_ShutdownXPCOM_P	xpcom/build/nsXPComInit.cpp:856
ScopedXPCOMStartup::~ScopedXPCOMStartup	toolkit/xre/nsAppRunner.cpp:951
XRE_main	toolkit/xre/nsAppRunner.cpp:3342

Comment 1

[timeless]

0459:e7774e2e7ca9 (previous:20258) <[e-mail]> 2008-10-14 16:16 +0200
Bug 459656 - Implementing nsIThreadJSContextStack in nsXPConnect. r+sr=mrbkap

the stack is very clear:

http://mxr-test.konigsberg.mozilla.org/bonsai/cvsblame.cgi?file=caps/src/nsScriptSecurityManager.cpp&xrev=a787dec4c28a&tree=mozilla1.9.1&mark=3327,3337,327#322

here's the general code path:
3327 nsScriptSecurityManager::Shutdown()
3337 [e-mail] 20459 NS_IF_RELEASE(sJSContextStack);

 323 nsScriptSecurityManager::GetCurrentJSContext()
 327 [e-mail] 20459 if (NS_FAILED(sJSContextStack->Peek(&cx)))

and here are the key stack frames:
nsScriptSecurityManager::GetCurrentJSConext
NS_ShutdownXPCOM_P

igor: please fix this.

Comment 2

[timeless]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e7774e2e7ca9

Comment 3

[Ludovic Hirlimann [:Usul]]

igor ?

Comment 4

[Wayne Mery (:wsmwk)]

currently #2, this is shaking out to be a topcrash for 3.0b4

Comment 5

[Daniel Veditz [:dveditz]]

Why is bug 459656 at fault? If someone has called nsScriptSecurityManager::Shutdown then no one ought to be running script after that. Maybe nsMsgDBView needs to announce on an earlier event, or the script-running listeners should be killed before that point.

Comment 6

[Ludovic Hirlimann [:Usul]]

STR (copied from 518863) :
Also, if I start TB in normal mode, disable all my extensions, remove Global
Search from the toolbar, restart TB, open a message in tab 2, click on tab 1,
click on Inbox, close TB, TB crashes.

Comment 7

[Wayne Mery (:wsmwk)]

topcrash for 3.0b4 is now definite. steady at #2, so we would want this fixed for 3.0. labeling tb3needs, but feel free to adjust if appropriate

(In reply to comment #6)
> STR (copied from bug 518863) :
> Also, if I start TB in normal mode, disable all my extensions, remove Global
> Search from the toolbar, restart TB, open a message in tab 2, click on tab 1,
> click on Inbox, close TB, TB crashes.

those comments are from bp-6f29c336-2736-4412-b963-f81402090928

Comment 8

[David :Bienvenu]

we're going to need to fix this in our code.

Comment 9

[David :Bienvenu]

I think during the shutdown cleanup caused by our component getting unloaded, we should not be sending these notifications. It's too late...

Comment 10

[David :Bienvenu]

removing tb3needs

Comment 11

[David :Bienvenu]

It's strange that removing the search widget from the toolbar causes all this grief. It also breaks persistence of tabs, so I suspect there's some code there that tries to access that widget and fails, and throws all the way out, past the code that closes the views for the open tabs, and leaving this dangling view. I'll poke around in a bit.

Comment 12

[David :Bienvenu]

now that bug 518863 is fixed, I'll be real curious to see if this stack disappears from nightly builds from tomorrow forward.  I guess any exception that prevented us from closing views on shutdown could cause this crash, so we might need to figure out some way to make the db stop sending notifications once we've gotten past a certain point in shutdown. But I'm not sure that the crash can't occur before we get to the module shutdown phase - I see a ton of assertions if I just close the 3 pane UI with an other mail window open, if I don't have a search bar.

But, CleanupCache is not really useful anymore, so it should probably get turned off. Its main purpose was to shake out memory leaks, but it's no good if it's going to crash.

Comment 13

[David :Bienvenu]

Attachment: proposed fix

by the time we get to unloading modules, it's too late to be sending out notifications, because js code might be gone, etc. It was also hiding some memory leaks/bloat...see Bug 519736

Comment 14

[[e-mail]]

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

>+#ifdef DEBUG
>+    // If you hit this warning, it means that some code is holding onto
>+    // a db at shutdown.
Nit: Doesn't need to be #ifdef because the only caller is already #ifdef

Comment 15

[Wayne Mery (:wsmwk)]

(In reply to comment #12)
> now that bug 518863 is fixed, I'll be real curious to see if this stack
> disappears from nightly builds from tomorrow forward.  I guess any exception
> that prevented us from closing views on shutdown could cause this crash, so we
> might need to figure out some way to make the db stop sending notifications
> once we've gotten past a certain point in shutdown

one crash thus far 20091001 build http://crash-stats.mozilla.com/report/index/2c48c79d-ad9f-4729-b3ac-ce0ae2091002

Comment 16

[David :Bienvenu]

(In reply to comment #14)

> Nit: Doesn't need to be #ifdef because the only caller is already #ifdef

I addressed that nit by changing that - we should still delete m_dbCache in release mode, because we don't want shutdown memory leaks...fix checked in.