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)

defect
Not set
normal

Tracking

(thunderbird_esr60 fixed, thunderbird60 fixed, thunderbird61 wontfix, thunderbird62 fixed)

RESOLVED FIXED
Thunderbird 62.0
Tracking Status
thunderbird_esr60 --- fixed
thunderbird60 --- fixed
thunderbird61 --- wontfix
thunderbird62 --- fixed

People

(Reporter: jorgk-bmo, Unassigned)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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)
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?
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.
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)
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)
(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)
>      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)
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.
Attached patch 1466802-fix-HandleContent.patch (obsolete) — Splinter Review
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.
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)
> 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)
Attached file tb-url-load.txt
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)
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)
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 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 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+
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
(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
Attachment #8983735 - Flags: approval-comm-esr60+
Attachment #8983735 - Flags: approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: