Closed Bug 135279 Opened 22 years ago Closed 22 years ago

Can't save e-mail messages on Mac!

Categories

(MailNews Core :: MIME, defect, P2)

Product:
MailNews Core
Component:
MIME
Platform:
PowerPC
macOS
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: bugzilla, Assigned: bugzilla)

References

Details

(Keywords: regression, Whiteboard: [adt2] Have fix)

Attachments

(2 files, 1 obsolete file)

Proposed fix, v2
4.04 KB, patch
Details | Diff | Splinter Review
Alternative patch, v1
4.04 KB, patch
naving
: review+
mscott
: superreview+
dbaron
: approval+
Details | Diff | Splinter Review 
I can't save rich-content HTML messages on Mac OS X.   If I save an e-mail with
a background image, embedded web page, or other rich content, the file size is
zero bytes (in other words, it didn't save anything).

Marking as major...

2002032915
Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:0.9.9+) Gecko/20020329
the database is pretty much not involved in save as html operations.  I don't
know what exactly is involved, but I think mime is involved.

Is this an imap or a local message? Do normal messages get saved correctly?
Assignee: bienvenu → ducarroz
Component: Mail Database → MIME
No... I just found out plaintext or HTML text messages don't save either.   I
first noticed it with rich HTML but now I find it is with other messages too!
Changing summary to "Can't save e-mail messages on Mac OS X!"
  Should the severity be upgraded?
Summary: Can't save rich HTML messages on Mac OS X! → Can't save e-mail messages on Mac OS X!
QA Contact: gayatri → esther
are these imap, local, or news messages? Or all three?
They are local (POP3) messages.
Stephen was able to reproduce this on his Mac OSX.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: nsbeta1
Keywords: nsbeta1+
Priority: -- → P2
Whiteboard: nsbeta1 → [adt2]
Target Milestone: --- → mozilla1.0
Note, I can save mail messages to the desktop with 6.2.2 on the mac osx and they
are not 0 bytes.  This is regression.
Keywords: regression
I am not able to reproduce this problem using recent builds! Can somebody give
me the exact step to reproduce the bug. Thanks
Status: NEW → ASSIGNED
Joel, do you have any special characters in your volume name like a / or \? what
is the full patch of your profile directory and what is the patch where you try
to save the message?
The volume name is "Macintosh HD".  The path I try to save to is "Macintosh HD"
or "Macintosh HD/Users/marknels/Documents" or "Macintosh HD/Users/marknels/Desktop".
Profile directory is "Macintosh HD/Users/marknels/Library/Mozilla/Profiles/"

Steps to reproduce:
Select e-mail message (HTML or plain) from a local folder for a POP account.  
Click File, then Save As > File...
Save message to disk.  File size will be O bytes.
Works just fine on Windows XP in 2002041603 (0.9.9+)   This is a Mac-specific bug.
*** Bug 138622 has been marked as a duplicate of this bug. ***
similar report in bug 139889 (Mac system 9.*)
Using branch 20020422 on mac os 10.1 this is working for me too.  Also still
works for  mac os9.1, linux and windows

I used a mail message I created with the NS6, contains one line in html and save
as to my desktop.  Saved as File using menu and right mouse click. 

I had a message.eml on my desktop from 4-5 that was zero bites so I must have
tested it too.   I check with Stephen again, maybe this is fixed. 
Jean-Francois the key is to do this from your local folder or POP account, IMAP
is OK. Still a bug with 20020422branch build.
 
Thank you very much Esther, I can know reproduce the problem.
Looks like a regression caused by the fix for bug 98650. When reading the data
from the local db, we are searching for LF to extract lines per line but the db
use a CR on Mac therefore we failed.

Naving, what should I do?
Attached patch Proposed fix, v1 (obsolete) — — Splinter Review 
The problem is that the function nsMsgLineStreamBuffer::ReadNextLine extract
lines from a buffer by looking for LF. But in the case of a Local mailbox on
Mac, the line ending used is CR. This function use to have a member variable
that contains the line end token to look for: LF, CR or CRLF depending of the
situation. But Naving change that when fixing bug 98650. I reviewed carefully
the patch in that bug and spoke with Naving to understand why he had to do
that. The conclusion was that we were receiving lines ending with LF when we
were expecting CRLF which cause the original code to eat characters it should
not.

My fix consist to implement a mechanism which is not dependent on the line
ending but with a minimal effect on performance (which the fix for bug 98650
had improved). If I cannot find the default line ending character in the data
(LF), I look for a CR. If the data contains a CR, I change the default line
ending to CR for next time.
cc'ing mscott
Whiteboard: [adt2] → [adt2] Have fix
Append on MacOS 9 as well.
Summary: Can't save e-mail messages on Mac OS X! → Can't save e-mail messages on Mac!
I see one potential problem in the case streamed data get cut between a CR and a
LF. In that case we will see that as two lines instead of one! Should we use the
new detection system on Mac only to minimize the side effect? or maybe I should
not test for alternative line ending when looking at the buffer left over! or,
once I detect a line ending (LF or CR) for a specific buffer object, I should
not let change it again...
I think it would be much simpler and error free if we go back to what was before 
I changed it. I mean apart from those three lines we can just revert (backout)
that patch in bug 98650. That particular pop server is sending out LF when it
should send out CRLF. According to pop3 RFC it says that 

"Responses to certain commands are multi-line.  In these cases, which
   are clearly indicated below, after sending the first line of the
   response and a CRLF, any additional lines are sent, each terminated
   by a CRLF pair."

We cannot sacrifice correctness for performance.
Attached patch Proposed fix, v2 — — Splinter Review 
Same than previous patch but this one take care of potential side effect
introduce by the auto line ending detection.
Attachment #81068 - Attachment is obsolete: true
This is a way more conservative approch to fix the problem. I have change the
minum to prevent any regression.
Comment on attachment 81760 [details] [diff] [review]
Alternative patch, v1

+#if defined(XP_MAC)
+	m_lineStreamBuffer = new nsMsgLineStreamBuffer(OUTPUT_BUFFER_SIZE,
PR_TRUE, PR_TRUE, '\r');
+#else

there should be just one
PR_TRUE, eatCRLF defaults to PR_TRUE

I don't see where we are
initializing m_lineToken = '\n'

Does XP_MAC covers MAC_OSX 


r=naving, this is what
I had in mind.
Attachment #81760 - Flags: review+
>there should be just one PR_TRUE, eatCRLF defaults to PR_TRUE
not true when you need to change lineToken as this parameter is declared after
eatCRLF

>I don't see where we are initializing m_lineToken = '\n'
+
nsMsgLineStreamBuffer(PRUint32 aBufferSize, PRBool aAllocateNewLines, PRBool
aEatCRLFs = PR_TRUE, char aLineToken = '\n'); // specify the size of the buffer
you want the class to use....

>Does XP_MAC covers MAC_OSX
yes

Comment on attachment 81760 [details] [diff] [review]
Alternative patch, v1

with the exception of this line:

if (m_eatCRLFs && m_lineToken == '\n' && aNumBytesInLine > 0 &&
startOfLine[aNumBytesInLine-1] == '\r')

this code looks like a no-op for non Mac platforms. That is, the code should
behave in exactly the same fashion as before your patch since you are replacing
uses of '\n' in the routine with a variable which defaults to '\n'. Am I
reading that right?
Right, this is the only real modification. I have to change the original line
which was:
-        if (startOfLine[aNumBytesInLine-1] == '\r')

because in the case m_eatCRLFs is set to false and we are looking for a CR, we
will faultly detect the CR and therefore return the line without the CR.

this line should have originaly be:
-        if (m_eatCRLFs && startOfLine[aNumBytesInLine-1] == '\r')
but that is not taking care in the case of an line that contains only a LF or a
CR. In that case if m_eatCRLFs is true, aNumBytesInLine would be 0 and therefore
startOfLine[aNumBytesInLine-1] will read memory outside the bondary of the
buffer (this is a bug). That why I have added a check for that.

But I don't realy need to check if we are processing a LF token. The test could
be only:
+    if (m_eatCRLFs && aNumBytesInLine > 0 && startOfLine[aNumBytesInLine-1] ==
'\r') // Remove the CR in a CRLF sequence

I'll change that before checkin

Comment on attachment 81760 [details] [diff] [review]
Alternative patch, v1

ok make that change to then 
sr=mscott

for the TRUNK only. I retract my sr if ADT sees this and thinks they'll squeeze
it into the branch for beta (i don't think they will). Too risky for beta but
we want to start getting some bake time on the trunk.
Attachment #81760 - Flags: superreview+
Fix checked in the trunk.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
esther, can you verify this on the trunk?

Scott, how do you feel about this for rtm?
I see this problem on a Tru64 UNIX machine. When I try to save 
mails the browser crashes. This happens with Mozilla 1.0 RC3
and Mozilla 1.0 Release. However this works alright on the nightlies.
Is there any plan to port this fix to Mozilla 1.0 release?
Esther, can you verify this on the trunk?
nominating.
Keywords: adt1.0.1
*** Bug 149319 has been marked as a duplicate of this bug. ***
Esther, can you verify this on the trunk?
I will verify this today 6-10 on the trunk.
Verified on macos10.1 and 9.1 this is fixed and still works on winxp and linux
with trunk builds 20020610.  Verified.
Status: RESOLVED → VERIFIED
adding adt1.0.1+.  Please get drivers approval before checking in.
Keywords: adt1.0.1 → adt1.0.1+, mozilla1.0.1
Comment on attachment 81760 [details] [diff] [review]
Alternative patch, v1

Please land this on the 1.0.1 branch.  Once there, replace the
"mozilla1.0.1+" keyword with the "fixed1.0.1" keyword.
Attachment #81760 - Flags: approval+
Fix checked in the branch
Keywords: mozilla1.0.1+ → fixed1.0.1
Using branch builds 20020618 on MAC os10.1 this is fixed.  I also retested on
branch builds 20020618 on linux, Mac 9.1 and winxp to make sure this still worked.
Verified
Keywords: fixed1.0.1 → verified1.0.1
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: