239226 - Opening Forward Message Attachment Opens parent message in a new window

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

In a trunk build (Mozilla Thunderbird 0.5+ (20040329)) when I have a message
from someone containing a forwarded message as an attachment, opening the
forwarded message in a new window by double clicking the attachment does not work. 

Steps to reproduce:
1) Preview an email containing a forwarded message as an attachment.
2) Double click the attachment to view the forwarded message in a new Mail window

Expected results:
- view the email attachment by itself in a new window

Actual results:
- parent email message itself (the one that contains the forwarded email
attachment) opens in a new window

As a result it is impossible to read most forwarded emails.

Comment 1

[Scott MacGregor]

this used to work

Comment 2

[Scott MacGregor]

I know this used to work not that long ago. I'm not sure what broke it. I will
investigate

Comment 3

[Scott MacGregor]

the regression is imap only. Moving it to a local folder and things still work fine.

When opening the attachment the url for the attachment looks like:
imap://[e-mail]:143/fetch%3EUID%3E.INBOX%3E858?part=1.2

This then gets turned into the following url by the imap service when it is run:
imap://[e-mail]:143/fetch%3EUID%3E.INBOX%3E858.part=1.2&type=x-message-display&filename=%5BFwd%3A%20Test

The structure of the url looks good. We just don't seem to be streaming the part
out correctly anymore for imap.

Comment 4

[timeless]

i don't remember it ever working. i've been using seamonkey for imap for a while
now. i was going to mark this w/ dupeme, but this is a thunderbird bug.

Comment 5

[Scott MacGregor]

this has worked for over a year until just recently. It was turned on in April
of 2003 and it worked in Thunderbird 0.5 so it's a very recent regression.

Bug #143565 is the bug that implemented this feature.

Comment 6

[Scott MacGregor]

the last working build for this was on the 18th of February. The string
defragmentation branch landed on that day and nothing really changed in
mailnews. My guess is we are looking for one of those subtle string change
regressions probably somewhere in libmime where we decide to extract a part from
a message stream.

Comment 7

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #0)

Ben Goodger, your problem is same as Mozilla Mail&News bug 203570?
> [message/rfc822] opening a fwd local message has original messages's attachments

Comment 8

[Scott MacGregor]

nope that's a separate issue that has never worked. This is a regression please
read the comments above :)

Comment 9

[Scott MacGregor]

Interesting, using a debug 0.5 build and comparing it to a debug trunk build
that has the problem, I see the following:

The URL sent to nsMessengerContentHandler::HandleContent which we then turn
around and open in a stand alone message window looks like:

0.5:
"imap://[e-mail]:143/fetch%3EUID%3E.INBOX%3E250?part=1.2&type=message/rfc822&filename=Mozilla%20Email"

0.5+:
+	mStr	0x033f6af8
"imap://[e-mail]:143/fetch%3EUID%3E.INBOX%3E250.part=1.2&type=message/rfc822&filename=Mozilla%20Email"

Note the lack of a question mark before the part=1.2 string in the url

Comment 10

[Scott MacGregor]

Attachment: the fix

The problem was caused by the string defragmentation landing. The url string we
were trying to run was getting mutated out from underneath the nsIImapUrl
object.

Comment 11

[Scott MacGregor]

Comment on attachment 145295 [details] [diff] [review]
the fix

asking Darin for the r=. This was a regression caused  by the string
defragmentation landing. The code in question was causing the url spec for the
underlying nsIURI being run to get modified inadvertantly. I'm calling
BeginWriting on the nsCAutoString before we attempt to modify the underlying
buffer.

Is this ok Darin?

Comment 12

[David :Bienvenu]

Comment on attachment 145295 [details] [diff] [review]
the fix

ok with me if it's ok with Darin.

Comment 13

[Scott MacGregor]

this regression fix is going to need 1.7 final status I think...I'll ask over
email. 

Comment 14

[chris hofmann]

a=chofmann for the trunk if it looks good to darin.  is this seamonkey too?

Comment 15

[Darin Fisher]

Comment on attachment 145295 [details] [diff] [review]
the fix

>Index: nsImapProtocol.cpp

>+  char * urlString;
>+  urlSpec.BeginWriting(urlString);
>+
>   // for now, truncate of the query part so we don't duplicate urls in the cache...
>+  char * anchor = (char *)strrchr(urlString, '?');

you can also write this as a shorthand:

  char * anchor = (char *)strrchr(urlSpec.BeginWriting(), '?');

thank dbaron for adding a BeginWriting() that returns the iterator :-)

r=darin

Comment 16

[Darin Fisher]

I just filed bug 239405 on the general problem exposed by this bug.  I'll search
the rest of the tree for similar problems.  Wish me luck! :-)

Comment 17

[Scott MacGregor]

I checked in the fix using Darin's suggested shorthand notation. Please note
this effects Thunderbird and seamonkey

Comment 18

[Mike Cowperthwaite]

I can confirm this has been fixed in Seamonkey
  Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040402