100591 - [pp]Unable to Save an attachment from the twice forwarded mail

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect, P1)

Description

[marina]

*** observed with 2001-09-19-0.9.4. build ***
Steps to reproduce:
- compose a mail and forward it to yourself twice;
- rightclick on the envelope where the attachment is;
- choose "Save as..";
//note: no "Save As " dlgbox is coming up
( this is functionning correctly when the mail is forwarded just once)

Comment 1

[scottputterman]

reassigning to ducarroz

Comment 2

[Michael Baffoni]

Is this similar/same as Bug 95619 ?

Comment 3

[marina]

i don't think those two bugs are related. Bug 95619 reports a problem with
viewing attachments, this one deals with Saving them from the envelope.

Comment 4

[Jean-Francois Ducarroz]

I can still reproduce this problem using today's commercial build. CC'ing mscott

Comment 5

[marina]

still repro in 2002-01-07 build

Comment 6

[Denis Antrushin]

WORKSFORME with today's trunk...

Comment 7

[marina]

still not working for me with 2002-01-17 build, the Save as... dlgbox doesn't
come up.

Comment 8

[Jean-Francois Ducarroz]

The save as problem is bug 114473 for which I have a fix...

Comment 9

[Jean-Francois Ducarroz]

This problem occurs on Windows only and is due to the fact the name of the
attchment contains a column characters:

[Fwd:xxxxxxxx]

Comment 10

[Jean-Francois Ducarroz]

Attachment: Proposed fix for Widget, v1

This patch address the generic problem in the filePicker for Window when
setting a invalid default file name. The WinAPI is not really frendly in that
case!

First I added code to correctly detect error with fileName. 

In the second part, I modified SetDefaultString to truncate fileName when it's
too long and to remove illegal characters. That should reduce the change to hit
the error I detect in the first part of the patch and therefore avoid use to
clear the default name when we can fix it.

Comment 11

[Jean-Francois Ducarroz]

I still have more work to do in messenger to prevent problem when using the
command save all attachment or when the attachment name contains a platform path
separator (\, / or : depending of the platform).

cc'ing law and hyatt for the first patch as it's their area.

Comment 12

[Bill Law]

Comment on attachment 66820 [details] [diff] [review]
Proposed fix for Widget, v1

r=law

Definitely a good feature to avoid problems with screwy urls that get into the
file picker.  Thanks.

Comment 13

[Jean-Francois Ducarroz]

Attachment: Proposed fix for URILoader, v1

This patch address problem saving or opening an attachment which the name
contains a path separator. The easy way to reproduce this problem is to send a
document (MS Word by example) from a Mac with the following name:
"apple\ibm.doc" and try to open it on Windows.

Comment 14

[Jean-Francois Ducarroz]

Attachment: Proposed fix for mailnews/base, v1

This patch will sanitize file name when the user save an attachment to disk
from the Message Display window/pane.

Comment 15

[Jean-Francois Ducarroz]

With those 3 fixes, we are now able to save/display/open any attachment,
whatever the origin platform of this one.

Comment 16

[Jean-Francois Ducarroz]

*** Bug 33567 has been marked as a duplicate of this bug. ***

Comment 17

[Bill Law]

Comment on attachment 66934 [details] [diff] [review]
Proposed fix for URILoader, v1

r=law

Comment 18

[mike lima]

*** Bug 123133 has been marked as a duplicate of this bug. ***

Comment 19

[Scott MacGregor]

Comment on attachment 66934 [details] [diff] [review]
Proposed fix for URILoader, v1

It's a shame we can't put this in an inline global xpcom or string routine
instead of repeating it twice in this file and once more in mail. 

sr=mscott

Comment 20

[Scott MacGregor]

Comment on attachment 66939 [details] [diff] [review]
Proposed fix for mailnews/base, v1

sr=mscott for most of the changes in this file. However as I mentioned in the
last patch, it might be worth looking to see if we could use a global routine
or maybe a global MACRO in nsCRT.h which defines the path separators on each
platform. 

i.e. in nsCRT.h, how about adding below:
#define CRSTR "\015"
#define LFSTR "\012"
#define CRLF "\015\012"     /* A CR LF equivalent string */
the following:
#define PATH_SEPARATOR

where the value is defined based on the platform
#if defined (XP_MAC)
#define PATH_SEPARATORS ":"
#else if defined (XP_WIN)
#define PATH_SEPARATORS "\\/:*?\"

etc. 

what do you think?

Comment 21

[Jean-Francois Ducarroz]

PATH_SEPARATORS "\\/:*?\"

The only patch separator for windows is \, others are just illegal characters!
Also the patch in mailnews is a little bit different than in the others module.
Sure I am duplication code but as it was just a line (per platform), I did not
feel the need to have a have a macro of a function. If we need this stuff in
more places, then sure, a macro/function would be justified!

Comment 22

[Scott MacGregor]

Comment on attachment 66939 [details] [diff] [review]
Proposed fix for mailnews/base, v1

I couldn't tell how your macros were different in the uriloader vs. in mail?
They looked identical to me. We don't have to call it platform separator, it
can be called something different. But if we add a #define to nsCRT.h where we
define line feeds & the like (or we could add it to nsIFile.h), then your code
goes from: 

+#if defined(XP_MAC)
+  ucs2Str.ReplaceChar(':', '-');
+#elif defined(XP_WIN) || defined(XP_OS2)
+  ucs2Str.ReplaceChar("\\/:*?\"<>|", '_');
+#elif (defined(XP_UNIX) || defined(XP_BEOS)
+  ucs2Str.ReplaceChar('/', '_');
+#endif

in 3 separate places

to just
someString.ReplaceChar(THE_PLATFORM_SPECIFIC_DEFINE, '_');

which is much easier to read and then we aren't copying the platform values in
3 separate places. It'll be easier to maintain if any of them change.

Comment 23

[Cavin Song]

Comment on attachment 66939 [details] [diff] [review]
Proposed fix for mailnews/base, v1

r=cavin.

Comment 24

[Scott MacGregor]

Comment on attachment 66939 [details] [diff] [review]
Proposed fix for mailnews/base, v1

taking back the sr till we work out the #define issue.

Comment 25

[Jean-Francois Ducarroz]

I'll rework on this fix once I checked in the fix for bug 122815 as that will
create couple merge conflict...

Comment 26

[Jean-Francois Ducarroz]

Attachment: proposed fix, v2 (xpcom/io part)

I added defines for the path separator and illegal characters for each platform

Comment 27

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v2 (widget part)

Comment 28

[Jean-Francois Ducarroz]

the only change in the widget patch is the use of the new defined
FILE_ILLEGAL_CHARACTERS.

Comment 29

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v2 (uriloader part)

the only change in the uriloader patch is the use of the new defined
FILE_ILLEGAL_CHARACTERS and FILE_PATCH_SEPARATOR.

Comment 30

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v2 (mailnews part)

I rewrote a little bit this part of the fix to use the new defines and solve
merge conflict with the another fix I checked in about truncating filename on
Mac. Also I mofified an exixting function in nsMsgUtils.cpp which was filtering
illegal characters. You will remark that the original code was removing more
characters that I do now, this because it was made to work also on Window 3.1
which we are not supporting anymore :-)

Comment 31

[Scott MacGregor]

Comment on attachment 68574 [details] [diff] [review]
proposed fix, v2 (xpcom/io part)

sr=mscott. Be sure to get module owner approval for this change though (I'd try
dougt). I think this is the right place to put these defines though.

Comment 32

[Scott MacGregor]

Comment on attachment 68579 [details] [diff] [review]
Proposed fix, v2 (uriloader part)

sr=mscott

Comment 33

[Scott MacGregor]

Comment on attachment 68583 [details] [diff] [review]
Proposed fix, v2 (mailnews part)

sr=mscott

Comment 34

[Alec Flett]

Comment on attachment 68576 [details] [diff] [review]
Proposed fix, v2 (widget part)

seems reasonable. sr=alecf

Comment 35

[Jean-Francois Ducarroz]

Comment on attachment 68574 [details] [diff] [review]
proposed fix, v2 (xpcom/io part)

I have added the needed defines into nsCRT.h

Comment 36

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v2 (xpcom/ds part)

I moved the needed defines from nsILocalFiles.idl to nsCRT.h as requested by
reviewer.

Comment 37

[Scott MacGregor]

Comment on attachment 68911 [details] [diff] [review]
Proposed fix, v2 (xpcom/ds part)

sr=mscott

Comment 38

[Jean-Francois Ducarroz]

Fixed and checked in

Comment 39

[Trix Supremo]

verified on mozilla & netscape trunk 2002022703 builds