Forwarding mail as an attachment fails with "error attaching subject.eml" when sending or saving

RESOLVED FIXED in Thunderbird 14.0

Status

MailNews Core
Composition
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rsx11m, Assigned: Bienvenu)

Tracking

({regression})

Trunk
Thunderbird 14.0
All
Windows 7
regression

Thunderbird Tracking Flags

(thunderbird13+ fixed)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
+++ This bug was initially created as a clone of Bug #736571 +++

Today's trunk build, using the plain-text editor (don't know if it matters).

1. Select Message > Forward As > Attachment, composition window opens.
2. Attachment is in the attachment pane and looks normal.
3. Try to send or save as draft.
4. See the error message that "subject.eml" couldn't be attached.

Looks like another fall-out from the "attachment in cloud" patch, but I may be wrong in that assumption (i.e., didn't try the respective earlier nightly).

Thanks to L.A.R. Grizzly for reporting this bug on MozillaZine.
(Reporter)

Updated

5 years ago
Summary: Forwarding mail as an attachment fails with "subject.eml not found" when sending or saving → Forwarding mail as an attachment fails with "error attaching subject.eml" when sending or saving

Comment 1

5 years ago
Here's a link to my post on Mozillazine if interested:

http://forums.mozillazine.org/viewtopic.php?p=11831885#p11831885

Comment 2

5 years ago
Works fine for me on the very latest trunk build.
(Assignee)

Comment 3

5 years ago
yeah, works fine here too with daily build.
(Reporter)

Comment 4

5 years ago
So, what might make the difference...

I can reproduce with several messages from my Local Folders, with or without a Subject being present. My default composition mode is plain text. That's a profile migrated from 10.0 and earlier, i.e., that wasn't a fresh profile.

I get the following in the Error Console after dismissing the error dialog:

Error: GenericSendMessage FAILED: [Exception... "Component returned failure code: 0x8055311a [nsIMsgCompose.SendMsg]"  nsresult: "0x8055311a (<unknown>)"  location: "JS frame :: chrome://messenger/content/messengercompose/MsgComposeCommands.js :: GenericSendMessage :: line 2823"  data: no]
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 2826

No add-ons other than Test Pilot (enabled or disabled doesn't make a difference).

Comment 5

5 years ago
Composition mode doesn't seem to make a dif
With or without .eml
Same error message here, using file>>send later, rather than actually sending.
Testing with:
Mozilla/5.0 (Windows NT 5.1; rv:14.0) Gecko/20120314 Thunderbird/14.0a1 ID:20120314030025

Solid failure, regardless of content.
(Reporter)

Comment 6

5 years ago
Testing Thunderbird/14.0a1 BuildID=20120317030033 on Windows 7 (64 bit) here.
I have neither Cloud nor IM set up, but haven't disabled it either.

Comment 7

5 years ago
This only seems to apply to Windows with POP3 messages.
(Reporter)

Comment 8

5 years ago
I didn't test on Linux or directly with an unsynchronized IMAP message, so let's declare it Windows-only for now. Maybe that's some drive-letter path issue?
OS: All → Windows 7
tracking-thunderbird13: ? → +
Checked with trunk builds on Win-XP.

(Problem-A) Local mail folder, Subject: ABC.
Internal url of attached mail in composition window. (#3247 = Offset)
> mailbox-message://x@x.x.x/Inbox/Mail-Forward-Test#3247
Save As Draft faild with following alert.
> Save Draft Error
>  Unable to save your message as draft.
>  There was an error attaching ABC.eml. Please check if you have access to the file.
>    [ OK ]
After click OK, same exception as comment #4 was shown at Error Console.
(Line No. depends on build)

Regression Window of (Problem-A) :
No problem.
  2012-03-07-03-00-33-comm-central/thunderbird-13.0a1.en-US.win32.zip
  Mozilla/5.0 (Windows NT 5.1; rv:13.0) Gecko/20120307 Thunderbird/13.0a1
Problem started to occur.
  2012-03-08-03-00-48-comm-central/thunderbird-13.0a1.en-US.win32.zip
  Mozilla/5.0 (Windows NT 5.1; rv:13.0) Gecko/20120308 Thunderbird/13.0a1

comm-central - pushlog, From: 2012-03-07 02:00:00  To: 2012-03-08 04:00:00
http://hg.mozilla.org/comm-central/pushloghtml?startdate=2012-03-07+02%3A00%3A00&enddate=2012-03-08+04%3A00%3A00

(Problem-B) IMAP mail folder, Subject: ABC. (both offline-use=On and offlin-use=Off)
Internal url of attached mail in composition window. (#12125 = UID)
> imap-message://yatter.one%40gmail.com@imap.gmail.com/INBOX#12125
Save As Draft was executed normally.
But following error was reported to Error Console.
> Timestamp: 2012/03/21 09:44:12
> Error: bundle is not defined
> Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 4337
> (Line No. depends on build)
"dialog after save" is enabled at Copies&Folders in my prefs.

Problem-B occurs in 20120307 build. So different regresson from Problem-A.
(Reporter)

Comment 10

5 years ago
(In reply to WADA from comment #9)
> Regression Window of (Problem-A) :
> No problem.
>   2012-03-07-03-00-33-comm-central/thunderbird-13.0a1.en-US.win32.zip
>   Mozilla/5.0 (Windows NT 5.1; rv:13.0) Gecko/20120307 Thunderbird/13.0a1
> Problem started to occur.
>   2012-03-08-03-00-48-comm-central/thunderbird-13.0a1.en-US.win32.zip
>   Mozilla/5.0 (Windows NT 5.1; rv:13.0) Gecko/20120308 Thunderbird/13.0a1

Thanks WADA, that likely eliminates BigFiles as the source of this regression.
On the other hand, nothing in the pushlog catches my eyes as a possible cause...
(Assignee)

Comment 11

5 years ago
Created attachment 608448 [details] [diff] [review]
proposed fix

this is backing out part of Bug 186724 - SetSpec fails because nsMailboxUrl::ParseUrl() fails with a file:///test url, because fileURL->GetFile(getter_AddRefs(fileURLFile)) fails with a non-existent file. This failure is harmless, and lets forward as attachment work again.
Assignee: nobody → dbienvenu
Attachment #608448 - Flags: review?(neil)
Comment on attachment 608448 [details] [diff] [review]
proposed fix

>       nsCOMPtr<nsIURI> aURL;
>       rv = messageService->GetUrlForUri(uri.get(), getter_AddRefs(aURL), nsnull);
>+      // we ignore errors with SetSpec because attached eml files seem
>+      // to cause issues.
>+      if (aURL)
>+        (void) aURL->SetSpec(nsDependentCString(uri.get()));
> 
>       rv = NS_NewInputStreamChannel(getter_AddRefs(m_converter_channel), aURL, nsnull);
Is there any chance you can explain what this code is doing? In particular, why is it using GetUrlForUri but then trying to change the spec back?

Also, nsDependentCString(.get()) is a really silly thing to do ;-)
(Assignee)

Updated

5 years ago
Blocks: 186724
No longer blocks: 698925
(Assignee)

Comment 13

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #12)
> Comment on attachment 608448 [details] [diff] [review]
> proposed fix
> 
> >       nsCOMPtr<nsIURI> aURL;
> >       rv = messageService->GetUrlForUri(uri.get(), getter_AddRefs(aURL), nsnull);
> >+      // we ignore errors with SetSpec because attached eml files seem
> >+      // to cause issues.
> >+      if (aURL)
> >+        (void) aURL->SetSpec(nsDependentCString(uri.get()));
> > 
> >       rv = NS_NewInputStreamChannel(getter_AddRefs(m_converter_channel), aURL, nsnull);
> Is there any chance you can explain what this code is doing? In particular,
> why is it using GetUrlForUri but then trying to change the spec back?
> 
the spec can be significantly different after GetUrlForUri is done with it, since nsMailboxService::PrepareMessageUrl can munge it. That function leaves originalSpec set to the original uri. I'm not sure what code downstream cares. I can try without it, but that's even scarier than the initial error handling change was :-(
Comment on attachment 608448 [details] [diff] [review]
proposed fix

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

::: mailnews/compose/src/nsMsgAttachmentHandler.cpp
@@ +589,5 @@
>        rv = messageService->GetUrlForUri(uri.get(), getter_AddRefs(aURL), nsnull);
> +      // we ignore errors with SetSpec because attached eml files seem
> +      // to cause issues.
> +      if (aURL)
> +        (void) aURL->SetSpec(nsDependentCString(uri.get()));

Comment should be inside the if.
(Assignee)

Comment 15

5 years ago
(In reply to Serge Gautherie (:sgautherie) from comment #14)
> Comment on attachment 608448 [details] [diff] [review]
> proposed fix
> 
> Review of attachment 608448 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/compose/src/nsMsgAttachmentHandler.cpp
> @@ +589,5 @@
> >        rv = messageService->GetUrlForUri(uri.get(), getter_AddRefs(aURL), nsnull);
> > +      // we ignore errors with SetSpec because attached eml files seem
> > +      // to cause issues.
> > +      if (aURL)
> > +        (void) aURL->SetSpec(nsDependentCString(uri.get()));
> 
> Comment should be inside the if.

I think that's less readable, and ugly, so, no.
(In reply to David :Bienvenu from comment #11)
> SetSpec fails because
> nsMailboxUrl::ParseUrl() fails with a file:///test url, because
> fileURL->GetFile(getter_AddRefs(fileURLFile)) fails with a non-existent
> file.

Should the comment in the patch be less vague?
Should we special-case .eml files?
Could an automatic test be added?
(Assignee)

Comment 17

5 years ago
The first question to answer is Neil's - perhaps we don't have to change the spec back, since failure to do so doesn't cause forward as attachment to fail.

We can do a mozmill test that forwards as attachment, and then attempts to save a local draft, to reproduce this issue.

Updated

5 years ago
Duplicate of this bug: 739254
(Assignee)

Comment 19

5 years ago
so, FTR, we do have a mozmill test that covers this.
(Assignee)

Comment 20

5 years ago
the SetSpec call was added by the patch in bug 161488, which had to do with decoding s/mime messages before forwarding. I suspect we can just remove the setspec call since forwarding as attachment works w/o it. I'll have to see if I can find an encrypted message to make sure we're not regressing bug 161488.
(Assignee)

Comment 21

5 years ago
Created attachment 609522 [details] [diff] [review]
remove setspec code completely

I think we can get rid of the setspec call entirely.
Attachment #608448 - Attachment is obsolete: true
Attachment #608448 - Flags: review?(neil)
Attachment #609522 - Flags: review?(neil)
Comment on attachment 609522 [details] [diff] [review]
remove setspec code completely

>       rv = messageService->GetUrlForUri(uri.get(), getter_AddRefs(aURL), nsnull);
[I wonder why kaie never checked this rv]
Attachment #609522 - Flags: review?(neil) → review+
(Assignee)

Comment 23

5 years ago
fixed on trunk - http://hg.mozilla.org/comm-central/rev/5bff4f51bfed
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
(Assignee)

Comment 24

5 years ago
Comment on attachment 609522 [details] [diff] [review]
remove setspec code completely

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):
Attachment #609522 - Flags: approval-comm-aurora?
(In reply to neil@parkwaycc.co.uk from bug 736789 comment #22)
> >       rv = messageService->GetUrlForUri(uri.get(), getter_AddRefs(aURL), nsnull);
> [I wonder why kaie never checked this rv]

I filed Bug 739616.
status-thunderbird14: affected → ---
Component: Mail Window Front End → Composition
Product: Thunderbird → MailNews Core
QA Contact: front-end → composition
Attachment #609522 - Flags: approval-comm-aurora? → approval-comm-aurora+
http://hg.mozilla.org/releases/comm-aurora/rev/58567bbd0b49
status-thunderbird13: affected → fixed
You need to log in before you can comment on or make changes to this bug.