User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-GB; rv:1.9.0.1) Gecko/2008070206 Firefox/3.0.1 Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20080830025944 Shredder/3.0b1pre When replying to a message with an embedded attachment, if the original is deleted before sending the reply it hangs with "Attaching..." Reproducible: Always Steps to Reproduce: 1.Go to inbox 2.Open a message with an embedded image 3.Reply to it 4.Go back to the inbox 5.Delete the message being replied to 6.Go to compose window and send it Actual Results: It sits there with an "Attaching" dialog - it looks like it is trying to find the attachment, which has been deleted. Expected Results: Not totally sure - an error would be acceptable but not what the user expects, especially since they often don't realise they are working with an attachment (I hit this with an embedded graphic in a signature and it took me a while to realise why there was an error) This may seem obscure, but I think its not uncommon message to start a reply to a message, then for some reason not completing it while continuing to work on the inbox - including deleting the message being replied to.

I could swear this is a duplicate but I'm not finding it. Perhaps it mentions something other than reply in the summary

Actually, I'm unable to find any exact duplicate either for this. The problem is more general than just related to mailbox attachments though. The encoding of an attachment is performed when either sending the message or saving it as a draft. Thus, for the time between attaching anything (any of file:, http:, or mailbox:/imap: URIs) and when it is actually used, mail/news encounters a problem if that reference is no longer valid. Since the mailbox: reference is effectively a byte count, it may be less easy to detect such a change, resulting in the effect seen (rather than an error message that the attachment does not exist, e.g., for a file).

Most probable dup Bug 216776 - Attach a file which then moves: confusing error message (which should change summary to a more accurate) this describes the infinite attaching process/alert like here .. or variation Bug 391190 - message "forward as attachment" from pop or local folder loses attachment if original email is moved/deleted before message is sent (well, fwd instead of re ..) Related: Bug 314732 - should not be able to delete a file attached to an open composition window -another way to put it [changin to all os ..]

Ok this probably should go into a bigger bucket: Composition and Editing might be a good category for a meta bug. Attaching....is a common lament and can be caused from a large variety of causes. Mostly because the editor is pretty much unowned and unsupported.

Doing some testing for bug 458899 gave me some further clues when the stalling case occurs and when an error is thrown. I could reproduce this on both Windows and Linux with current trunk builds. If an image is inserted from another message (either resulting from a reply or by explicit copy-pasting from another message), it receives a "mailbox:" or "imap:" path based on its origin (reproduced for both IMAP and local messages). The image is attached when sending and has to be available then. If the path was invalidated some time between attaching and sending (or saving as draft), bug 216776 applies and an error message "Sending of message failed. There was an error attaching. Please check if you have access to the file." should be presented. This is what I get when deleting the message *and* compacting the folder immediately after that and before clicking on Send. However, if the message is deleted but the folder *not* compacted, the message is still there and the mailbox/imap link still "kind of" valid, in this case though the "Attaching..." issue is seen. > (comment #4) this probably should go into a bigger bucket: > Composition and Editing might be a good category for a meta bug. I agree that there are sufficient different manifestations of this issue in various bugs, so that would certainly warrant a meta bug for tracking those. Since this here is a very specific case, I'm confirming the bug based on the testing results. It may well be that it will be resolved by a fix elsewhere. Most of the related bugs result from the fact that only a reference is kept when something is attached or included into the message and then has to be revisited when the message is actually sent. Making a snapshot of the content of an attachment (as proposed in bug 378046) would help to avoid such issues. [Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090326 Shredder/3.0b3pre, Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20090223 Thunderbird/3.0b2]

somewhat like bug 117698

bug 509128 may be related

I don't think #532395 depends on this. This one may be labelled as a bug, but it takes some work to trigger it and even then, it seems logical to me that if a referenced object no exists, error message pops up (or even a hang if the non-existence is not properly detected). Bug #532395 is different! EVERY single email I reply to (no auto-save, no deleting in between, no drafts horseplay, no uncompacted folder, etc) hangs unless I removed all images from the corporate signature. Just a simple, immediate, key combination CTRL-R + CTRL-ENTER immediately hangs indefinitely on "Attaching..." #532395 is an HTML composition/multipart-mishandling issue standing completely on its own.

(In reply to David Kubicek from comment #8) > Bug #532395 is different! Have you read thru bug 532395 comment #42? Bug 532395 comment #18 is same case as this bug. (Original of reply is deleted) If reply_to/forward_of a mail with embed image(i.e HTML mail) in local mail folder(call offset=XXX, embed image part number=M.N), and if offset of the replied/forwarded mail in local mail folder file is changed by Compact, one of following may occur. (A) There is no valud mail at Offset=XXX after Compact. Because no mail is found at Offset=XXX, it's same as "delete of original". So, this bug(and bug 532395 comment #18) occurs. (B) Valid mail is placed at at Offset=XXX after Compact. (B-1) part number=M.N exists in newly placed mail at Offset=XXX. data of part number=M.N of the mail is used. So wrong data of other mail is used. (phenomenon of bug 766495 and bug 799450) (B-2) part number=M.N doesn't exist in newly placed mail at Offset=XXX. Null data is used as data of part number=M.N, or attacching failure occurs. Same thing can occur on previous version on draft mail by Compact if local Drafts folder. Many comments in bug 532395 sounds local Drafts case except bug 532395 comment #18. They are same as (A) or same as (A) on local Drafts folder, i.e. same as this bug. As I wrote in bug 532395 comment #62, root cause of problems in many reports is this bug - Tb can't handle "missing replied/forwarded/previous-draft mail" case if embed image of HTML mail is used. > Bug #532395 is different! EVERY single email I reply to > (no auto-save, no deleting in between, no drafts horseplay, no uncompacted folder, etc) > hangs unless I removed all images from the corporate signature. >(snip) > #532395 is an HTML composition/multipart-mishandling issue standing completely on its own. "embed image" is HTML only(multipart/related) attribute, isn't it? "signature with image" is an "embed image" and is used in HTML mail only, isn't it?

Removing meta bug 498274 from Blocks: field, because, as David Kubicek says, Compact is absolutely irrelevant to this bug's problem and "Steps to reproduce".

Changing bug summary to original report in comment #0 for ease of bug reading.

FTR: This bug is part of a larger series that are all caused by the same wrong design: When attaching things (files, images, remote stuff etc.), instead of grabbing a snapshot copy of those things immediately, TB currently just keeps a reference and then tries to get those things later at some unpredictable time when saving & closing draft or sending. Worse, it depends on a number of factors including the *method* of attaching (Mapi vs. non-Mapi, see bug Bug 378046 comment 60) and user's behaviour of saving/closing drafts when exactly those external things are copied into TB-owned temp folders and/or mime-encoded into the message. Notwithstanding the need for figuring out details, all of these bugs have the same solution: For all things attached, TB should grab a snapshot copy asap, and probably mime-embed them asap, too.

(In reply to Thomas D. from comment #12) > FTR: This bug is part of a larger series that are all caused by the same > wrong design: When attaching things (files, images, remote stuff etc.), > instead of grabbing a snapshot copy of those things immediately, TB > currently just keeps a reference... I'm glad to see that I'm not the first to arrive at that conclusion: (In reply to rsx11m from comment #2) > The problem is more general than just related to mailbox attachments though. (In reply to rsx11m from comment #5) > Most of the related bugs result from the fact that only a reference is kept > when something is attached or included into the message and then has to be > revisited when the message is actually sent. Making a snapshot of the content > of an attachment (as proposed in bug 378046) would help to avoid such issues.

Actually, this bug happens even if the original message has been moved to another folder. How to reproduce it, on Windows: 1. Take a message with embedded images, click on reply. 2. Come back on the main window, move the message in another folder. 3. Go back on the compose window, write the answer, click on send. 4. Message is not sent "Attaching" is displayed forever, and the message is not sent.

(In reply to Andre Rodier from comment #14) > Actually, this bug happens even if the original message has been moved to > another folder. > > How to reproduce it, on Windows: Andre, thanks for pointing that out and providing STR, which is very helpful. We have plenty of bugs in this corner, all of them symptoms of a bigger problem of wrong design, as described in my meta bug 877159 aka "attach-paradigm-fail" where I focussed on this problem and bundled all the issues. I think at least by now there's some agreement on that description of the problem and the desired direction of fixing... However, it's not clear how much work it would be to fix them, and it's certainly non-trivial and requiring a lot of code knowledge and sophistication going right into the very internals of TB. Since TB is now maintained by volunteers, we'll need to find someone who is skilled, willing, and having enough time to start fixing this. Not easy (sigh)... Or perhaps there are still design choices to be made, like either embedding things like images as data-URI, or linking to a temporary snapshot file which is owned by TB, or linking to mime parts of the draft... Add IMAP and saved drafts into the picture, it's a tricky thing...

(In reply to Thomas D. from comment #12) > FTR: This bug is part of a larger series that are all caused by the same > wrong design: When attaching things (files, images, remote stuff etc.), > instead of grabbing a snapshot copy of those things immediately, TB > currently just keeps a reference and then tries to get those things later at > some unpredictable time when saving & closing draft or sending. Worse, it > depends on a number of factors including the *method* of attaching (Mapi vs. > non-Mapi, see bug Bug 378046 comment 60) and user's behaviour of > saving/closing drafts when exactly those external things are copied into > TB-owned temp folders and/or mime-encoded into the message. > > Notwithstanding the need for figuring out details, all of these bugs have > the same solution: For all things attached, TB should grab a snapshot copy > asap, and probably mime-embed them asap, too. Thomas, can you answer this question... Would I be able to tell my users a workaround of: When you initially click 'Reply' or 'Forward' on a message, *immediately* click the 'Save' button to save the draft. Would this 'grab a snapshot' of the referenced items/files, or would it just save the references? Thanks

(In reply to Charles from comment #16) > Would I be able to tell my users a workaround of: > > When you initially click 'Reply' or 'Forward' on a message, *immediately* > click the 'Save' button to save the draft. If Drafts folder is local mail folder(pop3/Local Folders), and if auto-compact is invoked while composing a mail or editing draft mail, Bug 817245 can happen. Proper workaround of this bug : Never delete/move mail to which you are replying to, or for which you are trying to do forward.

I've been seeing this for a very long time, and I'm not moving the original message before sending the reply. I'm seeing this when replying to messages I did not originate, where the sender may have used an image in their signature or in the message body. I have to remove them before I can reply. I am also seeing this on messages I did originate (M1) with images in the body, send to someone who replied (M2), and I try to reply back (M3). I may have moved M1 to another folder, but not M2 before the M3 reply. I have to remove the images from M3 or reinsert them before I can send it.

I think it's time we do something about this bug. Here a short analysis: When replying to a message with an embedded image, the composition contains this: <img src="mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/AAA%20Test%20message%201?number=2&amp;header=quotebody&amp;part=1.2&amp;filename=lijbmghmkilicioj.png" alt=""> In other words, the image is references via a mailbox:// URL. When the referenced message gets moved before the send, we get a problem. nsMsgAttachmentHandler::SnarfAttachment() tries to access that URL via FireURLRequest() with callback FetcherURLDoneCallback(). That "sort of" fails since the callback is never called. However, FireURLRequest() returns OK, so SnarfAttachment() returns OK. (When manually setting the return code to "failure", an "error attaching" is issued and sending fails as it should.) If I compact the mailbox where the referenced message was stored before, I immediately get an error since NS_ERROR_ILLEGAL_VALUE is returned from the FireURLRequest() call. In the compaction case, it fails on protocol->Initialize(aURI): nsMailboxService::NewChannel2() Line 609 C++ mozilla::net::nsIOService::NewChannelFromURIWithProxyFlagsInternal() Line 797 C++ mozilla::net::nsIOService::NewChannelFromURIWithProxyFlags2() Line 888 C++ mozilla::net::nsIOService::NewChannelFromURI2() Line 674 C++ NS_NewChannelInternal() Line 171 C++ NS_NewChannel() Line 262 C++ nsURLFetcher::FireURLRequest() Line 325 C++ nsMsgAttachmentHandler::SnarfAttachment() Line 786 C++ So the problem seems to be that the referenced message was moved and hence marked deleted in the mailbox, but the call accessing it via its URL still succeeds. In the "move but don't compact" case NS_NewChannel() succeeds and we get into OpenURI(). I traced it down a little: nsMsgProtocol::LoadUrl() Line 472 C++ nsMailboxProtocol::LoadUrl() Line 537 C++ nsMsgProtocol::AsyncOpen() Line 544 C++ nsURILoader::OpenURI() Line 821 C++ nsURLFetcher::FireURLRequest() Line 334 C++ nsMsgAttachmentHandler::SnarfAttachment() Line 786 C++ So somehow we need to deal with deleted but not purged message better. If we could always display an error in this case, then all the other bugs in this area (bug 1251853, bug 532395) could be brought a little closer to resolution. Kent, what's your comment?

Ideally, an embedded attachment would be subject to a snapshot as the editor does already with pasted images or those dragged and dropped from a file, then use a "data:" rather than "mailbox:" URI; but at least giving a proper error message in that case would certainly help (though it may be difficult to explain to the user how to actually resolve the issue, especially with the image actually displayed).

Thank you for working on this issue However, please note that it can occur even when the message being replied to was not moved or compacted, but remains in the Inbox.

(In reply to rsx11m from comment #23) > Ideally, an embedded attachment would be subject to a snapshot as the editor > does already with pasted images or those dragged and dropped from a file, > then use a "data:" rather than "mailbox:" URI; Magnus is working on using more data URIs in another bug. However, that doesn't change the way of referencing into existing messages and their images. > but at least giving a proper > error message in that case would certainly help (though it may be difficult > to explain to the user how to actually resolve the issue, especially with > the image actually displayed). What is displayed is a reference. If the referenced object goes away, for whatever reason, there should be an error and not an endless loop. Showing the error will alert the user to the fact that something happened that they should be aware of. (In reply to marty from comment #24) > Thank you for working on this issue > However, please note that it can occur even when the message being replied > to was not moved or compacted, but remains in the Inbox. That's what the "voodoo" bug 532395 is about where no one so far could deliver a reproducible case. The idea is to get this bug here fixed with a proper error message so we can diagnose the other bug better.

(In reply to rsx11m from comment #23) I agree mailbox: references aren't really going to be stable, and it would be preferable to just use a data: uri instead. That way what the user sees will be guaranteed to work, and be guaranteed to be what he intended. (We have that other bug that the referenced image can under some unknown conditions be something unintended.) This is probably not a small piece of work of course. OTHO just fixing it to show an alert is better than status quo, but still a major annoyance to an end user.

(In reply to Magnus Melin from comment #26) > This is probably not a small piece of work of course. OTHO just fixing it to > show an alert is better than status quo, but still a major annoyance to an > end user. I am an "end user", too. I can't remember ever having seen this issue in my day to day usage. Can you? Sure, I can reproduce it deleting a message I'm replying to and it can be reproduced with signed messages, too (bug 1251853). We don't know which add-ons dig around in the message store and all the evidence we have in bug 532395 is of "voodoo" quality: Wait 30 minutes, turn around twice, then it fails. Hopefully with the instructions in bug 532395 comment #200 we will get better cases. If it were such a big issue, someone should have already looked into it since the bug was first raised eight years ago.

OK, tracing the "purge case" a little further, it fails because as expected the header is not found in the DB: https://dxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgDatabase.cpp#1908 Call stack: nsMsgDatabase::GetMsgHdrForKey() Line 1908 C++ nsMailboxUrl::GetMsgHdrForKey() Line 193 C++ nsMailboxUrl::GetMessageHeader() Line 236 C++ nsMailboxProtocol::SetupMessageExtraction() Line 417 C++ nsMailboxProtocol::Initialize() Line 119 C++ nsMailboxService::NewChannel2() Line 609 C++ mozilla::net::nsIOService::NewChannelFromURIWithProxyFlagsInternal() Line 797 C++ mozilla::net::nsIOService::NewChannelFromURIWithProxyFlags2() Line 888 C++ mozilla::net::nsIOService::NewChannelFromURI2() Line 674 C++ NS_NewChannelInternal() Line 171 C++ NS_NewChannel() Line 262 C++ nsURLFetcher::FireURLRequest() Line 325 C++ nsMsgAttachmentHandler::SnarfAttachment() Line 786 C++ So I guess in the move/delete case where the header is still in the DB, we need to check somewhere that the message is not deleted. Maybe someone can suggest a good spot for that check. Kent?

With this patch, we don't get the hang, instead we get: Sending of message failed. There was an error attaching . Please check that you have access to the file. We note the space before the full stop. Obviously the error argument wasn't properly inserted here: errorAttachingFile=There was an error attaching %S. Please check that you have access to the file. With that fixed, there should be enough information for the user.

This also improves the message that the user sees. String handling is sub-optimal, I'm still fighting with the various string types after more than one year on the project.

Not pretty, but pretty informative ;-)

thanks for working on this. (In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #27) > ... > If it were such a big issue, someone should have already looked into it > since the bug was first raised eight years ago. True to a point, but ... things change. There has been a definite strong uptick in reports in support. Whether it's from some code change in TB or some external factor is (at least to me) unknown. A few examples of the type of issue (not necessarily all this bug report) in the first two screens of https://support.mozilla.org/en-US/search/advanced?q=attachment+crash&is_locked=-1&num_voted=0&num_votes=&asked_by=&answered_by=&q_tags=&product=thunderbird&created=2&created_date=01%2F01%2F2016&updated=0&updated_date=&sortby=0&a=1&w=2 (don't believe the hit count - SUMO lies)

Here another question for Kent: With this change, referencing deleted/moved messages reliably causes an error. However, trying to retrieve a MIME part that doesn't exist, see bug 1251853 comment #3, still doesn't trigger an error and still hangs in the "Attaching" state, albeit with a more informative message. The reference there is to <img src="mailbox:///D:/MAIL-THUNDERBIRD/jorgk.default/Mail/jorgk.jorgk.com/Drafts?number=160&amp;header=quotebody&amp;part=1.2&amp;filename=lijbmghmkilicioj.png" alt=""> but this part doesn't exist, it's now 1.1.2 in the message due to the digital signing. I have dug around in the debugger for hours but so far I haven't found the spot were it tries to read part "1.2" and fails. Perhaps someone knows. I'd like to improve my patch here to also catch this problem.

Fixed string processing.

I looked at this for about 15 minutes, as I cannot afford right now the many hours it would take to dive into it myself. It looks to me like the fix is trying to address one particular case of the problem, rather than the real issues. The real issues, as with much of our code, is the poorly documented and managed error handling. Generally, we should be following rules such as this: When a call initiates an async method that is expecting a callback, if that call returns NS_OK then we should guarantee that one and only one callback occurs. (If the initial call fails, then there should be no callback expected or given.) So we need to clearly understand when a method is responsible for giving a callback. If it is, then no NS_ENSURE_SUCCESS returns to an upstream that knows nothing about the responsibility to issue a callback. You need to capture failures, and give the error equivalent of the callback. So for example, URLS terminate with nsIUrlListener::OnStopRunningUrl(url, aExitCode). When a method is responsible for the callback, it cannot return NS_ERROR_FAILURE but must issue the callback with aExitCode == some failure code. Ideally, a single method should be responsible for the callback rather than scatter that responsibility all over creation. An example of violating code is nsMsgUtils GetOrCreateFolder. Lots of OnStopRunningUrl, lots of NS_ENSURE_SUCCESS. Back to the problem at hand, what I would expect to see is a fix to code that violates these assumptions somewhere, or fails to correctly respond to an error return. Bonus points for actually documenting the assumptions. This is not a definitive answer, just some feedback and comments as requested.

Comment on attachment 8760379 [details] [diff] [review] Checking message validity (v3). On this particular code, this is an attempt to solve the problem rather than proper managing of the error response. The code here may be useful, and might be part of the final solution. But really when there is a poor response to an error condition "TB hangs with "Attaching..." the first thing to do is to get the error response correct, as you now have a test case of failed error handling. Only then should you try to fix the underlying issue, which then eliminates your test case.

Sorry, someone else who understands the asynchronous processing needs to look into it. I couldn't work out where the asynchronous processing happens. All I can see that the message cannot be assembled correctly in SnarfAttachment() which is called synchronously as part of nsMsgCompose::SendMsg(): nsMsgAttachmentHandler::SnarfAttachment() Line 707 C++ nsMsgComposeAndSend::HackAttachments() Line 2538 C++ nsMsgComposeAndSend::Init() Line 3150 C++ nsMsgComposeAndSend::CreateAndSendMessage() Line 4037 C++ nsMsgCompose::SendMsgToServer() Line 1237 C++ nsMsgCompose::SendMsg() Line 1437 C++ Somehow the async part is done with nsIWebProgress.

Magnus, perhaps you can give some advice here. My patch catches the error early during the synchronous part and gives the user some information. Kent wants the asynchronous part fixed, but I wouldn't know how. The "Attaching %S..." message is put up by the synchronous part here: https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgSend.cpp#2515 That's in fact were I made some changes to show a decent message. As far as I understand it, the synchronous part assembles the message with all its attachments ready for sending and then returns. I wasn't able to debug the asynchronous part.

Comment on attachment 8760379 [details] [diff] [review] Checking message validity (v3). Review of attachment 8760379 [details] [diff] [review]: ----------------------------------------------------------------- I think we can agree it's not very pretty, but it's not like that's something new in this code ;) Since it's not likely to get sorted out another way (before the js rewrite is complete), we might as well take it IMHO. ::: mailnews/compose/src/nsMsgSend.cpp @@ +2539,5 @@ > { > nsString errorMsg; > nsAutoString attachmentFileName; > nsresult rv = ConvertToUnicode(nsMsgI18NFileSystemCharset(), m_attachments[i]->m_realName, attachmentFileName); > + if (attachmentFileName.Length() == 0) { .IsEmpty()

(In reply to Magnus Melin from comment #39) > Since it's not likely to get sorted out another way (before the js rewrite > is complete), we might as well take it IMHO. Yes. And since it doesn't cover all cases, as discussed in comment #33, we still get the "Attaching ..." hang, but with a better message. So there is still ample opportunity to fix what Kent wants fixed. If someone could explain to me where the asynchronous processing happens, I can look into that, too.

Since there are changes to nsMailboxUrl::GetMsgHdrForKey(), we're on the safe side with a try run, not that I expect anyone relying on being able to reach into dead messages: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=42eb9c305c7b

So here is my solution for the async error handling issue, that works for the test case of deleting a message with an embedded attachment prior to sending a reply. These 3 lines of changes are 14 hours of work. I'll comment on the issues in a feedback. One of the changes is risky.

Comment on attachment 8761088 [details] [diff] [review] Fix for the error handling Review of attachment 8761088 [details] [diff] [review]: ----------------------------------------------------------------- With all of these changes, I'm actually getting a dialog warning of the failed attachment download, and asking if I want to end anyway. This patch BTW is done on esr45 but I doubt if that matters. ::: mailnews/compose/src/nsMsgAttachmentHandler.cpp @@ +1132,5 @@ > > + // If the attachment is empty, let's call that a failure. > + if (!m_size) > + status = NS_ERROR_FAILURE; > + The IMAP response just sends an OK right after you request the non-existent body part, and the imap protocol does not complain. That seems odd, I wonder if that is the standard? But we can detect zero bytes to call it an error. ::: mailnews/imap/src/nsImapProtocol.cpp @@ +9294,1 @@ > } This is the risky change. We are adding a new listener in a very common path. I don't really understand how it worked at all before, but obviously it did so I need to understand that more to understand the ramifications of this. The whole attachment callback stuff was embedded in the m_channelListener. The way the protocol is written, when you actually have to talk to the server, it actually starts a whole new protocol to do that work. That protocol was not picking up the listeners from attachment snarfing. ::: mailnews/mime/src/nsStreamConverter.cpp @@ +999,5 @@ > nsresult > nsStreamConverter::OnStopRequest(nsIRequest *request, nsISupports *ctxt, nsresult status) > { > + // Make sure we fire any pending OnStartRequest before we do OnStop. > + FirePendingStartRequest(); The stream converter was delaying calling OnStartRequest because it wanted to figure out the content type prior to doing that. Unfortunately, with no data ever returned, neither was a content type. Therefore OnStartRequest never got fired for nsDocumentOpenInfo, and that call was necessary for that method to decide that the attachment listeners were indeed the listeners it wanted to use.

(In reply to Kent James (:rkent) from comment #43) > These 3 lines of changes are 14 hours of work. I'll comment on the issues in > a feedback. One of the changes is risky. I know the feeling. I spent seven months (not straight work) on a couple of lines on the disappearing .msf issue :-( So both patches go together? One hunk is for IMAP. I've only tested this for local folders. I'll do an IMAP test now.

With Kent's patch only, the POP test case gives: There was a problem including the file mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local Folders/AAA Test message 2?number=8&header=quotebody&part=1.2&filename=lijbmghmkilicioj.png in the message. Would you like to continue sending the message without this file? IMAP gives: There was a problem including the file imap://info@hostalpancheta.es@mail.hostalpancheta.es:993/fetch>UID>.INBOX.SPAM>58?header=quotebody&part=1.2&filename=lijbmghmkilicioj.png in the message. Would you like to continue sending the message without this file? Test case saving a digitally signed draft (test case from bug 1251853 comment #3) gives: There was a problem including the file mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local Folders/Drafts?number=22&header=quotebody&part=1.2&filename=lijbmghmkilicioj.png in the message. Would you like to continue saving the message without this file? So Kent's patch addresses all the reproducible issues I've come across. Great! Do we still want hunks from my patch? Providing a decent error message instead of an empty one certainly doesn't hurt although this is now not triggered any more. And the change in nsMailboxUrl::GetMsgHdrForKey() of stopping the access to a dead message may also be beneficial? Opinions?

Actually, my two hunks in nsMsgSend.cpp correct the "Attaching %S..." message, so we should take those. The hunk in nsMailboxUrl::GetMsgHdrForKey() seems to cause test failures: TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_localToImapFilterQuarantine.js but let's wait for the try to finish.

(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #32) > True to a point, but ... things change. There has been a definite strong > uptick in reports in support. Whether it's from some code change in TB or > some external factor is (at least to me) unknown. My first look would be whether the link being created as the reference always contains the KEY of the message. Or if it isn't the offset (WADA already speculates about offset in comment 9). OR if it is the key, whether we then fetch the right offset, not use the key as offset (if we need it). Those two no longer have the same value (for new messages), thanks the the last decoupling of them (which was done for TB45). But that is probably the other bug mentions here, where the image is not found even if original message was not moved.

Comment on attachment 8760892 [details] [diff] [review] Checking message validity (v3a). Review of attachment 8760892 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/local/src/nsMailboxUrl.cpp @@ +192,5 @@ > + // Did we get a db back? > + if (NS_SUCCEEDED(rv) && mailDB) { > + // GetMsgHdrForKey is not a test for whether the key exists, so check. > + bool hasKey = false; > + mailDB->ContainsKey(msgKey, &hasKey); Please use "(void)mailDB->ContainsKey(msgKey, &hasKey);" if you intentionally ignore return value. @@ +194,5 @@ > + // GetMsgHdrForKey is not a test for whether the key exists, so check. > + bool hasKey = false; > + mailDB->ContainsKey(msgKey, &hasKey); > + if (!hasKey) > + return NS_ERROR_NULL_POINTER; Wouldn't NS_ERROR_INVALID_ARG be semantically better?

(In reply to :aceman from comment #49) > Wouldn't NS_ERROR_INVALID_ARG be semantically better? Similar code uses NS_ERROR_NULL_POINTER on similar occasions. Anyway, this hunk will need to go since it causes test failures. New patch coming up.

Here's another try run with Kent's patch and mine: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=71f7cade1d21

Assigning to Kent since he kindly invested 14 hours of work to find the right solution ;-)

Sadly Kent's patch also upsets some tests: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=71f7cade1d21&selectedJob=19074 TEST-UNEXPECTED-FAIL | mailnews/compose/test/unit/test_attachment.js | xpcshell return code: 0 TEST-UNEXPECTED-FAIL | mailnews/compose/test/unit/test_attachment.js | testInput0 - [testInput0 : 83] -1 != -1 TEST-UNEXPECTED-FAIL | mailnews/compose/test/unit/test_messageHeaders.js | xpcshell return code: 0 TEST-UNEXPECTED-FAIL | mailnews/compose/test/unit/test_messageHeaders.js | testContentHeaders - [testContentHeaders : 77] false == true http://archive.mozilla.org/pub/thunderbird/try-builds/mozilla@jorgk.com-71f7cade1d21/try-comm-central-win32/try-comm-central_win7_ix_test-xpcshell-bm112-tests1-windows-build0.txt.gz Needs further looking into.

OK, the test failures are caused by this hunk: + // If the attachment is empty, let's call that a failure. + if (!m_size) + status = NS_ERROR_FAILURE; + if (NS_FAILED(status) && status != NS_ERROR_ABORT && NS_SUCCEEDED(mimeDeliveryStatus)) { // At this point, we should probably ask a question to the user // if we should continue without this attachment. Right there we get the lovely message/question as shown in comment #46. But an xpcshell test can't ask questions, so it crashes on ###!!! ASSERTION: attempted to open a new window with no WindowCreator: 'mWindowCreator', file c:/mozilla-source/comm-central/mozilla/embedding/components/windowwatcher/nsWindowWatcher.cpp, line 794" (at least in debug mode). So what is the way forward here? Change the tests to have non-empty attachments? Is an empty attachment really an error condition? A MIME part with just some headers and no content would be an empty attachment, right? That's not illegal. Is there not a better way to tell that an error occurred in fetching the part?

I tried the test case without that hunk: Result: The reply is sent but the image is missing. Nice that the "Attaching ..." hang has gone away, but what is sent is not what the user intended ;-(

Can you create an email that has as attachment that you think is valid, but is empty, and post it here? I can test this some more if I had an example.

I could work on the imap side if needed, but what I see there is the request BODYSTRUCTURE for what it wants, and the next IMAP reply is just OK. IMAP protocol code then returns at that point, and there is no content but also no error.

(In reply to Kent James (:rkent) from comment #58) > Can you create an email that has as attachment that you think is valid, but > is empty, and post it here? I can test this some more if I had an example. Well, at least the xpcshell tests seem to think that they are creating valid empty attachments that now trigger your condition. Anyway, I'll see whether I can create a manual example.

Here you go. I've just removed the attachment content. This comes back with m_size==0, but it shouldn't trigger an error.

I'm also not sure why you're mentioning IMAP since this also needs to work when replying to message in a local folder.

Thanks for the attachment. When testing with that, I could see no way to detect the difference between an empty attachment such as that one, and a missing attachment where the underlying source could not be found. This is a problem in propagating error conditions over a complex web of async interactions. Let's try to decide a way forward. My patch demonstrates the fundamental problem and a possible solution, which is that error handling only works when a listener is defined in nsDocumentOpenInfo, which happens in OnStartRequest(). But nsStreamConverter which is responsible for sending that delays the OnStartRequest, only sending it when data is received. No data, no OnStartRequest, hang. The change to StreamConverter solves that. Imap has a separate issue that the listener does not get attached, which a small change fixes (and I need to confirm that that is indeed needed, and has no side effects). Then we come down to what is really a new issue, exposed by the fix: that the attachment handler has no way of detecting a failed attachment. Maybe we could handle that as a separate bug, and land the changes to get rid of the hang? The effect of that is that the failures would not show, and the message would get sent without warning with an empty embedded attachment. My suggestion would be that we leave the error detection on empty attachment, and fix the tests to work with that. I think that is the lesser evil here, short of diving into the entire way attachment errors are propagated. There are other places (msgHdrViewOverlay.js open attachment) where an empty attachment puts up a dialog. The example that you gave is not one I could imagine makes any sense in a message (an empty image). Do we know of any places there is is not only possible, but reasonable, to have an attachment without content? But I am open to suggestions. After we land the fix for the hang we could have followup bugs (including perhaps Jorg's patch). Really though we should stop using message URLs as attachment references in compose.

Kent, thank you for the time spent on this. (In reply to Kent James (:rkent) from comment #63) > Thanks for the attachment. When testing with that, I could see no way to > detect the difference between an empty attachment such as that one, and a > missing attachment where the underlying source could not be found. This is a > problem in propagating error conditions over a complex web of async > interactions. I wish I understood more of the async stuff here. To my eye, there is the synchronous part of nsMsgCompose::SendMsg() which is ultimately called from here: https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2865 Does it not completely assemble the message to be sent as per my comment #37? Perhaps you can spend a bit more time to explain the sync/async interaction here, also useful as a knowledge transfer. I find it pretty amazing that the system can't tell the difference between an empty attachment and a missing attachment. Personally, I wouldn't support a solution where a message is not sent as last seen by the user, that is, without the image. If we really can't detect the failure to retrieve the attachment, then checking for empty is acceptable IMHO, since empty attachments don't make much sense.

My understanding of the sync processing (or lack thereof) only comes from hours spent in front of the debugger tracing what is happening, I cannot claim to understand it. I'm not sure it is understandable. This is the async equivalent of spaghetti code, impossible to reason about. If I try to explain my understanding, it will be vague and possibly wrong. Trying to document this is a major project I'm afraid. What would probably be useful is to try to understand why it is so hard to understand, and imagine how we could simplify it to make it easier. Maybe someday. Comment 37 "Somehow the async part is done with nsIWebProgress.": I don't think so, but I am not sure. That paths that I was tracing all went through OnStartRequest/OnStopRequest which is channel-based stream processing. I think you are supporting landing what we can, and living with the error message for empty attachment. If so I'll try to make progress on that approach.

(In reply to Kent James (:rkent) from comment #65) > I'm not sure it is understandable. Just tell me what happens after the synchronous part, that is, SendMsg(), returns. I haven't been able to set a single breakpoint on anything after the return of the function. But I will try OnStartRequest/OnStopRequest now. > I think you are supporting landing what we can, and living with the error > message for empty attachment. If so I'll try to make progress on that > approach. I support anything that will give the user a better experience and lead to a clearer understanding of what they can and can't do. What progress needs to be made? It works fine already, we'd only have to change the tests with their empty attachments.

OK, here some more debugging. After clicking "Send" of the reply, after the original message got moved to another folder, this is called: Sync part: 1) nsMsgCompose::SendMsg() 2) nsMsgAttachmentHandler::SnarfAttachment() 3) nsMsgCompose::SendMsg() returns Async part: 4) nsStreamConverter::OnStartRequest() 5) nsStreamConverter::OnStopRequest() 6) nsMsgAttachmentHandler::UrlExit() - call stack (many *OnStopRequest functions). That finally hits your: if (!m_size) status = NS_ERROR_FAILURE; I have no idea how OnStart/StopRequest is arranged to be called. Anyway. My approach from attachment 8760379 [details] [diff] [review] (which you gave f-) was to detect "bad" attachments in the sync process during SendMsg(). My fix was to detect access to dead messages in nsMailboxUrl::GetMsgHdrForKey(). Admittedly, that only fixed one part of the problem, refer the three test cases listed in comment #47. I can see that detecting an error in the async part comes late and communicating that attachments "didn't work" over there is hard. That's why I tried to ensure a valid attachment in the sync part (also given the fact that I couldn't get hold of the async part). Here just a wild fantasy: Can't we add a member to the attachment where the sync part can flag a "bad" attachment? Then the async part could check that instead of relying on m_size?

Something along these lines: Sync part: In SnarfAttachment() before FireURLRequest() set m_could_access_attachment to false. In the callback FetcherURLDoneCallback() set this to true. From memory, the callback wasn't called if there was no access to the attachment. Async part: Then check this in nsMsgAttachmentHandler::UrlExit() instead of relying on m_size. Just dreaming, I'll try it tomorrow unless Kent gets to it first. There must be a decent way to carry information from the sync to the async part.

Ignore comment #68, it was late-night/early-morning wishful thinking. FetcherURLDoneCallback() is the thing that is called in the async part which in turn calls UrlExit(). So it's too late in there.

I find it ironic that you comment "What progress needs to be made? It works fine already ..." then proceed to give several comments about additional details of the process, and suggestions about ways to better detect issues. I'm proposing that we fix the "Attaching..." hang with my patch, "landing what we can, and living with the error message for empty attachment" by fixing the tests, and deal with other improvements in other bugs. I have not looked at your patch other than the 15 minutes from comment 35. So, do we: 1) Accept my "Fix for the error handling", but first fix any tests that are assuming empty attachments? 2) Investigate your patch further (with or without my patch), or 3) Investigate further whether it is possible to properly detect existing but empty attachments prior to fixing the underlying hang, which then exposes that issue?

Sorry about the irony ;-) It comes from not being able to edit BMO comments. (In reply to Kent James (:rkent) from comment #70) > 1) Accept my "Fix for the error handling", but first fix any tests that are > assuming empty attachments? I'd say so. Who'd going to look into those cryptic xpcshell tests? As for my patch in attachment 8761139 [details] [diff] [review]: It only makes sure that the message "Attaching %S..." always has a decent value for the %S supplied. You can review this in two minutes. I removed the hunk in nsMailboxUrl::GetMsgHdrForKey() that checked for dead messages (comment #47).

Not sure who should review this, but here is a fix with the changes to stop the failing tests. Try run is here: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0b21c36bfcd1ed160007781d71fe46deb5e31310 The IMAP change is particularly difficult to review. I'm not sure we have anybody who would feel confident reviewing this!

BTW Jorg I am still focusing narrowly on the issue of the hung attachment. Your patch may still make sense as well to give better information on why.

Comment on attachment 8764052 [details] [diff] [review] Fix failing tests by eliminating null test attachments [checked in with mods, comment #76] Review of attachment 8764052 [details] [diff] [review]: ----------------------------------------------------------------- We already discussed this quite a bit. I didn't run the tests, but I assume you did. r=jorgk. Can you address the nit below, please. I'll also get you to review my patch since Magnus seems busy. So we could land both together. ::: mailnews/compose/test/unit/test_messageHeaders.js @@ +374,5 @@ > yield richCreateMessage(fields, [plainAttachment], identity); > checkDraftHeaders(plainAttachmentHeaders, "2"); > > let httpAttachment = makeAttachment({ > + url: "data:text/html,%3Chtml%2F%3E", That's <html/> right? You can't simply put the < > for better readability? And why is there a /?

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #74) > Comment on attachment 8764052 [details] [diff] [review] > Fix failing tests by eliminating null test attachments > > Review of attachment 8764052 [details] [diff] [review]: > ----------------------------------------------------------------- > > We already discussed this quite a bit. I didn't run the tests, but I assume > you did. > r=jorgk. > Can you address the nit below, please. I'll also get you to review my patch > since Magnus seems busy. So we could land both together. > > ::: mailnews/compose/test/unit/test_messageHeaders.js > @@ +374,5 @@ > > yield richCreateMessage(fields, [plainAttachment], identity); > > checkDraftHeaders(plainAttachmentHeaders, "2"); > > > > let httpAttachment = makeAttachment({ > > + url: "data:text/html,%3Chtml%2F%3E", > > That's <html/> right? > You can't simply put the < > for better readability? > And why is there a /? I saw one example in MDN where they escaped the characters in a data uri, but looking through existing tests there are plenty of example where they do not. I just replaced it with <html><body></body></html> which was used in some Mozilla examples.

Comment on attachment 8764052 [details] [diff] [review] Fix failing tests by eliminating null test attachments [checked in with mods, comment #76] Pushed with fixed nits https://hg.mozilla.org/comm-central/rev/40f4c524ba0b

Comment on attachment 8761139 [details] [diff] [review] Supply proper message arguments (v4a) Review of attachment 8761139 [details] [diff] [review]: ----------------------------------------------------------------- OK with changes, r+=me with the changes. ::: mailnews/compose/src/nsMsgSend.cpp @@ +2509,5 @@ > // > > // Display some feedback to user... > nsString msg; > + nsAutoString attachmentFileName; Please move this to right before you use it, that is before: attachmentFileName.AssignASCII(asciiSpec.get()); (Otherwise, it is still defined when a little later it is redefined in a new block, which can be very confusing). @@ +2516,5 @@ > + if (!params.IsEmpty()) { > + formatParams[0] = params.get(); > + } else { > + nsCString asciiSpec; > + m_attachments[i]->mURL->GetAsciiSpec(asciiSpec); I don't see any clear evidence that we are confident that ->mURL is non-null. Perhaps you could guarantee that by tracing through the calling code, but instead just add a check. That is, replace the } else { with: } else if (m_attachments[i]->mURL) { @@ +2541,5 @@ > nsAutoString attachmentFileName; > nsresult rv = ConvertToUnicode(nsMsgI18NFileSystemCharset(), m_attachments[i]->m_realName, attachmentFileName); > + if (attachmentFileName.IsEmpty()) { > + nsCString asciiSpec; > + m_attachments[i]->mURL->GetAsciiSpec(asciiSpec); Same issue here. Change the if to: if (attachmentFileName.IsEmpty() && m_attachments[i]->mURL)

Comment on attachment 8761139 [details] [diff] [review] Supply proper message arguments (v4a) Review of attachment 8761139 [details] [diff] [review]: ----------------------------------------------------------------- Kent, I can't do what you asked for, see below. ::: mailnews/compose/src/nsMsgSend.cpp @@ +2517,5 @@ > + formatParams[0] = params.get(); > + } else { > + nsCString asciiSpec; > + m_attachments[i]->mURL->GetAsciiSpec(asciiSpec); > + attachmentFileName.AssignASCII(asciiSpec.get()); You want me to move it here? That's what I had initially. @@ +2518,5 @@ > + } else { > + nsCString asciiSpec; > + m_attachments[i]->mURL->GetAsciiSpec(asciiSpec); > + attachmentFileName.AssignASCII(asciiSpec.get()); > + formatParams[0] = attachmentFileName.get(); This grabs a pointer to the content of attachmentFileName. @@ +2524,2 @@ > mComposeBundle->FormatStringFromName(MOZ_UTF16("gatheringAttachment"), > formatParams, 1, getter_Copies(msg)); And since attachmentFileName would have gone out of scope and been destroyed, this now prints garbage. So Kent, I can't do what you asked for. If instead of the .get() I use something else to take ownership of the string, I then need to free formatParams, but only in the case it was supplied from attachmentFileName.

(In reply to Kent James (:rkent) from comment #75) > I saw one example in MDN where they escaped the characters in a data uri, > but looking through existing tests there are plenty of example where they do > not. I just replaced it with <html><body></body></html> which was used in > some Mozilla examples. This caused test failures: TEST-UNEXPECTED-FAIL | mailnews/compose/test/unit/test_messageHeaders.js | testContentHeaders - [testContentHeaders : 71] "\\"data:text/html,<html><body></body></ht\\tml>\\"" == "\\"data:text/html,<html><body></body></html>\\""

Please check that you're OK with the change.

Pushed with fix to failing test: https://hg.mozilla.org/comm-central/rev/3a7dac0fdaca Total pushes in the bug: https://hg.mozilla.org/comm-central/rev/40f4c524ba0b (see comment #76) https://hg.mozilla.org/comm-central/rev/3a7dac0fdaca

Comment on attachment 8764142 [details] [diff] [review] Supply proper message arguments (v4b) [checked in with mods, comment #81] Review of attachment 8764142 [details] [diff] [review]: ----------------------------------------------------------------- Yes that is fine. I was going to ask you to remove the double declare of the string, and that is what you did on your own!

(In reply to Kent James (:rkent) from comment #82) > Yes that is fine. I was going to ask you to remove the double declare of the > string, and that is what you did on your own! Glad you like it. I fixed your test, too ;-) How do you feel about uplifting this? This has been a long standing issue, so it would be good to include it into TB 45.x one day.

I'm hesitant to uplift to TB 45 for two reasons. First, flagging as errors empty attachments could have unforeseen consequences. Second, the additional listener added in the IMAP case is somewhat mysterious to me and could have unforeseen consequences.

Comment on attachment 8764052 [details] [diff] [review] Fix failing tests by eliminating null test attachments [checked in with mods, comment #76] I think that we should uplift these to beta and aurora, even if we don't for esr45. It would be good to be able to point to a beta for people to test that the fix solves their issues for the many manifestations of this issue.

Comment on attachment 8764052 [details] [diff] [review] Fix failing tests by eliminating null test attachments [checked in with mods, comment #76] Sure.

Comment on attachment 8764142 [details] [diff] [review] Supply proper message arguments (v4b) [checked in with mods, comment #81] This needs uplift, too.

Aurora (TB 49): https://hg.mozilla.org/releases/comm-aurora/rev/aca753894875 https://hg.mozilla.org/releases/comm-aurora/rev/41cdfb1f9c8b

Beta (TB 48): https://hg.mozilla.org/releases/comm-beta/rev/999a626ee960 https://hg.mozilla.org/releases/comm-beta/rev/4fdb71bcd424

Sorry to learn this won't be uplifted for 45ESR... But, at least we only have a couple more months to wait... Really glad to see the work on this bug, thanks to all!

"I'm hesitant to uplift to TB 45" is not quite the same as "We won't uplift to TB 45" but it is close. Still, I think that "wontfix" has a finality that is overstated. I'm still open to opinions and results from the beta tests, but an uplift is still unlikely.