Last Comment Bug 616222 - crash [@ nsMsgAttachmentHandler::GetMimeDeliveryState(nsIMsgSend**)]
: crash [@ nsMsgAttachmentHandler::GetMimeDeliveryState(nsIMsgSend**)]
Status: VERIFIED FIXED
: crash, topcrash
Product: MailNews Core
Classification: Components
Component: MIME (show other bugs)
: Trunk
: x86 All
: -- critical (vote)
: Thunderbird 24.0
Assigned To: Hiroyuki Ikezoe (:hiro)
:
Mentors:
Depends on: 939391
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-02 11:47 PST by Wayne Mery (:wsmwk, NI for questions)
Modified: 2013-11-15 22:26 PST (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
-
wontfix


Attachments
Make nsMsgAttachmentHandler ref-countable (46.05 KB, patch)
2013-06-06 18:19 PDT, Hiroyuki Ikezoe (:hiro)
no flags Details | Diff | Review
Make nsMsgAttachmentHandler ref-countable rev.2 (49.77 KB, patch)
2013-06-06 20:38 PDT, Hiroyuki Ikezoe (:hiro)
m_kato: review+
Details | Diff | Review
Make nsMsgAttachmentHandler ref-countable rev.3 (49.77 KB, patch)
2013-06-16 03:08 PDT, Hiroyuki Ikezoe (:hiro)
hiikezoe: review+
Details | Diff | Review

Description Wayne Mery (:wsmwk, NI for questions) 2010-12-02 11:47:36 PST
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, NI for questions) 2012-06-22 08:58:09 PDT
#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, NI for questions) 2012-12-28 11:18:17 PST
increased to ~ #60 for version 17.

9eba5a53-6598-4bd1-bfa4-8db782121126
Comment 3 Makoto Kato [:m_kato] 2013-05-06 19:23:32 PDT
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, NI for questions) 2013-05-31 13:51:06 PDT
(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, NI for questions) 2013-05-31 13:57:17 PDT
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) 2013-06-06 18:19:51 PDT
Created attachment 759531 [details] [diff] [review]
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) 2013-06-06 20:38:29 PDT
Created attachment 759586 [details] [diff] [review]
Make nsMsgAttachmentHandler ref-countable rev.2

I had totally forgotten to fix nsURLFetcher.cpp in the previous patch.
Comment 8 Makoto Kato [:m_kato] 2013-06-09 22:49:14 PDT
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) 2013-06-16 03:08:23 PDT
Created attachment 763216 [details] [diff] [review]
Make nsMsgAttachmentHandler ref-countable rev.3

Carrying over review+
Comment 10 Ryan VanderMeulen [:RyanVM] 2013-06-18 08:26:36 PDT
https://hg.mozilla.org/comm-central/rev/ec86e36132dc
Comment 11 Scoobidiver (away) 2013-06-27 09:36:35 PDT
It's #11 top crasher in TB 17.0.6 and #8 in 17.0.7.
Comment 12 Mark Banner (:standard8) 2013-08-01 07:53:49 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.