440236 - crash after connection lost [@ nsMsgDatabase::GetTableCreateIfMissing(char const*, char const*, nsIMdbTable**, unsigned int&, unsigned int&)], in v2 [@ nsMsgDatabase::GetTableCreateIfMissing]

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[Wayne Mery (:wsmwk)]

crash after connection lost [@ nsMsgDatabase::GetTableCreateIfMissing]

TB44803148
Stack Signature	nsMsgDatabase::GetTableCreateIfMissing 25191134
Product ID	Thunderbird2
Build ID	2008042104
Trigger Time	2008-05-06 05:33:10.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	thunderbird.exe + (00519dcb)
URL visited	
User Comments	Just closing it. However, it was funny: my IMAP mail was unreachable via TB, while via Outlook there was little problem. Note: I lost the internet connection which was caused by my ADSL provider. PJTV
Since Last Crash	258177 sec
Total Uptime	258177 sec
Trigger Reason	Access violation
Source File, Line No.	e:/builds/tinderbox/Tb-Mozilla1.8-Release/WINNT_5.0_Depend/mozilla/mailnews/db/msgdb/src/nsMsgDatabase.cpp, line 1569
Stack Trace 	
nsMsgDatabase::GetTableCreateIfMissing  [mozilla/mailnews/db/msgdb/src/nsMsgDatabase.cpp, line 1569]
nsMailDatabase::GetAllOfflineOpsTable  [mozilla/mailnews/db/msgdb/src/nsMailDatabase.cpp, line 132]
nsImapMailFolder::UpdateImapMailboxInfo  [mozilla/mailnews/imap/src/nsImapMailFolder.cpp, line 2638]
XPTC_InvokeByIndex  [mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp, line 102]
EventHandler  [mozilla/xpcom/proxy/src/nsProxyEvent.cpp, line 561]
nsMsgAccountManager::cleanupOnExit  [mozilla/mailnews/base/src/nsMsgAccountManager.cpp, line 1120]
hashEnumerate  [mozilla/xpcom/ds/nsHashtable.cpp, line 131]
nsHashtable::Enumerate  [mozilla/xpcom/ds/nsHashtable.cpp, line 319]
nsMsgAccountManager::CleanupOnExit  [mozilla/mailnews/base/src/nsMsgAccountManager.cpp, line 1634]
XPTC_InvokeByIndex  [mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp, line 102]
XPCWrappedNative::CallMethod  [mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2169]
XPC_WN_CallMethod  [mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 1455]
js_Invoke  [mozilla/js/src/jsinterp.c, line 1384]
js_Interpret  [mozilla/js/src/jsinterp.c, line 3955]
js_Invoke  [mozilla/js/src/jsinterp.c, line 1403]
js_InternalInvoke  [mozilla/js/src/jsinterp.c, line 1478]
JS_CallFunctionValue  [mozilla/js/src/jsapi.c, line 4363]
nsJSContext::CallEventHandler  [mozilla/dom/src/base/nsJSEnvironment.cpp, line 1493]
nsJSEventListener::HandleEvent  [mozilla/dom/src/events/nsJSEventListener.cpp, line 195]
nsEventListenerManager::HandleEventSubType  [mozilla/content/events/src/nsEventListenerManager.cpp, line 1655]
nsEventListenerManager::HandleEvent  [mozilla/content/events/src/nsEventListenerManager.cpp, line 1762]
nsGlobalWindow::HandleDOMEvent  [mozilla/dom/src/base/nsGlobalWindow.cpp, line 1733]
DocumentViewerImpl::PageHide  [mozilla/layout/base/nsDocumentViewer.cpp, line 1215]
nsDocShell::FirePageHideNotification  [mozilla/docshell/base/nsDocShell.cpp, line 944]
nsDocShell::Destroy  [mozilla/docshell/base/nsDocShell.cpp, line 3586]
nsXULWindow::Destroy  [mozilla/xpfe/appshell/src/nsXULWindow.cpp, line 514]
nsWebShellWindow::Destroy  [mozilla/xpfe/appshell/src/nsWebShellWindow.cpp, line 850]
nsWebShellWindow::HandleEvent  [mozilla/xpfe/appshell/src/nsWebShellWindow.cpp, line 408]
nsWindow::DispatchEvent  [mozilla/widget/src/windows/nsWindow.cpp, line 1319]
nsWindow::DispatchStandardEvent  [mozilla/widget/src/windows/nsWindow.cpp, line 1359]
nsWindow::ProcessMessage  [mozilla/widget/src/windows/nsWindow.cpp, line 4509]
nsWindow::WindowProc  [mozilla/widget/src/windows/nsWindow.cpp, line 1507]
USER32.dll + 0x8724 (0x7e418724)
USER32.dll + 0x8806 (0x7e418806)

Comment 1

[timeless]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/db/msgdb/src/nsMsgDatabase.cpp&mark=1569,1682&rev=MOZILLA_1_8_BRANCH#1569

bienvenu, this code has always scared me....

is there a reason not to cache GetStore() once per function. And why do some people null check, but not everyone?

Comment 2

[Wayne Mery (:wsmwk)]

bienvenu, question ...

(In reply to comment #1)
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/db/msgdb/src/nsMsgDatabase.cpp&mark=1569,1682&rev=MOZILLA_1_8_BRANCH#1569
> 
> bienvenu, this code has always scared me....
> 
> is there a reason not to cache GetStore() once per function. And why do some
> people null check, but not everyone?

#46 crash for 3.0b4. #67 for TB2.0.0.23

Comment 3

[Wayne Mery (:wsmwk)]

(In reply to comment #2)
> bienvenu, question ...
> 
> (In reply to comment #1)
> > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/db/msgdb/src/nsMsgDatabase.cpp&mark=1569,1682&rev=MOZILLA_1_8_BRANCH#1569
> > 
> > bienvenu, this code has always scared me....
> > 
> > is there a reason not to cache GetStore() once per function. And why do some
> > people null check, but not everyone?
> 
> #46 crash for 3.0b4. #67 for TB2.0.0.23

bp-8e1ce453-6ec2-40f4-b51e-cbb7d2081221 "era in standbay" (error in standby?)
bp-59727255-57b1-4927-b567-5ab272091003 "witching folder from left pane (smart folders). Clicking on a saved search including all messages (sent and received) from 3 accounts (8000 msgs)."
bp-6580fb7b-7330-49bc-9bec-a0c092090205 fails to process, but comment is "archiving of most emails in my quite large inbox and clicked on other folder in the folder panel -- the application archived for few minutes, five times I got message that folder_view (or something like that) script is taking too long, clicked 'continue' each time and it suddenly crashed."

Comment 4

[David :Bienvenu]

I could never figure out how the db could be opened successfully without having a store - if we get an m_mdb, we should have a store. We can add more null checks, I suppose, though it always seems to be this CreateTableIfMissing that crashes...I can whip up a patch, I suppose.

Comment 5

[David :Bienvenu]

This has become a top crash in 3.0 pre builds. Taking.

Comment 6

[Wayne Mery (:wsmwk)]

and all OS

Comment 7

[David :Bienvenu]

Attachment: proposed fix

I've checked the callers and they all look like they handle the error returns. Since this is showing up in crash-stats, we should be able to see if this fix squashes the problem.

Comment 8

[[e-mail]]

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

>+  mdb_err err  = m_mdbStore->StringToToken(GetEnv(), scope, &scopeToken);
>+  err = m_mdbStore->StringToToken(GetEnv(), kind, &kindToken);
Hmm, so we don't actually use err here?

>-    err = (nsresult) store->NewTable(GetEnv(), scopeToken,kindToken,
>+    err = (nsresult) m_mdbStore->NewTable(GetEnv(), scopeToken,kindToken,
>                                     PR_FALSE, nsnull, table);
Nit: could do with some whitespace cleanup.
[I'm confused. Does NewTable return an nsresult or an mdb_err?]

Comment 9

[David :Bienvenu]

I removed the err stuff, and the confused cast, and fixed the whitespace...mdb used to return its own kind of errors, but I made them nsresults when I xpcom-ified the interface.

Comment 10

[David :Bienvenu]

this is no longer showing up in crash-stats in recent builds.

Comment 11

[Wayne Mery (:wsmwk)]

(In reply to comment #10)
> this is no longer showing up in crash-stats in recent builds.

agree. last nightly build to crash is 2009100900, and nothing appears so far for 3.0

Comment 12

[Wayne Mery (:wsmwk)]

Bienvenu will this patch apply in v2?  
#11 crash in v2.0.0.23

Comment 13

[David :Bienvenu]

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

yes, it should apply to 1.8.1.next - it's basically a null check.

Comment 14

[Daniel Veditz [:dveditz]]

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

Approved for 1.8.1.24, a=dveditz

Comment 15

[David :Bienvenu]

fixed for 1.8.1.24