Closed
Bug 1466802
Opened 6 years ago
Closed 6 years ago
"Editing as new" of a message with .eml attachment or forwarding or attaching a message from .eml file it doesn't allow opening the .eml attachment from the attachment pane
Categories
(Thunderbird :: Message Compose Window, defect)
Thunderbird
Message Compose Window
Tracking
(thunderbird_esr60 fixed, thunderbird60 fixed, thunderbird61 wontfix, thunderbird62 fixed)
RESOLVED
FIXED
Thunderbird 62.0
People
(Reporter: jorgk-bmo, Unassigned)
Details
(Keywords: regression)
Attachments
(2 files, 1 obsolete file)
31.55 KB,
text/plain
|
Details | |
1.90 KB,
patch
|
aceman
:
review+
bzbarsky
:
feedback+
jorgk-bmo
:
approval-comm-beta+
jorgk-bmo
:
approval-comm-esr60+
|
Details | Diff | Splinter Review |
STR: Send message with attached message (.eml) Edit sent message as new or forward it. Double-click the message attachment. It doesn't open :-( Alice, can you find this regression for us. Nothing in the error console. Tested in TB 60 beta.
Flags: needinfo?(alice0775)
Reporter | ||
Comment 1•6 years ago
|
||
Other attachments, like PDF or images can be opened in the forwarded message. Also, the .eml attachment *is* forwarded, only that it can't be opened during message composition. Maybe caused by bug 1343536?
Comment 2•6 years ago
|
||
At least 3 regression when double click on attachement .eml in "Write: fwd:" window #1 Opened, but the content shows the following error: XML Parsing Error: undefined entity Location: jar:file:///L:/comm/2018/03/thunderbird%20comm-central%2004-Mar-2018%20175246/omni.ja!/chrome/toolkit/content/global/netError.xhtml Line Number 313, Column 37: <h1 id="et_blockedByPolicy">&blockedByPolicy.title;</h1> https://hg.mozilla.org/comm-central/pushloghtml?fromchange=20758564d6cb90003a142f83df23e2697ca16b45&tochange=0dc12c51c93ef013811d6b4daaf63fef3ce2fa3a https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8cced2a46f73238da13e41bcae8f6f801419bb7a&tochange=21f7f94a2c18dc8010ac2f300a36cc4ddd16081c #2 Opened, but the content shows "File not found" https://hg.mozilla.org/comm-central/pushloghtml?fromchange=0dc12c51c93ef013811d6b4daaf63fef3ce2fa3a&tochange=887ece68ebe55d06b7ff9421a907c04d09fe4bde https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=21f7f94a2c18dc8010ac2f300a36cc4ddd16081c&tochange=190b536928f8a8ca96e52101d2013c88a1a66384 #3 Nothing open https://hg.mozilla.org/comm-central/pushloghtml?fromchange=b41990561a61963c1e454567d83464f9197047b8&tochange=715910aab7dfd57ca8c65521fac3ee7f70bae0a0 https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0ef34a9ec4fbfccd03ee0cfb26b182c03e28133a&tochange=0ef34a9ec4fbfccd03ee0cfb26b182c03e28133a
Flags: needinfo?(alice0775)
Reporter | ||
Comment 3•6 years ago
|
||
Thanks Alice. All three ranges are at the beginning of March 2018. Yes, M-C tightened the security back then, and introduced that "blockedByPolicy" error, for which the string was first missing in C-C. On top of that there were a whole lot of nsIURI changes. I fixed something similar in bug 1374779 and in bug 1406574. But that was all in 2017. Hmm.
Reporter | ||
Comment 4•6 years ago
|
||
Boris, can you please help me here. When you forward a message that contains - say - a PDF and another message as attachments, these show up in the attachment pane/bucket and if you hover them you can see the URL: For a PDF I see this under Windows: C:///Users/[snip]/Temp/nsmail-4.pdf For the message attachment I see: C:///Users/[snip]/Temp/nsmail-5.eml Double-clicking the PDF link opens the PDF. Double-clicking the .eml does nothing. It doesn't seem to be related to the security tightening in M-C that caused bug 1374779 and bug 1406574. With you much better understanding of the overall architecture, could you please give me a tip on where to start looking. Actually, this never really worked well. I've downloaded a TB from March 1st (and also tried TB 52) and when double-clicking the message attachment in the bucket, it launches the dialog "Which program do you want to use to open this". It doesn't automatically open it like if the bucket contained a mailnews URL, so a reference to a message stored in TB. If you follow the "Which program ..." prompts and choose to open the message in TB, it doesn't work since TB is already running. So even if we restored the previous behaviour, it wouldn't be much use :-(
Flags: needinfo?(bzbarsky)
Comment 5•6 years ago
|
||
OK, so... When you double-click it, does Thunderbird basically kick off a docshell load? That would be the first thing to check. The old behavior sounds like we did a docshell load of the file:// URL, got back a MIME type we can't handle, etc. But that should all happen identically for .pdf and .eml... So I guess first things to check: 1) When you double-click, does nsURILoader::OpenChannel (either signature) get called? 2) If it does, then what do we end up deciding to do once we reach nsDocumentOpenInfo::DispatchContent for that load?
Flags: needinfo?(bzbarsky)
Reporter | ||
Comment 6•6 years ago
|
||
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #5) > 1) When you double-click, does nsURILoader::OpenChannel (either signature) > get called? Yes. From nsURILoader::OpenURI from JS with JS stack: 0 OpenSelectedAttachment([object XULCommandEvent]) ["chrome://messenger/content/messengercompose/MsgComposeCommands.js":5533] this = [object XULElement] 1 onxblclick(event = [object MouseEvent]) ["chrome://messenger/content/mailWidgets.xml":483] this = [object XULElement] Code is in OpenSelectedAttachment() and reads: let uriLoader = Cc["@mozilla.org/uriloader;1"].getService(Ci.nsIURILoader); uriLoader.openURI(channel, true, new nsAttachmentOpener()); I guess the uri is that file URI and we use a system principal when constructing the channel: https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mail/components/compose/content/MsgComposeCommands.js#5497 (line numbers don't match, DXR seems out of date). > 2) If it does, then what do we end up deciding to do once we reach > nsDocumentOpenInfo::DispatchContent for that load? Yes, we get there with this stack: xul.dll!nsDocumentOpenInfo::DispatchContent(nsIRequest * request, nsISupports * aCtxt) Line 390 C++ xul.dll!nsDocumentOpenInfo::OnStartRequest(nsIRequest * request, nsISupports * aCtxt) Line 315 C++ xul.dll!nsBaseChannel::OnStartRequest(nsIRequest * request, nsISupports * ctxt) Line 859 C++ xul.dll!nsInputStreamPump::OnStateStart() Line 521 C++ xul.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream * stream) Line 440 C++ xul.dll!nsInputStreamReadyEvent::Run() Line 104 C++ xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 1091 C++ Stepping through this I see mContentType="message/rfc822". forceExternalHandling=false. Finally it exits via: LOG((" Success! Our default listener likes this type")); // All done here return NS_OK; Sadly nothing happens at all. It would have been great for TB just to open the message without doing that dialogue to pick an external program. What other debugging should I do? And should I keep setting NI?bz, or you're following this anyway?
Flags: needinfo?(bzbarsky)
Comment 7•6 years ago
|
||
> LOG((" Success! Our default listener likes this type"));
Great, so we're handing off the load to the listener. Then what happens?
Under nsDocumentOpenInfo::TryContentListener did we call IsPreferred or CanHandleContent?
What is the concrete type of aListener there?
Did we end up on the ConvertData codepath?
(Note that you could also do a log with URILoader:5 as the module and attach it here...)
Needinfo is probably best.
Flags: needinfo?(bzbarsky)
Reporter | ||
Comment 8•6 years ago
|
||
Hmm, that logging doesn't answer your questions, these are the last lines: [10116:Main Thread]: D/URILoader nsURILoader::OpenURI for file:///C:/Users/jorgk/AppData/Local/Temp/nsmail-11.eml [10116:Main Thread]: D/URILoader nsURILoader::OpenChannel for file:///C:/Users/jorgk/AppData/Local/Temp/nsmail-11.eml [10116:Main Thread]: D/URILoader [0x00000216A8438C40] nsDocumentOpenInfo::Prepare [10116:Main Thread]: D/URILoader [0x00000216A8438C40] nsDocumentOpenInfo::OnStartRequest [10116:Main Thread]: D/URILoader [0x00000216A8438C40] nsDocumentOpenInfo::DispatchContent for type '' [10116:Main Thread]: D/URILoader Got type from channel: 'message/rfc822' [10116:Main Thread]: D/URILoader forceExternalHandling: no [10116:Main Thread]: D/URILoader [0x00000216A8438C40] nsDocumentOpenInfo::TryContentListener; mFlags = 0x1 [10116:Main Thread]: D/URILoader Listener has aborted the load [10116:Main Thread]: D/URILoader Success! Our default listener likes this type [10116:Main Thread]: D/URILoader After dispatch, m_targetStreamListener: 0x0000000000000000, rv: 0x00000000 [10116:Main Thread]: D/URILoader OnStartRequest returning: 0x00000000 [10116:Main Thread]: D/URILoader [0x00000216A8438C40] nsDocumentOpenInfo::OnStopRequest However, I noticed that in the debug console: [10116, Main Thread] WARNING: Trying to handle content that's not a nsIMsgMailNewsUrl?: file c:/mozilla-source/comm-central/comm/mailnews/base/src/nsMessengerContentHandler.cpp, line 64 That's certainly something I implemented when following M-C's nsIURI changes back in March 2018: https://hg.mozilla.org/comm-central/rev/8e0def3875f3#l2.55 Looks like we need to handle files there as well, or just try to open anything. Patch will come.
Reporter | ||
Comment 9•6 years ago
|
||
That tries to restore the previous behaviour, but doesn't. Now I get: File not found The file mailbox:///C:/Users/jorgk/AppData/Local/Temp/nsmail-13.eml&number=0 cannot be found. Please check the location and try again. as Alice already indicated in comment #2. Let me try a crude fix to replace mailbox with file and strip the query.
Reporter | ||
Comment 10•6 years ago
|
||
Hmm, I debugged it a little further. The URL in question is: {mData=0x0000021d3b58ba0c "file:///C:/Users/jorgk/AppData/Local/Temp/nsmail-15.eml" mLength=55 mDataFlags=...} However, the error is: The file mailbox:///C:/Users/jorgk/AppData/Local/Temp/nsmail-13.eml&number=0 cannot be found. Looks like nsMessengerContentHandler::OpenWindow() doesn't do the right thing, Boris? https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mailnews/base/src/nsMessengerContentHandler.cpp#74 Strangely before that launched something else triggering the "Which program do you want to use to open this" prompt for external stuff. Should we check the URL for a scheme of "file" and then try to open the file? How?
Flags: needinfo?(bzbarsky)
Comment 11•6 years ago
|
||
> Hmm, that logging doesn't answer your questions
It sort of does part of it:
[10116:Main Thread]: D/URILoader Listener has aborted the load
That means we called DoContent and it responded with "stop now". Doesn't tell you what the listener is, of course.
The content handler thing is only reached if listeners all say "no thank you", which is not what that log is showing. This is why I asked you to attach (not paste) the whole log.
Anyway, I have no idea what nsMessengerContentHandler.cpp is trying to do and why. But for your next step, you probably want to find the nsIURIContentListener that aborted the load.
Flags: needinfo?(bzbarsky)
Reporter | ||
Comment 12•6 years ago
|
||
Here's the full log. (In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #11) > The content handler thing is only reached if listeners all say "no thank > you", which is not what that log is showing. This is why I asked you to > attach (not paste) the whole log. I believe I pasted the relevant lines, I pasted all the lines related to the message/rfc822 attachment. The log ends after [10116:Main Thread]: D/URILoader [0x00000216A8438C40] nsDocumentOpenInfo::OnStopRequest (last line pasted). I think our message content handler is geared towards messages and it tries to open/show messages using a message window ("chrome://messenger/content/messageWindow.xul") here: https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mailnews/base/src/nsMessengerContentHandler.cpp#83 I have little idea about how it all hangs together, but I could imagine that we want to force our own handler to be used. I guess the nsIURIContentListener would be here: https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mail/components/compose/content/MsgComposeCommands.js#5511 which is the attachment opener. Especially canHandleContent() returns false. That code has also changed at the beginning of March, see https://hg.mozilla.org/comm-central/rev/253ed5106854 but not the canHandleContent(). I've reverted that change so doContent() and isPreferred() unconditionally return false again and that restores the previous useless behaviour of trying to launch an external program for the .eml. I'd be interested to understand why calling return wwatch->OpenWindow(0, "chrome://messenger/content/messageWindow.xul", "_blank", "all,chrome,dialog=no,status,toolbar", aURI, getter_AddRefs(newWindow)); in nsMessengerContentHandler::OpenWindow() with file:///C:/Users/jorgk/AppData/Local/Temp/nsmail-15.eml, we get: The file mailbox:///C:/Users/jorgk/AppData/Local/Temp/nsmail-13.eml&number=0 cannot be found. So perhaps you can comment on https://hg.mozilla.org/comm-central/rev/253ed5106854 which was approved by Honza back then and show me a way to implement opening the .eml in a useful way.
Flags: needinfo?(bzbarsky)
Reporter | ||
Comment 13•6 years ago
|
||
This does the trick. And much better than ever before since we're now opening this in its own stand-alone window.
Attachment #8983606 -
Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Attachment #8983735 -
Flags: review?(acelists)
Attachment #8983735 -
Flags: feedback?(bzbarsky)
Reporter | ||
Comment 14•6 years ago
|
||
Aceman, there is a much easier test case: Create a new message, drag an .eml file into the attachment bucked, try to open it from the bucket.
Summary: "Editing as new" of a message with .eml attachment or forwarding it doesn't allow opening the .eml attachment from the attachment pane → "Editing as new" of a message with .eml attachment or forwarding or attaching a message from .eml file it doesn't allow opening the .eml attachment from the attachment pane
Comment 15•6 years ago
|
||
Comment on attachment 8983735 [details] [diff] [review] 1466802-fix-HandleContent.patch (v2) So we're still canceling the original load, then doing this window.open thing, right? Seems ok if that' the behavior you want.
Attachment #8983735 -
Flags: feedback?(bzbarsky) → feedback+
Comment 16•6 years ago
|
||
Comment on attachment 8983735 [details] [diff] [review] 1466802-fix-HandleContent.patch (v2) Review of attachment 8983735 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this works for me. Compiling with the change produces a warning with clang: mailnews/base/src/nsMessengerContentHandler.cpp:72:13: warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result] NS_MutateURI(aUri) Can you fix it before it aborts the build in the future?
Attachment #8983735 -
Flags: review?(acelists) → review+
Comment 17•6 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/7a2b8cfdb36e Also open non-mailnews URLs in nsMessengerContentHandler::HandleContent. r=aceman
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 18•6 years ago
|
||
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #15) > So we're still canceling the original load, then doing this window.open > thing, right? Yes, not my idea originally, but working. (In reply to :aceman from comment #16) > Can you fix it before it aborts the build in the future? Done.
Target Milestone: --- → Thunderbird 62.0
Reporter | ||
Updated•6 years ago
|
Attachment #8983735 -
Flags: approval-comm-esr60+
Attachment #8983735 -
Flags: approval-comm-beta+
Reporter | ||
Comment 19•6 years ago
|
||
TB 60 beta 7 (BETA_60_CONTINUATION branch): https://hg.mozilla.org/releases/comm-beta/rev/a3ff69cb7060c197fa2d301828884c6ea88aaf42
status-thunderbird60:
--- → fixed
status-thunderbird61:
--- → affected
status-thunderbird62:
--- → fixed
status-thunderbird_esr60:
--- → affected
Reporter | ||
Comment 20•6 years ago
|
||
https://hg.mozilla.org/releases/comm-esr60/rev/863da7772a94
You need to log in
before you can comment on or make changes to this bug.
Description
•