Last Comment Bug 736789 - Forwarding mail as an attachment fails with "error attaching subject.eml" when sending or saving
: Forwarding mail as an attachment fails with "error attaching subject.eml" whe...
Status: RESOLVED FIXED
: regression
Product: MailNews Core
Classification: Components
Component: Composition (show other bugs)
: Trunk
: All Windows 7
: -- major (vote)
: Thunderbird 14.0
Assigned To: David :Bienvenu
:
Mentors:
http://forums.mozillazine.org/viewtop...
: 739254 (view as bug list)
Depends on:
Blocks: 186724
  Show dependency treegraph
 
Reported: 2012-03-17 17:04 PDT by rsx11m
Modified: 2012-06-23 17:09 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed


Attachments
proposed fix (1.07 KB, patch)
2012-03-22 13:31 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
remove setspec code completely (1.29 KB, patch)
2012-03-26 16:05 PDT, David :Bienvenu
neil: review+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description rsx11m 2012-03-17 17:04:02 PDT
+++ 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.
Comment 1 L.A.R. Grizzly 2012-03-17 17:33:40 PDT
Here's a link to my post on Mozillazine if interested:

http://forums.mozillazine.org/viewtopic.php?p=11831885#p11831885
Comment 2 Jim Porter (:squib) 2012-03-17 17:38:03 PDT
Works fine for me on the very latest trunk build.
Comment 3 David :Bienvenu 2012-03-17 17:44:40 PDT
yeah, works fine here too with daily build.
Comment 4 rsx11m 2012-03-17 18:47:51 PDT
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 Joe Sabash [:JoeS1] 2012-03-17 19:05:53 PDT
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.
Comment 6 rsx11m 2012-03-17 19:16:55 PDT
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 Jim Porter (:squib) 2012-03-17 19:59:19 PDT
This only seems to apply to Windows with POP3 messages.
Comment 8 rsx11m 2012-03-17 20:02:15 PDT
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?
Comment 9 WADA 2012-03-20 20:33:15 PDT
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.
Comment 10 rsx11m 2012-03-20 20:44:28 PDT
(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...
Comment 11 David :Bienvenu 2012-03-22 13:31:15 PDT
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.
Comment 12 neil@parkwaycc.co.uk 2012-03-22 17:34:30 PDT
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 ;-)
Comment 13 David :Bienvenu 2012-03-23 08:04:59 PDT
(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 14 Serge Gautherie (:sgautherie) 2012-03-23 16:48:53 PDT
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.
Comment 15 David :Bienvenu 2012-03-23 16:49:46 PDT
(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.
Comment 16 Serge Gautherie (:sgautherie) 2012-03-25 07:20:28 PDT
(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?
Comment 17 David :Bienvenu 2012-03-25 07:27:20 PDT
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.
Comment 18 Mike Conley (:mconley) - (needinfo me!) 2012-03-26 08:58:18 PDT
*** Bug 739254 has been marked as a duplicate of this bug. ***
Comment 19 David :Bienvenu 2012-03-26 15:13:07 PDT
so, FTR, we do have a mozmill test that covers this.
Comment 20 David :Bienvenu 2012-03-26 15:47:27 PDT
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.
Comment 21 David :Bienvenu 2012-03-26 16:05:33 PDT
Created attachment 609522 [details] [diff] [review]
remove setspec code completely

I think we can get rid of the setspec call entirely.
Comment 22 neil@parkwaycc.co.uk 2012-03-27 07:04:41 PDT
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]
Comment 23 David :Bienvenu 2012-03-27 07:39:52 PDT
fixed on trunk - http://hg.mozilla.org/comm-central/rev/5bff4f51bfed
Comment 24 David :Bienvenu 2012-03-27 07:40:14 PDT
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):
Comment 25 Serge Gautherie (:sgautherie) 2012-03-27 08:17:28 PDT
(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.
Comment 26 Mark Banner (:standard8) 2012-04-04 02:30:41 PDT
http://hg.mozilla.org/releases/comm-aurora/rev/58567bbd0b49

Note You need to log in before you can comment on or make changes to this bug.