616222 - crash [@ nsMsgAttachmentHandler::GetMimeDeliveryState(nsIMsgSend**)]

Status: VERIFIED FIXED

(MailNews Core :: MIME, defect)

Description

[Wayne Mery (:wsmwk)]

crash [@ nsMsgAttachmentHandler::GetMimeDeliveryState(nsIMsgSend**)]

bp-6ebc754a-191f-49a8-a690-72a212101120
EXCEPTION_ACCESS_VIOLATION_READ
0x72007504
0	thunderbird.exe	nsMsgAttachmentHandler::GetMimeDeliveryState	mailnews/compose/src/nsMsgAttachmentHandler.cpp:1347
1	thunderbird.exe	nsURLFetcher::OnStartRequest	mailnews/compose/src/nsURLFetcher.cpp:257
2	thunderbird.exe	nsDocumentOpenInfo::OnStartRequest	uriloader/base/nsURILoader.cpp:290
3	thunderbird.exe	nsHttpChannel::CallOnStartRequest	netwerk/protocol/http/src/nsHttpChannel.cpp:848
4	thunderbird.exe	nsHttpChannel::ProcessNormal	netwerk/protocol/http/src/nsHttpChannel.cpp:1155
5	thunderbird.exe	nsHttpChannel::ProcessResponse	netwerk/protocol/http/src/nsHttpChannel.cpp:1024
6	thunderbird.exe	nsHttpChannel::OnStartRequest	netwerk/protocol/http/src/nsHttpChannel.cpp:5190
7	thunderbird.exe	nsInputStreamPump::OnStateStart	netwerk/base/src/nsInputStreamPump.cpp:441
8	thunderbird.exe	nsInputStreamPump::OnInputStreamReady	netwerk/base/src/nsInputStreamPump.cpp:397

Comment 1

[Wayne Mery (:wsmwk)]

#1 mime-related crash
~90 ranked crash for version 13.0.1

bp-994e32c1-f405-4887-9417-a898b2120621 version 13.0.1
bp-8b55941e-8c02-4b78-9ff7-087342120601 Mac v12.0.1 (pzampino)

Comment 2

[Wayne Mery (:wsmwk)]

increased to ~ #60 for version 17.

9eba5a53-6598-4bd1-bfa4-8db782121126

Comment 3

[Makoto Kato [:m_kato]]

The root cause may be that nsMsgAttachmentHandler isn't refcount based object.  Since nsIURLFetcher is async, there is no way whether deleted.

Comment 4

[Wayne Mery (:wsmwk)]

(In reply to Makoto Kato from comment #3)
> The root cause may be that nsMsgAttachmentHandler isn't refcount based
> object.  Since nsIURLFetcher is async, there is no way whether deleted.

thanks Makoto

possible testcase - http://forums.mozillazine.org/viewtopic.php?f=39&t=2711393 / bp-5df7b9b5-021b-4983-aec2-d7ded2130527 - describes where the a file for the user's signature is accessible because the network folder is gone.  bp-3fd191cd-77dd-4a84-83b0-a0ae52130525 adds more flavor about the UI behavior "TB keeps putting a warning on the screen telling me I can't save this email (another reply) as a draft".  (I've not been able to reproduce using daily build and 17.0.6, but perhaps someone else can)

side note - it seems for many months (perhaps even a year or more) I/we've missed identifying nsMsgAttachmentHandler::GetMimeDeliveryState(nsIMsgSend**) as a topcrash, perhaps because I typically check bugs against rankings via signature rather than bug number. Another example where bug 518823 would have helped avoid this problem.

Comment 5

[Wayne Mery (:wsmwk)]

potential testers if we need them:
bp-a1f88dbe-d2ea-42df-bf2e-8998d2130530
bp-ca23583f-404d-4d38-a03d-113932130530
bp-ac2a7d59-1e5c-4ff3-b033-d01d82130521

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Make nsMsgAttachmentHandler ref-countable

To make nsMsgAttachmentHandler ref-countable class nsIMsgSend.getAttachmentHandlers have to be changed. Fortunately nsIMsgSend.getAttachmentHandlers is not scriptable and used only in mailnews core. I hope there is no extension using nsIMsgSend.getAttachmentHandlers.

nsIMsgSend.attachmentCount is not used any more in Thunderbird codes but left there because it's scriptable.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Make nsMsgAttachmentHandler ref-countable rev.2

I had totally forgotten to fix nsURLFetcher.cpp in the previous patch.

Comment 8

[Makoto Kato [:m_kato]]

Comment on attachment 759586 [details] [diff] [review]
Make nsMsgAttachmentHandler ref-countable rev.2

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

::: mailnews/compose/src/nsMsgSend.cpp
@@ +1926,3 @@
>      {
>        nsString   newSpec(NS_LITERAL_STRING("cid:"));
> +      newSpec.Append(NS_ConvertASCIItoUTF16(m_attachments[duplicateOf == -1 ? i : duplicateOf]->m_contentId));

Use newSpec.AppendASCII(...)

@@ +2111,5 @@
>            // We'd like to output a part for the attachment, just an html part
>            // with information about how to download the attachment.
> +          // m_attachments[newLoc]->m_done = true;
> +          attachment->GetHtmlAnnotation(m_attachments[newLoc]->mHtmlAnnotation);
> +          m_attachments[newLoc]->m_type = "text/html";

Use m_attachments[newLoc]-m_type.AssignLiteral(...);

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Make nsMsgAttachmentHandler ref-countable rev.3

Carrying over review+

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/ec86e36132dc

Comment 11

[Scoobidiver (away)]

It's #11 top crasher in TB 17.0.6 and #8 in 17.0.7.

Comment 12

[Mark Banner (:standard8)]

I'd semi-like to take this, but the changes are too substantial for a branch. Given the closeness of TB 24, I think we can just let this ride the trains and ship it in 24.

Comment 13

[Wayne Mery (:wsmwk)]

Thanks Hiro

Signature gone in T24 and newer
https://crash-stats.mozilla.com/query/?product=Thunderbird&version=Thunderbird%3A27.0a1&version=Thunderbird%3A26.0a2&version=Thunderbird%3A26.0a1&version=Thunderbird%3A25.0a2&version=Thunderbird%3A24.0b3&version=Thunderbird%3A24.0b2&version=Thunderbird%3A24.0b1&range_value=4&range_unit=weeks&date=09%2F21%2F2013+15%3A00%3A00&query_search=signature&query_type=contains&query=nsMsgAttachmentHandler%3A%3AGetMimeDeliveryState&reason=&release_channels=&build_id=&process_type=any&hang_type=any