782738 - crash at mail startup [@ nsParseNewMailState::MoveIncorporatedMessage]

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Wayne Mery (:wsmwk)]

+++ This bug was initially created as a clone of Bug #713683 +++

regression - crash has returned in version 13. continues in 14. 
bp-699182e9-5b86-44ab-a9f4-3c8062120726 TB15

it is the most common filter crash.
#29 crash for TB14

Comment 1

[Tony Mechelynck [:tonymec]]

According to the Socorro statistics, in the last 4 weeks this crash has been seen on both Thunderbird and SeaMonkey, on all three platforms, and on both 32- and 64-bit architectures (except of course that no Win64 builds exist yet).

Only once (of 1328 crash reports) on a Gecko version higher than 14. That was bp-699182e9-5b86-44ab-a9f4-3c8062120726 supposedly for "Tb 15.0b1" on Windows, with build ID 20120717113334. Otherwise no version "later" than Tb14.0, Sm2.11. Don't know how relevant since "release" builds have much more users (but how much?) than beta, aurora and trunk.

Always for the same crash reason: trying to access location 0x0, which gives EXCEPTION_ACCESS_VIOLATION_READ on Windows, SIGSEGV on Linux, or EXC_BAD_ACCESS / KERN_INVALID_ADDRESS on Mac.

Comment 2

[Kent James (:rkent)]

There is never any check to make sure that newHdr is valid before it is used, unless that is buried in the call to AppendMsgFromStream (which I would never trust anyway). Certainly CopyHdrFromExistingHdr has a variety of ways that it could fail to create newHdr.

Comment 3

[Kent James (:rkent)]

Attachment: Use the existing error handling

I don't particularly like the error message that is given when this fails, but it is what it is, and the existing error handling does nice things like release the semaphore and close the db.

Comment 4

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 665501 [details] [diff] [review]
Use the existing error handling

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

I'm concerned that this will prevent the crash but possibly leave the message DB inconsistent.

::: mailnews/local/src/nsParseMailbox.cpp
@@ +2502,3 @@
>    if (destMailDB)
>      destMailDB->CopyHdrFromExistingHdr(nsMsgKey_None, mailHdr, true,
>                                         getter_AddRefs(newHdr));

We're throwing away the return value from this call, which is probably why we're ending up with null newHdr below (aside from the destMailDB==null possibility).

I'm not sure of the implications of failure here, in terms of whether we should just close up and return the error or call AppendMsgFromStream() with a null newHdr, but CopyHdrFromExistingHdr returns interesting enough nsresult codes that I don't think we should throw them away.

@@ +2504,5 @@
>                                         getter_AddRefs(newHdr));
>    uint32_t messageLength;
>    mailHdr->GetMessageSize(&messageLength);
>    rv = AppendMsgFromStream(inputStream, newHdr, messageLength,
>                             destIFolder);

AppendMsgFromStream() calls nsMsgBrkMBoxStore::GetNewMsgOutputStream(), which tries to create a new message header if one isn't passed in; I suspect AppendMsgFromStream leaks the created header in that case.

That said, GetNewMsgOutputStream around https://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsMsgBrkMBoxStore.cpp#657 also throws away the nsresult if it fails to create the message header; not sure if that really matters in this case.

Based on that, my inclination right now would be to fail early if CopyHdrFromExistingHdr fails rather than proceeding to AppendMsgFromStream. If we proceed the message gets copied and some sort of header gets created, but we don't do all the post-processing around https://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsParseMailbox.cpp#2529 so I'm not confident that the state will be consistent.

Comment 5

[Kent James (:rkent)]

Attachment: Fail earlier

OK I see your points and agree with them. Here's another patch that I think resolves your concerns.

Comment 6

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 667005 [details] [diff] [review]
Fail earlier

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

I'm OK with pushing this patch as is; if you want to remove the destMailDB==null logic then r? me again.

::: mailnews/local/src/nsParseMailbox.cpp
@@ +2509,5 @@
> +    uint32_t messageLength;
> +    mailHdr->GetMessageSize(&messageLength);
> +    rv = AppendMsgFromStream(inputStream, newHdr, messageLength,
> +                             destIFolder);
> +  }

This is always going to fail early if we can't get destMailDB, because we won't even try to get a value for newHdr. I think this is the right thing to do, but it makes all the subsequent code that handles the destMailDB==null case redundant.

Comment 7

[Kent James (:rkent)]

Attachment: Remove redundant tests

As suggested, I expanded the patch to remove redundant tests - not only for destMailDB but also for destIFolder (which would earlier crash if null) and redundant checks of rv.

I don't normally like to do this kind of cleanup in a crash fix, but what the heck why not, we'll never do it at all otherwise.

Comment 8

[Kent James (:rkent)]

Checked in https://hg.mozilla.org/comm-central/rev/500529637889