Thunderbird crash [@ MaildirStoreParser::ParseNextMessage(nsIFile*)] - global search

RESOLVED FIXED in Thunderbird 66.0

Status

defect
--
critical
RESOLVED FIXED
6 years ago
5 months ago

People

(Reporter: catalin.rusu, Assigned: jorgk)

Tracking

(Blocks 2 bugs, {crash})

Thunderbird 66.0
x86
All
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [maildir][patchlove], crash signature)

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

6 years ago
User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1599.10 Safari/537.36

Steps to reproduce:

fresh install Thunderbird 24.0b1.exe
changed storage format to Maildir: 
    Tools\Preference\Advanced\General\Config Editor 
    Preferance name: mail.serverDefaultStoreContractID
    Value: @mozilla.org/msgstore/maildirstore;1
restarted Thunderbird
imported my Outlook Express mails [Thunderbird-Tools-Import-Mail-Next-OutlookExpress] 
my Outlook Express email client has aprox 42000 emails organised in many subfolders
the folder structure was inported in thunderbird
no emails were displayed in thunderbird after import, but all emails are available on disk 
deleted *.msf files
in order to rebuild all index files [msf] I created a global search [Body contains " " (space)] which was saved as Search Folder.
click on the saved search folder 
this behaviour does not occur if thunderbird email format is mbox [mail.serverDefaultStoreContractID=@mozilla.org/msgstore/berkeleystore;1]


Actual results:

click on Search Folder 
thunderbitrd crashed


Expected results:

all existing email should be available in search folder
all index files (*.msf) should be rebuilt
I imagine this is backend/db.
Catalin is the only one crashing

[@ MaildirStoreParser::ParseNextMessage(nsIFile*)] 
bp-39175622-150d-4011-bf81-a93232130723
0	xul.dll	MaildirStoreParser::ParseNextMessage(nsIFile *)	mailnews/local/src/nsMsgMaildirStore.cpp
1	xul.dll	MaildirStoreParser::TimerCallback(nsITimer *,void *)	mailnews/local/src/nsMsgMaildirStore.cpp
2	xul.dll	nsTimerImpl::Fire()	xpcom/threads/nsTimerImpl.cpp
3	xul.dll	nsTimerEvent::Run()	xpcom/threads/nsTimerImpl.cpp
4	xul.dll	nsThread::ProcessNextEvent(bool,bool *)	xpcom/threads/nsThread.cpp
5	xul.dll	NS_ProcessNextEvent(nsIThread *,bool)	objdir-tb/mozilla/xpcom/build/nsThreadUtils.cpp
6	xul.dll	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *)	ipc/glue/MessagePump.cpp
Severity: normal → critical
Crash Signature: [@ MaildirStoreParser::ParseNextMessage(nsIFile*)]
Component: Search → Database
Keywords: crash
Product: Thunderbird → MailNews Core
Summary: Thunderbird crash - global search → Thunderbird crash [@ MaildirStoreParser::ParseNextMessage(nsIFile*)] - global search
Whiteboard: [maildir

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [maildir → [maildir]
well-known issue that m_db is null.
bp-93748b72-9dc8-4d79-b61d-b22dc2130817 (signature EMPTY) is also one of catalin's crashes.
(In reply to Makoto Kato (:m_kato) from comment #2)
> well-known issue that m_db is null.
Crash Signature: [@ MaildirStoreParser::ParseNextMessage(nsIFile*)] → [@ MaildirStoreParser::ParseNextMessage(nsIFile*)] [@ EMPTY: no crashing thread identified; corrupt dump ]

Comment 5

5 years ago
I think I am having the same problem here (both with thunderbird and icedove):

Program received signal SIGSEGV, Segmentation fault.
MaildirStoreParser::ParseNextMessage (this=this@entry=0x7fffb99d1a60, aFile=
    0x7fffbabce8c0)
    at /build/icedove-QEY8qr/icedove-24.6.0/mailnews/local/src/nsMsgMaildirStore.cpp:1028
1028	  rv = m_db->CreateNewHdr(nsMsgKey_None, getter_AddRefs(newMsgHdr));
(gdb) p m_db
$11 = {<nsCOMPtr_base> = {mRawPtr = 0x0}, <No data fields>}
#0  MaildirStoreParser::ParseNextMessage (this=this@entry=0x7fffb99d1a60, 
    aFile=0x7fffbabce8c0)
    at /build/icedove-QEY8qr/icedove-24.6.0/mailnews/local/src/nsMsgMaildirStore.cpp:1028
#1  0x00007ffff32916be in MaildirStoreParser::TimerCallback (
    aTimer=<optimized out>, aClosure=0x7fffb99d1a60)
    at /build/icedove-QEY8qr/icedove-24.6.0/mailnews/local/src/nsMsgMaildirStore.cpp:1106
#2  0x00007ffff373bf8a in nsTimerImpl::Fire (this=0x7fffbb5b5ec0)
    at /build/icedove-QEY8qr/icedove-24.6.0/mozilla/xpcom/threads/nsTimerImpl.cpp:543
#3  0x00007ffff373c05f in nsTimerEvent::Run (this=0x7fffffffca98)
    at /build/icedove-QEY8qr/icedove-24.6.0/mozilla/xpcom/threads/nsTimerImpl.cpp:627
#4  0x00007ffff37397cc in nsThread::ProcessNextEvent (this=0x7ffff6c26b60, 
    mayWait=<optimized out>, result=0x7fffffffcccf)
    at /build/icedove-QEY8qr/icedove-24.6.0/mozilla/xpcom/threads/nsThread.cpp:626
#5  0x00007ffff370c654 in NS_ProcessNextEvent (thread=<optimized out>, 
    mayWait=mayWait@entry=false)
    at /build/icedove-QEY8qr/icedove-24.6.0/mozilla/xpcom/build/nsThreadUtils.cpp:238
#6  0x00007ffff342d2e4 in mozilla::ipc::MessagePump::Run (this=0x7fffe565ddc0, 
    aDelegate=0x7fffe564f240)
    at /build/icedove-QEY8qr/icedove-24.6.0/mozilla/ipc/glue/MessagePump.cpp:82
#7  0x00007ffff375949d in RunHandler (this=0x7fffe564f240)
    at /build/icedove-QEY8qr/icedove-24.6.0/mozilla/ipc/chromium/src/base/message_loop.cc:212
#8  MessageLoop::Run (this=0x7fffe564f240)
    at /build/icedove-QEY8qr/icedove-24.6.0/mozilla/ipc/chromium/src/base/message_loop.cc:186
#9  0x00007ffff31bb27f in nsBaseAppShell::Run (this=0x7fffde39b660)
    at /build/icedove-QEY8qr/icedove-24.6.0/mozilla/widget/xpwidgets/nsBaseAppShell.cpp:163
#10 0x00007ffff3090527 in nsAppStartup::Run (this=0x7fffdb3102e0)
    at /build/icedove-QEY8qr/icedove-24.6.0/mozilla/toolkit/components/startup/nsAppStartup.cpp:269
#11 0x00007ffff2774e71 in XREMain::XRE_mainRun (this=this@entry=0x7fffffffcef0)
    at /build/icedove-QEY8qr/icedove-24.6.0/mozilla/toolkit/xre/nsAppRunner.cpp:3856
#12 0x00007ffff2776e54 in XREMain::XRE_main (this=this@entry=0x7fffffffcef0, 
    argc=argc@entry=1, argv=argv@entry=0x7fffffffe2d8, 
    aAppData=aAppData@entry=0x7ffff6c23800)
    at /build/icedove-QEY8qr/icedove-24.6.0/mozilla/toolkit/xre/nsAppRunner.cpp:3924

Comment 6

5 years ago
Looking more into it, seems like ParseFolder is being called with a null mDatabase. In many places the code checks for mDatabase != nullptr but not in this specific code path:

#0  nsMsgMaildirStore::RebuildIndex (this=<optimized out>, 
    aFolder=0x7fffbf39d838, aMsgDB=0x0, aMsgWindow=<optimized out>, 
    aListener=0x7fffbf39d848)
    at /build/icedove-QEY8qr/icedove-24.6.0/mailnews/local/src/nsMsgMaildirStore.cpp:1139
#1  0x00007ffff327e6a0 in nsMsgLocalMailFolder::ParseFolder (
    this=0x7fffbf39d800, aMsgWindow=0x0, aListener=0x7fffbb0733e0)
    at /build/icedove-QEY8qr/icedove-24.6.0/mailnews/local/src/nsLocalMailFolder.cpp:197
#2  0x00007ffff3272a34 in nsMsgSearchOfflineMail::OpenSummaryFile (
    this=0x7fffbb0733a0)
    at /build/icedove-QEY8qr/icedove-24.6.0/mailnews/base/search/src/nsMsgLocalSearch.cpp:292
#3  0x00007ffff3273da6 in nsMsgSearchOfflineMail::Search (this=0x7fffbb0733a0, 
    aDone=0x7fffffffc8ee)
    at /build/icedove-QEY8qr/icedove-24.6.0/mailnews/base/search/src/nsMsgLocalSearch.cpp:707
#4  0x00007ffff32782a1 in nsMsgSearchSession::TimeSliceSerial (
    this=this@entry=0x7fffba4e2c50, aDone=aDone@entry=0x7fffffffc8ee)
    at /build/icedove-QEY8qr/icedove-24.6.0/mailnews/base/search/src/nsMsgSearchSession.cpp:643
#5  0x00007ffff3278315 in nsMsgSearchSession::TimeSlice (
    this=this@entry=0x7fffba4e2c50, aDone=aDone@entry=0x7fffffffc8ee)
Posted patch Check for null db (obsolete) — Splinter Review
I'm not really sure what the root cause of this is, but we should at least prevent the crash, and see if someone can show some other issues in parsing maildir folders.
Assignee: nobody → kent
Status: NEW → ASSIGNED
Attachment #8543578 - Flags: review?(Pidgeot18)
So it looks like the m_db in MaildirStoreParser ultimately comes from here:
<https://hg.mozilla.org/comm-central/annotate/4b97fba5d943/mailnews/local/src/nsLocalMailFolder.cpp#193>.

This suggests that we're being asked to parse the folder without having an open database. I've been too long out of the database code to suggest why this scenario should be occurring, but I'm not sure wallpapering over it is necessarily the best idea.
OK I think you are correct. Looking at the equivalent mbox parser code, opening of the mailDB is a fairly involved process, including setting a backupDB (which was my change) See the code here:

NS_IMETHODIMP nsMsgMailboxParser::OnStartRequest(nsIRequest *request, nsISupports *ctxt)

So it looks like this may be another case of incomplete maildir implementation that I need to fix.
Comment on attachment 8543578 [details] [diff] [review]
Check for null db

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

Cancelling review until the prior comment is addressed.
Attachment #8543578 - Flags: review?(Pidgeot18)
No longer blocks: 1135309

Updated

4 years ago
Crash Signature: [@ MaildirStoreParser::ParseNextMessage(nsIFile*)] [@ EMPTY: no crashing thread identified; corrupt dump ] → [@ MaildirStoreParser::ParseNextMessage(nsIFile*)] [@ EMPTY: no crashing thread identified; corrupt dump ] [@ MaildirStoreParser::ParseNextMessage]
(In reply to Kent James (:rkent) from comment #9)
> OK I think you are correct. Looking at the equivalent mbox parser code,
> opening of the mailDB is a fairly involved process, including setting a
> backupDB (which was my change) See the code here:
> 
> NS_IMETHODIMP nsMsgMailboxParser::OnStartRequest(nsIRequest *request,
> nsISupports *ctxt)
> 
> So it looks like this may be another case of incomplete maildir
> implementation that I need to fix.

straightforward?
Blocks: 1317066
Flags: needinfo?(rkent)
Whiteboard: [maildir] → [maildir][patchlove]
Hi Adam. I see you have this crash running 55.0b1. You might update your beta
Flags: needinfo?(rkent)
mangus, care do dust this one off?
Assignee: rkent → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(mkmelin+mozilla)
OS: Windows XP → All

Updated

5 months ago
Duplicate of this bug: 1506613
Assignee

Comment 16

5 months ago
Posted patch 906469-fix-crash.patch (obsolete) — Splinter Review
I've rebased this and also taken out some URL nonsense. If it's OK to call with nullptr, there's no point to construct a URL for "mailbox://" which has no value.

This needs more cleanup, since the 'delete parser' you can see there is also a foot gun. What you can't see in the patch is:
  MaildirStoreParser *parser = (MaildirStoreParser *) aClosure;

So the TimerCallback() deleted the object it got given, which was created without ref-counting with this code:
  MaildirStoreParser *fileParser = new MaildirStoreParser(aFolder, aMsgDB, ...

We'll save this for another bug.
Assignee: nobody → jorgk
Attachment #8543578 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)
Attachment #9031766 - Flags: review?(mkmelin+mozilla)
Assignee

Comment 17

5 months ago
Sorry, no review on time, so I'll take Kent's patch on my review and I'll add my patch separately.
Attachment #9031766 - Attachment is obsolete: true
Attachment #9031766 - Flags: review?(mkmelin+mozilla)
Attachment #9032016 - Flags: review+
Assignee

Updated

5 months ago
Keywords: leave-open

Comment 18

5 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e756111cbbd0
fix maildir crash while parsing a folder. r=jorgk
Assignee

Comment 19

5 months ago
Further down we already call
  parser->m_listener->OnStopRunningUrl(nullptr, ...);
Attachment #9032017 - Flags: review?(mkmelin+mozilla)
Assignee

Comment 21

5 months ago
Try is green. The code in question is triggered when a maildir folder is repaired. The OnStopRunningUrl() calls goes here:
https://searchfox.org/comm-central/rev/85923675941e2dad376a1ecfdb26e3cf824ea7fc/mailnews/local/src/nsLocalMailFolder.cpp#3138
and later here:
https://searchfox.org/comm-central/rev/85923675941e2dad376a1ecfdb26e3cf824ea7fc/mailnews/base/util/nsMsgDBFolder.cpp#1522

They all cope with null being passed in as the URL and I didn't see a spot where "mailbox://" would receive any useful processing.

I've traced it down into nsresult nsMsgMailNewsUrl::UnRegisterListener() and the mUrlListeners.RemoveElement() returns false since apparently nothing was ever stored on the "mailbox://" URL which serves as a dummy.

But hold on, if we pass null, this won't get executed:
https://searchfox.org/comm-central/rev/85923675941e2dad376a1ecfdb26e3cf824ea7fc/mailnews/base/util/nsMsgDBFolder.cpp#
NotifyFolderEvent(kFolderLoaded);

That might have some undesired side-effects, one never knows in that crufty mailnews code. Let's not run any risk.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 66.0
Assignee

Comment 22

5 months ago
Comment on attachment 9032017 [details] [diff] [review]
906469-follow-up-URL-stuff.patch

Withdrawing this, see comment #21.
Attachment #9032017 - Attachment is obsolete: true
Attachment #9032017 - Flags: review?(mkmelin+mozilla)
You need to log in before you can comment on or make changes to this bug.