1343536 - Dragging message/rfc822 attachment to attachment bucket of new message and double-clicking it in the attachment bucket crashes

Status: RESOLVED FIXED

(Thunderbird :: General, defect, P2)

Description

[Jorg K (CEST = GMT+2)]

This is the ugly sister of bug 1343488. It was first reported in bug 1316416 comment #37, but that bug has a hotchpotch of various crashes, mostly irreproducible, that end up in a stack overflow.

This crash here is easy to reproduce with the following steps:

STR:
Attach a BMO message which has CTE quoted-printable to a message and send.
Result: message/rfc822 attachment looks good in the preview pane.

Compose a new message and drag the message/rfc822 attachment from the sent message to the attachment bucket.

Double click the attachment in the attachment bucket - Crash!
https://crash-stats.mozilla.com/report/index/bp-e49fbf9b-7fc3-4024-8fa7-243512170301
https://crash-stats.mozilla.com/report/index/bp-aa5a6b84-2661-47b8-8365-efd0c2170301

Using TB 45.x, double clicking the attachment in the attachment bucket is just ignored.

If you look at the crash, there seems to be an endless loop of these four calls:
52 	xul.dll 	nsStreamConverter::OnStopRequest(nsIRequest*, nsISupports*, nsresult) 	C:/builds/moz2_slave/tb-c-cen-w64-ntly-000000000000/build/mailnews/mime/src/nsStreamConverter.cpp:1081
53 	xul.dll 	nsDocumentOpenInfo::OnStopRequest(nsIRequest*, nsISupports*, nsresult) 	uriloader/base/nsURILoader.cpp:340
54 	xul.dll 	nsUnknownDecoder::OnStopRequest(nsIRequest*, nsISupports*, nsresult) 	netwerk/streamconv/converters/nsUnknownDecoder.cpp:301
55 	xul.dll 	nsDocumentOpenInfo::OnStopRequest(nsIRequest*, nsISupports*, nsresult) 	uriloader/base/nsURILoader.cpp:340

In the end you get a stack overflow.

Comment 1

[Jorg K (CEST = GMT+2)]

Wayne, what is the URL used for?

Comment 2

[Wayne Mery (:wsmwk)]

I used wrong field. ("see also", to indicate a relationship not as strong as dependency)

Comment 3

[Jorg K (CEST = GMT+2)]

OK, here comes the initial investigation.

Here some stack dumps:

nsMimeBaseEmitter::Complete() finds that there is data left and calls OnDataAvailable() while running in an OnStopRequest(). Weird.

nsStreamConverter::OnStartRequest(nsIRequest * request, nsISupports * ctxt) Line 969	C++
nsDocumentOpenInfo::OnStartRequest(nsIRequest * request, nsISupports * aCtxt) Line 289	C++
nsUnknownDecoder::FireListenerNotifications(nsIRequest * request, nsISupports * aCtxt) Line 680	C++
nsUnknownDecoder::OnDataAvailable(nsIRequest * request...) Line 210	C++
nsDocumentOpenInfo::OnDataAvailable(nsIRequest * request, ...) Line 325	C++
nsMimeBaseEmitter::Complete() Line 1051	C++
nsStreamConverter::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult status) Line 1067	C++
nsMsgProtocol::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult aStatus) Line 340	C++
nsMailboxProtocol::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult aStatus) Line 383	C++

A little later we have this situation:

nsStreamConverter::OnStartRequest(nsIRequest * request, nsISupports * ctxt) Line 969	C++
nsDocumentOpenInfo::OnStartRequest(nsIRequest * request, nsISupports * aCtxt) Line 289	C++
nsUnknownDecoder::FireListenerNotifications(nsIRequest * request, nsISupports * aCtxt) Line 680	C++
nsUnknownDecoder::OnStopRequest(nsIRequest * request, nsISupports * aCtxt, nsresult aStatus) Line 291	C++
nsDocumentOpenInfo::OnStopRequest(nsIRequest * request, nsISupports * aCtxt, nsresult aStatus) Line 341	C++
nsStreamConverter::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult status) Line 1088	C++
nsDocumentOpenInfo::OnStopRequest(nsIRequest * request, nsISupports * aCtxt, nsresult aStatus) Line 341	C++
nsUnknownDecoder::OnStopRequest(nsIRequest * request, nsISupports * aCtxt, nsresult aStatus) Line 302	C++
nsDocumentOpenInfo::OnStopRequest(nsIRequest * request, nsISupports * aCtxt, nsresult aStatus) Line 341	C++
nsStreamConverter::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult status) Line 1088	C++
nsMsgProtocol::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult aStatus) Line 340	C++
nsMailboxProtocol::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult aStatus) Line 383	C++

So nsUnknownDecoder::OnStopRequest() calls nsUnknownDecoder::FireListenerNotifications() and that calls nsDocumentOpenInfo::OnStartRequest(). Weird.

Next we get:

nsStreamConverter::OnStartRequest(nsIRequest * request, nsISupports * ctxt) Line 969	C++
nsDocumentOpenInfo::OnStartRequest(nsIRequest * request, nsISupports * aCtxt) Line 289	C++
nsUnknownDecoder::FireListenerNotifications(nsIRequest * request, nsISupports * aCtxt) Line 680	C++
nsUnknownDecoder::OnStopRequest(nsIRequest * request, nsISupports * aCtxt, nsresult aStatus) Line 291	C++
nsDocumentOpenInfo::OnStopRequest(nsIRequest * request, nsISupports * aCtxt, nsresult aStatus) Line 341	C++
nsStreamConverter::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult status) Line 1088	C++
nsDocumentOpenInfo::OnStopRequest(nsIRequest * request, nsISupports * aCtxt, nsresult aStatus) Line 341	C++
nsUnknownDecoder::OnStopRequest(nsIRequest * request, nsISupports * aCtxt, nsresult aStatus) Line 302	C++
nsDocumentOpenInfo::OnStopRequest(nsIRequest * request, nsISupports * aCtxt, nsresult aStatus) Line 341	C++
nsStreamConverter::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult status) Line 1088	C++
nsDocumentOpenInfo::OnStopRequest(nsIRequest * request, nsISupports * aCtxt, nsresult aStatus) Line 341	C++
nsUnknownDecoder::OnStopRequest(nsIRequest * request, nsISupports * aCtxt, nsresult aStatus) Line 302	C++
nsDocumentOpenInfo::OnStopRequest(nsIRequest * request, nsISupports * aCtxt, nsresult aStatus) Line 341	C++
nsStreamConverter::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult status) Line 1088	C++
nsMsgProtocol::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult aStatus) Line 340	C++
nsMailboxProtocol::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult aStatus) Line 383	C++

So there's the infinite recursion. Honza, could you shed some light onto this? It looks that Necko got itself in a bit of a knot here.

Comment 4

[Jorg K (CEST = GMT+2)]

In nsMimeBaseEmitter::Complete() I removed the call to nsDocumentOpenInfo::OnDataAvailable(). That had no effect. I still get endless recursion. Previous debugging showed that nsMimeBaseEmitter::Complete() only calls nsDocumentOpenInfo::OnDataAvailable() once, so it's not part of the infinite recursion loop.

Comment 5

[Honza Bambas (:mayhemer)]

I'll build TB and try to repro.  I think there are two doc infos involved.

Comment 6

[Honza Bambas (:mayhemer)]

What is the priority of this bug?

Comment 7

[Jorg K (CEST = GMT+2)]

Hi Honza, nice to see you and even nicer that you assigned the bug to yourself. To answer your question: It's a crash, and crashes are not nice. On the other hand, the steps are a little obscure:
Send a message (1) that has another message (2) attached. Then create a new composition (3) and drag the attached message (2) from the sent message (1) into the composition. Then double-click it in the attachment bucket.

Who does that? I did since I didn't want to go looking for the original message again when attaching it a second time, so I dragged from the sent message. That was overall a bad idea since the dragged message will be sent garbled (bug 1343488).

I didn't answer your question so far: Well, I'd say it's not very important. Any fix in the next few weeks (or even months) would be fine. I tried looking at it myself briefly, but I got a little confused by layers of netwerk calls.

Comment 8

[Honza Bambas (:mayhemer)]

Thanks.  I was looking at some of the crashes in bug 1316416 and they are way different than this one.  So I rather double checked what the priority of this one was.  It's low.

Comment 9

[Wayne Mery (:wsmwk)]

(In reply to Jorg K (GMT+2) from comment #7)
> Hi Honza, nice to see you and even nicer that you assigned the bug to
> yourself. To answer your question: It's a crash, and crashes are not nice.
> On the other hand, the steps are a little obscure:
> ...
> Who does that? 
> ...

Thunderbird has very few actionable crash reports, so perhaps more relevant is crash rank, the lucky fact that this has steps to reproduce, and we don't know these are the only steps to reproduce.  Rankings #76 for production and #38 for beta suggest we should follow up when the resources allow.

Comment 10

[Honza Bambas (:mayhemer)]

I'll try to help (if no action in two weeks, I will release this bug again tho)

Comment 11

[Wayne Mery (:wsmwk)]

(In reply to Honza Bambas (:mayhemer) from comment #10)
> I'll try to help (if no action in two weeks, I will release this bug again tho)

This takes on added importance given bug 1411735 which has the testcase (on which you were copied a couple months ago). 
It would be nice for us to ship a fix for 60.
Might you have time to look at it now?

Comment 12

[Wayne Mery (:wsmwk)]

"I attached attachment from another email to new email and tried to open attachement from new email, it crashes." bp-3bcc39cd-e867-41ed-b972-550bd0180118

"I want to send email again with corrected address but can't get it to happen." bp-3684a841-3ac1-4fa2-9e1a-107690180109

Comment 13

[Honza Bambas (:mayhemer)]

P2 (feel free to adjust)

I'll take a look and try to deliver this for 60.  The most helpful would be to have a good steps to reproduce.  Is the STR from comment 0 still usable?  Thanks.

Comment 14

[Jorg K (CEST = GMT+2)]

(In reply to Honza Bambas (:mayhemer) from comment #13)
> Is the STR from comment 0 still usable? 
Absolutely ;-)

Comment 15

[Honza Bambas (:mayhemer)]

c-c@d45c9605a8e4
m-c@5e9bd04333f2

nsDocumentOpenInfo::DispatchContent:

    // XXXbz have to be careful here; may end up in some sort of bizarre infinite
    // decoding loop.


yeah.. we may :)




We are starting here (below stack) when the attachment is dblclicked:


 	xul.dll!nsStreamConverterService::AsyncConvertData(0x1b0cba7c, 0x1b0cba54, 0x0bf600a0, 0x08754780, 0x006f8eb4) Line 462	C++	Symbols loaded.
	xul.dll!nsImapMockChannel::SetupPartExtractorListener(0x0df80100, 0x0bf600a0) Line 9740	C++	Symbols loaded.
 	xul.dll!nsImapMockChannel::ReadFromLocalCache() Line 9616	C++	Symbols loaded.
 	xul.dll!nsImapMockChannel::AsyncOpen(0x0bf600a0, 0x00000000) Line 9700	C++	Symbols loaded.
 	xul.dll!nsImapMockChannel::AsyncOpen2(0x0bf600a0) Line 9725	C++	Symbols loaded.
 	xul.dll!nsURILoader::OpenURI(0x08754780, 1, 0x0bbfea30) Line 840	C++	Symbols loaded.
0 OpenSelectedAttachment([object XULCommandEvent]) ["chrome://messenger/content/messengercompose/MsgComposeCommands.js":5262]
    this = [object XULElement]
1 onxblclick(event = [object MouseEvent]) ["chrome://messenger/content/mailWidgets.xml":483]
    this = [object XULElement]


This creates comm-central\mailnews\mime\src\nsStreamConverter.cpp instance for "message/rfc822" -> "*/*".  That stream converter tries to find a target type from the URL spec.  It finds "part=" in the query but doesn't find "type=".  Hence, it bails and falls back to nsMimeOutput::nsMimeMessageRaw what makes the converter target format be "application/x-unknown-content-type".  This is probably fine at the moment.

Note that the loading channel's (the mock channel's) content type is set to this type at [2].


Then the mock channel starts pushing content and we get here:

	xul.dll!nsDocumentOpenInfo::ConvertData(0x08754780, 0x0bbfeac0, {...}, {...}) Line 624	C++	Symbols loaded.
 	xul.dll!nsDocumentOpenInfo::DispatchContent(0x08754780, 0x00000000) Line 516	C++	Symbols loaded.
 	xul.dll!nsDocumentOpenInfo::OnStartRequest(0x08754780, 0x00000000) Line 295	C++	Symbols loaded.
 	xul.dll!nsStreamConverter::FirePendingStartRequest() Line 1150	C++	Symbols loaded.
 	xul.dll!mime_output_fn(0x0c0bf000, 3208, 0x0bf9b0d0) Line 850	C++	Symbols loaded.
 	xul.dll!MimeOptions_write(0x21238b50, 0x0c3c27c0, 0x0c0bf000, 3208, true) Line 1742	C++	Symbols loaded.
 	xul.dll!MimeHeaders_write_raw_headers(0x21238b50, 0x0c3c27c0, false) Line 828	C++	Symbols loaded.
 	xul.dll!MimeMessage_close_headers(0x21b07740) Line 455	C++	Symbols loaded.
 	xul.dll!MimeMessage_parse_line(0x22814c00, 2, 0x21b07740) Line 257	C++	Symbols loaded.
 	xul.dll!convert_and_send_buffer(0x22814c00, 2, true, 0x12daca30, 0x21b07740) Line 152	C++	Symbols loaded.
 	xul.dll!mime_LineBuffer(0x006fdcac, 2, 0x21b0775c, 0x21b07764, 0x21b0776c, true, 0x12daca30, 0x21b07740) Line 238	C++	Symbols loaded.
 	xul.dll!MimeObject_parse_buffer(0x006fdcac, 2, 0x21b07740) Line 240	C++	Symbols loaded.
 	xul.dll!MimeMultipart_parse_child_line(0x21b076f0, 0x2280e400, 54, false) Line 653	C++	Symbols loaded.
 	xul.dll!MimeMultipart_parse_line(0x2280e400, 56, 0x21b076f0) Line 306	C++	Symbols loaded.
 	xul.dll!convert_and_send_buffer(0x2280e400, 56, true, 0x12db0520, 0x21b076f0) Line 152	C++	Symbols loaded.
 	xul.dll!mime_LineBuffer(0x2229b800, 56, 0x21b0770c, 0x21b07714, 0x21b0771c, true, 0x12db0520, 0x21b076f0) Line 238	C++	Symbols loaded.
 	xul.dll!MimeObject_parse_buffer(0x2229b800, 56, 0x21b076f0) Line 240	C++	Symbols loaded.
 	xul.dll!MimeMessage_parse_line(0x2229b800, 56, 0x20deb600) Line 212	C++	Symbols loaded.
 	xul.dll!convert_and_send_buffer(0x2229b800, 56, true, 0x12daca30, 0x20deb600) Line 152	C++	Symbols loaded.
 	xul.dll!mime_LineBuffer(0x0b847080, 1154, 0x20deb61c, 0x20deb624, 0x20deb62c, true, 0x12daca30, 0x20deb600) Line 238	C++	Symbols loaded.
 	xul.dll!MimeObject_parse_buffer(0x0b846000, 5378, 0x20deb600) Line 240	C++	Symbols loaded.
 	xul.dll!mime_display_stream_write(0x00e66260, 0x0b846000, 5378) Line 905	C++	Symbols loaded.
 	xul.dll!nsStreamConverter::OnDataAvailable(0x08754780, 0x00000000, 0x0bf18fa0, 0, 5378) Line 937	C++	Symbols loaded.
>	xul.dll!nsImapCacheStreamListener::OnDataAvailable(0x0c36e2a0, 0x00000000, 0x0bf18fa0, 0, 5378) Line 8915	C++	Symbols loaded.


The reason we convert again is that none of the listeners of the documentopeninfo can (or better said want to, [1]) handle "message/rfc822" content type.

And here starts the dance...  It runs a nested stream conversion from "application/x-unknown-content-type" to "*/*".  This creates nsUnknownDecoder, which correctly, via registered sniffers, recognizes the content as "message/rfc822" and, for a brief time, it updates the mock channel's content type to it.  We hit the following stack:

	xul.dll!nsDocumentOpenInfo::ConvertData(0x08754780, 0x0bbfeac0, {...}, {...}) Line 624	C++	Symbols loaded.
 	xul.dll!nsDocumentOpenInfo::DispatchContent(0x08754780, 0x0df80108) Line 516	C++	Symbols loaded.
 	xul.dll!nsDocumentOpenInfo::OnStartRequest(0x08754780, 0x0df80108) Line 295	C++	Symbols loaded.
 	xul.dll!nsUnknownDecoder::FireListenerNotifications(0x08754780, 0x0df80108) Line 767	C++	Symbols loaded.
 	xul.dll!nsUnknownDecoder::OnDataAvailable(0x08754780, 0x0df80108, 0x0bf18d00, 512, 3808) Line 231	C++	Symbols loaded.
 	xul.dll!nsDocumentOpenInfo::OnDataAvailable(0x08754780, 0x0df80108, 0x0bf18d00, 0, 4320) Line 340	C++	Symbols loaded.
 	xul.dll!nsMimeBaseEmitter::Complete() Line 1041	C++	Symbols loaded.
 	xul.dll!nsStreamConverter::OnStopRequest(0x08754780, 0x00000000, NS_OK) Line 1064	C++	Symbols loaded.
 	xul.dll!nsImapCacheStreamListener::OnStopRequest(0x0c36e2a0, 0x00000000, NS_OK) Line 8899	C++	Symbols loaded.


The mock channel's C-T = message/rfc822, each nsDocumentOpenInfo is a different object since they nest (every conversion is targeted to a new openinfo.)

Anyway, we then try to convert "message/rfc822" to 'something' again.  We end up with the same stream converter as initially used in nsImapMockChannel::SetupPartExtractorListener.  And this is how the loop is built...


Jorg, if you have any ideas how to fix this or what is the actual expected outcome, let me know.  I think the most important for me is to understand the following bits:
- what is the expected outcome of the dblclick, what is the expected target consumer to handle the content and what is the expected content type?
- how exactly the nsMimeBaseEmitter thing works?

If you don't have any idea, then let me know as well, I'll try to dive a bit more into this.  


[1] https://dxr.mozilla.org/comm-central/rev/228f05fcbd6c04911e6e32e4df911ff51dec0fbd/mail/components/compose/content/MsgComposeCommands.js#5294
[2] https://dxr.mozilla.org/comm-central/rev/228f05fcbd6c04911e6e32e4df911ff51dec0fbd/mailnews/mime/src/nsStreamConverter.cpp#583

Comment 16

[Honza Bambas (:mayhemer)]

*** c-c@552fd5ab0b44 !!!

Comment 17

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/552fd5ab0b44
Please paste full URLs, so I can click them ;-) Yes, we don't build with Stylo on buildbot, but we do build with it on TaskCluster and some of our developers build with it locally (like myself).

Now to the bug at hand:

From the user perspective, a message can be attached to a new composition in two ways:
1) You drag a message from the thread pane/message list into the so-called attachment bucket.
   That works fine and you can later double-click that attached message and it will open.
   The URL as reflected in the tooltip is: mailbox-message: ... or imap-message: ...
2) You drag a message that is already attached to a new composition.
   When double-clicking to open it, it crashes.
   The URL as reflected in the tooltip is: mailbox: ... ?number=xx&part=1.2?filename=xx.eml
From the user's perspective those actions are almost the same, the user doesn't care where the message originates from.

Oh, as an aside, I see IMAP in your stack, are you doing this with an IMAP message being the source? I've only ever tried from a local folder, but perhaps that doesn't make a difference.

Now coming to your question: The frank answer is: I have no idea, but I do know the expected outcome: Double-clicking the attached message in the bucket should open it and not crash.

Do you know why case 1) and 2) are so different? Why does opening a mailbox-message: URL work and a mailbox: ... ?number=xx&part=1.2?filename=xx.eml crash?

When I double-click the source message attachment, it would open the mailbox: ... ?number=xx&part=1.2?filename=xx.eml URL, no? Why is it different when this is placed into the attachment bucket?

That aside, you mention that some converter is looking for a type and doesn't find one. Could a type simply be added when adding the attachment around here:
https://dxr.mozilla.org/comm-central/rev/228f05fcbd6c04911e6e32e4df911ff51dec0fbd/mail/components/compose/content/MsgComposeCommands.js#5817

Sorry if I haven't been helpful, I'm a "jack of all trades, master of none" on the project, and I haven't dug too deep into the MIME code.

Comment 18

[Honza Bambas (:mayhemer)]

Jorg, this was not helpful, sorry.  I'm a platform/fx engineer, and I don't know c-c internals at all, so I really can't answer most of the technical questions.  I see all of this the first time.  

Do you know about anyone who could know this code?  I'd like to go the guess/investigate-myself path only as the last resort.

IMAP: yes, I'm using a test IMAP account.  the message I'm dragging the attachment from is an imap sent message.

I'll try how the things work when the attachment is clicked at the sent email and not in the attachment bucket of a new message.  maybe it will tell something.

Thanks.

Comment 19

[Honza Bambas (:mayhemer)]

So, clicking an attachment in a message directly uses a URL with &type=message/rfc822 in it, hence the stream converter correctly recognizes it as application/x-message-display content type and handler is found for it.

Looks like updating the URL in attachment bucket would solve this, but I have no idea how it's constructed.

Comment 20

[Jorg K (CEST = GMT+2)]

(In reply to Honza Bambas (:mayhemer) from comment #19)
> Looks like updating the URL in attachment bucket would solve this, but I
> have no idea how it's constructed.
Sorry I wasn't helpful before. The type could be added to the URL here:
https://dxr.mozilla.org/comm-central/rev/228f05fcbd6c04911e6e32e4df911ff51dec0fbd/mail/components/compose/content/MsgComposeCommands.js#5817

To test this, simply do
   attachment.url = rawData + "&type=message/rfc822";
at https://dxr.mozilla.org/comm-central/rev/228f05fcbd6c04911e6e32e4df911ff51dec0fbd/mail/components/compose/content/MsgComposeCommands.js#5866
as a hack. Given your analysis, this should work and I can integrate this into the "attaching logic" in a patch and get this reviewed.

Sounds like a plan?

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to Jorg K (GMT+1) from comment #20)
> (In reply to Honza Bambas (:mayhemer) from comment #19)
> > Looks like updating the URL in attachment bucket would solve this, but I
> > have no idea how it's constructed.
> Sorry I wasn't helpful before. The type could be added to the URL here:
> https://dxr.mozilla.org/comm-central/rev/
> 228f05fcbd6c04911e6e32e4df911ff51dec0fbd/mail/components/compose/content/
> MsgComposeCommands.js#5817
> 
> To test this, simply do
>    attachment.url = rawData + "&type=message/rfc822";
> at
> https://dxr.mozilla.org/comm-central/rev/
> 228f05fcbd6c04911e6e32e4df911ff51dec0fbd/mail/components/compose/content/
> MsgComposeCommands.js#5866
> as a hack. Given your analysis, this should work and I can integrate this
> into the "attaching logic" in a patch and get this reviewed.
> 
> Sounds like a plan?

The hack works.

I can't find you on irc...

I think a bit better approach would be to use the result of the unknown decoder.  It recognizes the message as message/rfc822.  It should be used to hint the comm-central/streamconverter to use it when it's not found in the URL.

OTOH, if any message attachment (an .eml file) will be message/rfc822, then we can add it manually to the URL as you suggested.  I don't know how the thing in onDrop exactly works, so I'm not sure where to add the type= arg exactly.

To prevent the crash in the future would be a platform change in URILoader, that would not be that simple (prevent the loop).  The user experience here would not be a crash, but inability to view the content - better than now, tho.  I'll file a separate bug for this, I think a depth limit might be the simplest and most deterministic solution.

Comment 22

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #21)
> I think a bit better approach would be to use the result of the unknown
> decoder.  It recognizes the message as message/rfc822.  It should be used to
> hint the comm-central/streamconverter to use it when it's not found in the
> URL.

Probably a nonsense as the stream converter is registered only for message/rfc822:
https://searchfox.org/comm-central/source/mailnews/mime/public/nsMsgMimeCID.h#9-16

Comment 23

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #21)
> To prevent the crash in the future would be a platform change in URILoader,
> that would not be that simple (prevent the loop).  The user experience here
> would not be a crash, but inability to view the content - better than now,
> tho.  I'll file a separate bug for this, 

bug 1440663

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: 1343536-add-type.patch

Hours of debugging led to a 20-minute fix. That's quite common :-(

Honza, many thanks for your help. This would be my suggestion.

I'm not sure whether you want to pursue this idea further:
> I think a bit better approach would be to use the result of the unknown decoder.

Comment 25

[Honza Bambas (:mayhemer)]

Comment on attachment 8953435 [details] [diff] [review]
1343536-add-type.patch

wfm, thanks.

Comment 27

[Jorg K (CEST = GMT+2)]

Honza, thanks again for the cooperation! I'll land this with "research by Honza Bambas" in the check-in comment.

Comment 28

[Honza Bambas (:mayhemer)]

(In reply to Jorg K (GMT+1) from comment #27)
> Honza, thanks again for the cooperation! I'll land this with "research by
> Honza Bambas" in the check-in comment.

:) pleasure to help.

Comment 29

[:aceman]

Comment on attachment 8953435 [details] [diff] [review]
1343536-add-type.patch

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

Great work, works for me.

Comment 30

[Jorg K (CEST = GMT+2)]

Attachment: 1343536-add-type.patch (v2)

Sadly the previous patch was wrong, it added "type=message/rfc822" to everything dragged out of the attachments area of an existing message.

Now I'm checking for filename and if I find ".eml", I add the type.

Thinking about this some more, this seems like a kludge. If, for example, I drag over a PDF, the URL added to the bucket is:
mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/somefolder?number=8&part=1.2&filename=xx.pdf
and there is no problem opening that later.

So why doesn't the system associate ".eml" with "type=message/rfc822"? Wouldn't it be the cleaner way to fix this?

Comment 31

[:aceman]

Comment on attachment 8953719 [details] [diff] [review]
1343536-add-type.patch (v2)

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

Thanks.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5843,5 @@
> +              // supports attachments. We don't want to hard-code mailbox:, imap:
> +              // and all the others here.
> +              if (Services.io.getProtocolHandler(scheme) instanceof
> +                    Components.interfaces.nsIMsgMessageFetchPartService &&
> +                  !rawData.includes("type=")) {

Should this be '&type=' for safety? Or can 'type=' be at the very start of the rawData?

@@ +5847,5 @@
> +                  !rawData.includes("type=")) {
> +                // Here we must add type=message/rfc822 to the URL if we're
> +                // attaching a message. Otherwise the MIME handler will not be
> +                // able to open the attachment from the bucket later.
> +                let fnMatch = /[?&;]filename=([^?&]+)/.exec(rawData);

Isn't there a proper library to get these arguments from the query part of an URI?
Yeah, rawData is a string, so either we could convert it into nsIURI, or make a library of helpers to cut out args from strings. We already have removeQueryPart() in this same file.

Comment 32

[Jorg K (CEST = GMT+2)]

I'm still not convinced that this is the right solution, see comment #30. So I'll await Honza's answer.

(In reply to :aceman from comment #31)
> Should this be '&type=' for safety? Or can 'type=' be at the very start of
> the rawData?
In all the cases I've seen, there was no type at all, certainly not for ".eml" attachments. We could drop this clause altogether. type is not likely to be at the start, since the query typically starts with ?number=. 

> Isn't there a proper library to get these arguments from the query part of
> an URI?
Only in C++: MsgExtractQueryPart() in nsMsgUtils.cpp.

> Yeah, rawData is a string, so either we could convert it into nsIURI, ...
Not useful, since that will only give you the query, not its parts.

> ... or make a library of helpers to cut out args from strings. We already have
> make removeQueryPart() in this same file.
Yes, possible, you could just turn those two lines into a function.

Right now I'd like to decide whether to pursue this approach at all.

Comment 33

[Honza Bambas (:mayhemer)]

(In reply to Jorg K (GMT+1) from comment #30)
> Created attachment 8953719 [details] [diff] [review]
> 1343536-add-type.patch (v2)
> 
> Sadly the previous patch was wrong, it added "type=message/rfc822" to
> everything dragged out of the attachments area of an existing message.
> 
> Now I'm checking for filename and if I find ".eml", I add the type.
> 
> Thinking about this some more, this seems like a kludge. If, for example, I
> drag over a PDF, the URL added to the bucket is:
> mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.
> testing/Mail/Local%20Folders/somefolder?number=8&part=1.2&filename=xx.pdf
> and there is no problem opening that later.
> 
> So why doesn't the system associate ".eml" with "type=message/rfc822"?
> Wouldn't it be the cleaner way to fix this?

I really don't know the c-c code in this area, so I'm not sure why you are asking me about this.  I'm not sure I follow what you mean with "system associates" here, but after bug 1440663 lands the "open or save as" dialog will pop up instead for unrecognized (type= missing) attachments.

Comment 34

[Jorg K (CEST = GMT+2)]

Attachment: 1343536-alternative.patch

I went back reading through the bug. In comment #15 Honza said that the attachment opener doesn't handle anything:
https://dxr.mozilla.org/comm-central/rev/228f05fcbd6c04911e6e32e4df911ff51dec0fbd/mail/components/compose/content/MsgComposeCommands.js#5294

So from that observation I derived this patch, code copied from
https://dxr.mozilla.org/comm-central/rev/1c9918ea5681de850f01054db233247086368873/mail/components/nsMailDefaultHandler.js#119.

This seems to work and is less of a kludge than adding the type to certain attachments upon drop.

Comment 35

[:aceman]

But how do we know in this code it is a message attached?

Comment 36

[Jorg K (CEST = GMT+2)]

As far as I understand it, the isPreferred() function says: I'll only handle "message/rfc822", so doContent() only gets presented this type. Copied from where I said is was copied from. I guess Honza can tell us whether there is something missing.

Comment 37

[Honza Bambas (:mayhemer)]

Comment on attachment 8954067 [details] [diff] [review]
1343536-alternative.patch

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

I can't say I would be an expert, but from what I know about content handlers, this seems to be the correct solution!  Thanks.

Comment 38

[Honza Bambas (:mayhemer)]

And of course, I re-tested the STR + few more clicks here and there to check this patch works.

Comment 39

[:aceman]

Comment on attachment 8954067 [details] [diff] [review]
1343536-alternative.patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5287,5 @@
>    },
>  
>    doContent: function(contentType, isContentPreferred, request, contentHandler)
>    {
> +    request.URI.query += "&type=message/rfc822";

How are we sure the type isn't already there?

Comment 40

[Jorg K (CEST = GMT+2)]

Looks like we're not 100% sure. It didn't work without the line :-)
In comment #15 we read: It finds "part=" in the query but doesn't find "type="

Comment 41

[:aceman]

Strangely, when I edit the original message as new, even before dragging the attachment out, it can't be opened. It tries to open a nsmail-*.tmp file which is considered plain text.
When just viewing the message (without editing), the eml attachment is displayed fine as a message.

This happens without the patch. The patch does not change anything for me in this regard. 
What is going on?

Comment 42

[Jorg K (CEST = GMT+2)]

"Edit as new" is not part of the STR :-(
The patch fixes the crash with STR in comment #0. You found another bug :-(

Comment 43

[:aceman]

Yeah, sorry to drag in Edit as new.
So without that step and dragging the eml from a message displayed in window now works.

Interestingly, the URI.query (inside doContent()) of that attachment will be:
number=3&part=1.2&filename=name.eml&type=message/rfc822

When using the file picker to attach an .eml file, the URI.query is:
&type=message/rfc822

I don't know why we need the original number and part when the message was dragged and why there is no filename when attaching a local file (when we best know the filename), but whatever. The opening of the attachment works now.
Maybe you could at least not add the leading '&' when the query is empty.

Comment 44

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #43)
> I don't know why we need the original number and part when the message was
> dragged ...
Then you're misunderstanding something.

When you drag a message into the bucket, you get a URL like
mailbox-message: ... or imap-message: ... (see comment #17).

In the case of the bug, we're dragging a message attachment. So the URL of that attachment is:
mailbox: ... ?number=xx&part=1.2?filename=xx.eml
Up to "number=xx" we're addressing the message from which we drag, with part=x.y we're addressing a part of that message. If you drag the first attachment it will be 1.2, for the second 1.3 and for more complicated MIME structures it could be 2.y.

> Maybe you could at least not add the leading '&' when the query is empty.
OK, I don't think that the query will ever be empty since we have ?number and ?part or for IMAP most likely only ?part. I'll play around with it tonight.

Comment 45

[:aceman]

But why do we need to maintain the info that the attachment came dragged from another message? Is that still only a link and not the real data until the msg is actually sent? That is possible, I think that also happens with the local files, that they are actually fully read on message send (so they must be in the filesystem until then).

The query is empty, e.g. in the case of attaching a local file, not dragging (comment 43). I have dumped the query when playing with various scenarios.

Comment 46

[Jorg K (CEST = GMT+2)]

Attachment: 1343536-alternative.patch (v2)

Thanks for testing this. You are quite right, attached message files come through this new code path too now.

So I have adding the type more specific, we'll only add it if we're accessing a message part.

Comment 47

[Jorg K (CEST = GMT+2)]

So I have *made* adding the type ...

Comment 48

[:aceman]

Comment on attachment 8955688 [details] [diff] [review]
1343536-alternative.patch (v2)

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

I do not understand why you do not need to add the type for local files. But it seems to work.
Before the patch, attached local .eml files couldn't be opened (the 'open with' dialog appeared claiming this is a HTML document).
With the patch, the eml opens automatically as a message in a new window. Also the dragged eml as in the report does work now.
Thanks.

(Eml attachment in a message in Outbox edited as new still doesn't work. For a new bug.)

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/253ed5106854
Open message/rfc822 attachments correctly from the attachment bucket. r=aceman

Comment 50

[Jorg K (CEST = GMT+2)]

TB 52.8 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/7ff2cfa93818e88db48e850148dc17475318a035