646901 - crash [@ nsMsgXFVirtualFolderDBView::OnSearchHit(nsIMsgDBHdr*, nsIMsgFolder*)]

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Wayne Mery (:wsmwk)]

crash [@ nsMsgXFVirtualFolderDBView::OnSearchHit(nsIMsgDBHdr*, nsIMsgFolder*)]

another potential regression of bug 481824 (3.0b4). I can track it back to at least 3.0.1.  xref Bug 620343 crash @ nsMsgXFGroupThread::FindMsgHdr(nsIMsgDBHdr*)

bp-7b1b4991-ea4a-46be-aff5-685e62110306
EXCEPTION_ACCESS_VIOLATION_READ
0x0
crash happened when i moved 3 messages en bloc from inbox to custom folder
0	thunderbird.exe	nsMsgXFVirtualFolderDBView::OnSearchHit	mailnews/base/src/nsMsgXFVirtualFolderDBView.cpp:304
1	thunderbird.exe	nsMsgSearchSession::AddSearchHit	mailnews/base/search/src/nsMsgSearchSession.cpp:601
2	thunderbird.exe	nsMsgSearchOnlineMail::AddResultElement	mailnews/base/search/src/nsMsgLocalSearch.cpp:818
3	thunderbird.exe	nsImapMailFolder::NotifySearchHit	mailnews/imap/src/nsImapMailFolder.cpp:5624

Comment 1

[Wayne Mery (:wsmwk)]

only a couple per month
bp-e2e10f67-ea56-46d8-a06c-211e02121205 is na example
0	xul.dll	nsMsgXFVirtualFolderDBView::OnSearchHit	mailnews/base/src/nsMsgXFVirtualFolderDBView.cpp:274
1	xul.dll	nsMsgXFVirtualFolderDBView::OnNewHeader	mailnews/base/src/nsMsgXFVirtualFolderDBView.cpp:132
2	xul.dll	nsMsgDBView::OnHdrAdded	mailnews/base/src/nsMsgDBView.cpp:5978
3	xul.dll	nsMsgDatabase::NotifyHdrAddedAll	mailnews/db/msgdb/src/nsMsgDatabase.cpp:870
4	xul.dll	nsMsgDatabase::AddNewHdrToDB	mailnews/db/msgdb/src/nsMsgDatabase.cpp:3437
5	xul.dll	nsParseNewMailState::PublishMsgHeader	mailnews/local/src/nsParseMailbox.cpp:1875
6	xul.dll	nsPop3Sink::IncorporateComplete	mailnews/local/src/nsPop3Sink.cpp:873
7	xul.dll	nsPop3Protocol::HandleLine	mailnews/local/src/nsPop3Protocol.cpp:3570
8	xul.dll	nsPop3Protocol::RetrResponse	mailnews/local/src/nsPop3Protocol.cpp:3356

Comment 2

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

Comment 3

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm marking this bug as WORKSFORME as bug crashlog signature didn't appear from a long time (over half year).

Comment 4

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Ups, my bad and due to ( bug #1348631 ) looks like there are sill crashes, so reopening.

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: bug646901_dbToUse_crash.patch

dbToUse was null

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 9012833 [details] [diff] [review]
bug646901_dbToUse_crash.patch

Looks good as well, thanks.

Comment 7

[Jorg K (CEST = GMT+2)]

Maybe that r+ was too quick. Looking at the context, from the call
  aFolder->GetDBFolderInfoAndDB(getter_AddRefs(folderInfo), getter_AddRefs(dbToUse));
the first return result 'folderInfo' is ignored, and the second is not used on all code paths, only to check dbToUse->HdrIsInCache().

So another option would be only to make this call when needed, and if unsuccessful, not default to the header not being in the cache. What do you think?

Comment 8

[Jorg K (CEST = GMT+2)]

I'm making this comment since you're changing system behaviour here on all code paths even though only one code path is affected by the crash.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: bug646901_dbToUse_crash.patch (JK)

I'm not at home in the view code, perhaps Alta88 can enlighten us whether NS_ENSURE_SUCCESS(rv, rv); is preferable.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: bug646901_dbToUse_crash.patch (JK, v1b)

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9012935 [details] [diff] [review]
bug646901_dbToUse_crash.patch (JK, v1b)

Review of attachment 9012935 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah I guess we can move the code like this. Leaning towards NS_ENSURE_SUCCESS(rv, rv);

Comment 12

[alta88]

Comment on attachment 9012935 [details] [diff] [review]
bug646901_dbToUse_crash.patch (JK, v1b)

Sure, that's better practice to move it where used; the only other code path is a case where a saved search folder is being Quick Filtered. It's possible that what's happening here is a saved search is looking for a message in a folder whose msf (the db) is being rebuilt, ie not open/cached yet. Perhaps it's better to then assume |hdrInCache| is false and add it to the results list, rather than exiting as NS_ENSURE_SUCCESS does and presenting a false list (if i recall correctly how it works).

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 9012935 [details] [diff] [review]
bug646901_dbToUse_crash.patch (JK, v1b)

OK, let's go with the |if (NS_SUCCEEDED(rv)) {| version, I'll remove the comment.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cac40c2c829d
add error checking to fix crash in nsMsgXFVirtualFolderDBView::OnSearchHit(). r=jorgk

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 9012935 [details] [diff] [review]
bug646901_dbToUse_crash.patch (JK, v1b)

Maybe run this through a beta cycle since it's only 98% straight forward.

Comment 16

[Jorg K (CEST = GMT+2)]

Beta (TB 63):
https://hg.mozilla.org/releases/comm-beta/rev/b2e463ce60651a9d20289c0a8874fd0e22bb7730

Comment 17

[Jorg K (CEST = GMT+2)]

TB 60.3 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/83014a6ce917