Closed Bug 786683 Opened 8 years ago Closed 7 months ago

crash in nsMsgBrkMBoxStore::GetNewMsgOutputStream

Categories

(MailNews Core :: Import, defect, critical)

x86
Windows NT
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 71.0

People

(Reporter: wsmwk, Assigned: aceman)

References

Details

(Keywords: crash, regression, Whiteboard: [rare])

Crash Data

Attachments

(1 file, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-7a58f6cf-510e-489b-b7ee-432a02120822 .
============================================================= 
0	xul.dll	nsMsgBrkMBoxStore::GetNewMsgOutputStream	mailnews/local/src/nsMsgBrkMBoxStore.cpp:626
1	xul.dll	nsOutlookMail::ImportMailbox	mailnews/import/outlook/src/nsOutlookMail.cpp:392
2	xul.dll	ImportOutlookMailImpl::ImportMailbox	mailnews/import/outlook/src/nsOutlookImport.cpp:428
3	xul.dll	ImportMailThread	mailnews/import/src/nsImportMail.cpp:844
4	nspr4.dll	_PR_NativeRunThread	nsprpub/pr/src/threads/combined/pruthr.c:394
5	nspr4.dll	pr_root	nsprpub/pr/src/md/windows/w95thred.c:90
mboxFile is nullptr, so aFolder->GetFilePath(getter_AddRefs(mboxFile)) returns error.
But the return value of GetFilePath is not checked.

Would this suffice?
rv = aFolder->GetFilePath(getter_AddRefs(mboxFile))
NS_ENSURE_SUCCESS(rv, rv);
(In reply to :aceman from comment #2)
> But the return value of GetFilePath is not checked.
> 
> Would this suffice?
> rv = aFolder->GetFilePath(getter_AddRefs(mboxFile))
> NS_ENSURE_SUCCESS(rv, rv);

Probably. That is the normal standard in Mozilla code. Personally though in my own code, I use the non-standard

NS_ENSURE_STATE(mboxFile);

because it is shorter, doesn't need the rv, and doesn't rely on correct behavior of GetFilePath to work.
But it returns a different error code (does not propagate the one that GetFilePath returns). Not sure if that is a problem but may add complexity in tracing.
I am just curious to know how frequent GetFilePath fails.

nsMsgDBFolder::GetFilePath fails only if do_CreateInstance fails. Does it usually happen? Doesn't OOM work?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> I am just curious to know how frequent GetFilePath fails.
> 
> nsMsgDBFolder::GetFilePath fails only if do_CreateInstance fails. Does it
> usually happen? Doesn't OOM work?

native path of nsIFile is invalid, so error code may be NS_ERROR_FILE_UNRECOGNIZED_PATH or NS_ERROR_FAILURE.
Ah, xpcom/io doesn't check file->InitWithFile() return code.  It will be NS_OK.  OOM?
https://crash-stats.mozilla.com/report/index/68e20132-6af5-4c9d-9632-befcc2140227
bp-8011e46e-9315-48a5-b01b-cb89d2150722 version 31

none so far in version 38, but then import is supposed to be disabled in 38
Whiteboard: [do not close]
Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.
Crash Signature: [@ nsMsgBrkMBoxStore::GetNewMsgOutputStream(nsIMsgFolder*, nsIMsgDBHdr**, bool*, nsIOutputStream**)] → [@ nsMsgBrkMBoxStore::GetNewMsgOutputStream(nsIMsgFolder*, nsIMsgDBHdr**, bool*, nsIOutputStream**)] [@ nsMsgBrkMBoxStore::GetNewMsgOutputStream]
This happens much more with older version like 31 and 17. Probably because import is mostly toast
Crash Signature: [@ nsMsgBrkMBoxStore::GetNewMsgOutputStream(nsIMsgFolder*, nsIMsgDBHdr**, bool*, nsIOutputStream**)] [@ nsMsgBrkMBoxStore::GetNewMsgOutputStream] → [@ nsMsgBrkMBoxStore::GetNewMsgOutputStream(nsIMsgFolder*, nsIMsgDBHdr**, bool*, nsIOutputStream**)] [@ nsMsgBrkMBoxStore::GetNewMsgOutputStream] [@ @0x0 | nsMsgBrkMBoxStore::GetNewMsgOutputStream ]
Whiteboard: [do not close] → [rare][do not close]
Some imports still work. But the original report was originating from Outlook import which is disabled right now.
Crash Signature: [@ nsMsgBrkMBoxStore::GetNewMsgOutputStream(nsIMsgFolder*, nsIMsgDBHdr**, bool*, nsIOutputStream**)] [@ nsMsgBrkMBoxStore::GetNewMsgOutputStream] [@ @0x0 | nsMsgBrkMBoxStore::GetNewMsgOutputStream ] → [@ nsMsgBrkMBoxStore::GetNewMsgOutputStream] [@ @0x0 | nsMsgBrkMBoxStore::GetNewMsgOutputStream ]
Flags: needinfo?(acelists)
Whiteboard: [rare][do not close] → [rare]
Attached patch 786683.patch (obsolete) — Splinter Review

Maybe the Outlook import was fixed/refreshed since then.
We could either close this or add the safety check to prevent a similar crash in the future.

Assignee: nobody → acelists
Status: NEW → ASSIGNED
Flags: needinfo?(acelists)
Attachment #9097151 - Flags: review?(jorgk)
Comment on attachment 9097151 [details] [diff] [review]
786683.patch

Look, please check the status on **all four** such calls in the file. Why do we check one only?
Attachment #9097151 - Flags: review?(jorgk)
Attached patch 786683.patchSplinter Review

Sure, no problem.

Attachment #9097151 - Attachment is obsolete: true
Attachment #9097157 - Flags: review?(jorgk)
Comment on attachment 9097157 [details] [diff] [review]
786683.patch

Yes, even with some boy-scout clean-up moving the code into the block on the last hunk.
Attachment #9097157 - Flags: review?(jorgk) → review+
Keywords: checkin-needed

Could be a microscopic perf win :)

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/4f8dc02144b1
check success of GetFilePath() throughout nsMsgBrkMBoxStore.cpp. r=jorgk

Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0
You need to log in before you can comment on or make changes to this bug.