Deleted text gets sent when Forwarding e-mail

VERIFIED FIXED in mozilla1.2final

Status

MailNews Core
Composition
P3
major
VERIFIED FIXED
15 years ago
9 years ago

People

(Reporter: parish, Assigned: Jean-Francois Ducarroz)

Tracking

({regression})

Trunk
mozilla1.2final
regression

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: have fix, waiting approval)

Attachments

(1 attachment, 1 obsolete attachment)

629 bytes, patch
varada
: review+
(not reading, please use seth@sspitzer.org instead)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2b) Gecko/20021017

If you delete *all* the text in a forwarded e-mail, and don't type anything, the
original text is sent - even though it is not shown in the Composition window

To reproduce:

1. Select an e-mail
2. Select Forward (inline)
3. In the Composition window select and delete *all* the text (don't add any new
text).
4. Address to yorself and Send
5. View the e-mail in your Inbox

Expected result: Empty e-mail body

Actual result: all the original text is there.

If you leave some of the original text, or add new text, then it works as expected.

BTW, before anyone asks why I forward e-mails with no message, I do it when an
attachment is the important part, the Subject: is self-explanatory, and the body
is just full of old headers.

Could this be related to bug 173953 which has just been fixed?

Comment 1

15 years ago
Due to various blockers I still use Mozilla/5.0 (X11; U; Linux i686; en-US;
rv:1.2b) Gecko/20021002 which does not have this bug. I hope to be able to test
with a new version later today. So indeed this would be some new effect.

pi
Keywords: regression

Comment 2

15 years ago
I was able to reproduce this bug using the same build (2002101708) on WinXP SP1;
the only exception is that I have to use "Select all" to make the bug happen. 
If I simply highlight all of the text with the mouse and hit DEL, a blank
message is sent, as expected.

Comment 3

15 years ago
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/2002101815 (+ patches
101274, 101762, 103210)

I see this problem.

pi
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All
(Assignee)

Comment 4

15 years ago
Raising severity to major. We should send exactly what the user sees!
Looks like editor not fully delete the content. Reassign to editor team
Assignee: ducarroz → kin
Severity: normal → major
Component: Composition → Editor: Core
Keywords: mailtrack
Product: MailNews → Browser
QA Contact: esther → sujay
(Reporter)

Comment 5

15 years ago
I've now tried this on my 2002-10-17-18 CVS build on my WinXP (no service pack)
machine and comfirm Frank's observation in comment 2; drag-selecting with the
mouse and deleting (hitting the DEL key) *does* send an empty message, however
selecting using Ctrl-A or Edit->Select All, then DEL shows the bug.

I also double checked on Win2K-SP2 and drag-selecting with the mouse and DEL
shows the bug (as per my original report). So there is obviously a difference
between versions of Windows.

Comment 6

15 years ago
I only see this problem in plaintext mail compose.
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.2final
(Reporter)

Comment 7

15 years ago
Oh yeah, I forgot to mention I use plain text only.

Comment 8

15 years ago
I see this in my debug build, and also in a release 2002101608 build.

Comment 9

15 years ago
As I suspected, this really is a Plaintext Message Compose bug on the MailNews
side. The problem is that |nsMsgCompose::SendMsg()| does not reset m_body if the
body is empty, so it sends the original text that was used to create the body
when the window was brought up.

Back to ducarroz.
Assignee: kin → ducarroz
Status: ASSIGNED → NEW
(Assignee)

Comment 10

15 years ago
good catch, thanks...
Status: NEW → ASSIGNED

Comment 11

15 years ago
Looks like the editorshell removal introduced this.

I changed:
    rv = m_editor->GetContentsAs(format.get(), flags, &bodyText);
    
    if (NS_SUCCEEDED(rv) && nsnull != bodyText)
to
    rv = m_editor->OutputToString(format, flags, msgBody);
    
    if (NS_SUCCEEDED(rv) && !msgBody.IsEmpty())

(m_editor used to be the editorshell, not editor.)  Seems
editorShell->GetContentsAs returns a pointer to a zero-length string, not a null
pointer.
Assignee: ducarroz → akkana
Status: ASSIGNED → NEW

Comment 12

15 years ago
Created attachment 103350 [details] [diff] [review]
Don't check msgBody.IsEmpty()

Here's the fix.  Ducarroz, would you please review?
(Reporter)

Comment 13

15 years ago
I've just noticed that Moz is no longer adding my .sig in either mail or news.
Could this be related, or should I file another bug (I can't see one filed already)?
(Assignee)

Comment 14

15 years ago
Created attachment 103353 [details] [diff] [review]
Proposed fix, v2

I rather prefers resetting the message body than going through this code with
an empty message body. Thanks akkana and jfrancis for fixing my mess.
(Assignee)

Updated

15 years ago
Attachment #103350 - Attachment is obsolete: true

Comment 15

15 years ago
Comment on attachment 103353 [details] [diff] [review]
Proposed fix, v2

good catch;r=varada
Attachment #103353 - Flags: review+
(Assignee)

Comment 16

15 years ago
reassign to myself.
Assignee: akkana → ducarroz
Comment on attachment 103353 [details] [diff] [review]
Proposed fix, v2

why do you have to cast the null ptr?  is that to silence a warning?
s/cast the null ptr/cast nsnull

Comment 19

15 years ago
parish: This bug only applies to completely empty message bodies, so it's not
what you're seeing.  Could bug 174987 be the bug you're seeing?  (Only happens
on rewrap.)  Otherwise, file a bug with more specific details.
(Assignee)

Comment 20

15 years ago
I need to cast the null pointer because there is 2 functions SetBody, one takes
a const char pointer and the other one takes a const PRUnichar *. Without the
cast, it wont compile because the compiler doesn't which fuction to use
Status: NEW → ASSIGNED
Comment on attachment 103353 [details] [diff] [review]
Proposed fix, v2

sr=sspitzer

please add a small comment about how there are two SetBody() methods, and
that's why the cast is needed.
Attachment #103353 - Flags: superreview+
(Assignee)

Comment 22

15 years ago
.
Component: Editor: Core → Composition
Product: Browser → MailNews
Whiteboard: have fix, waiting approval

Comment 23

15 years ago
Comment on attachment 103353 [details] [diff] [review]
Proposed fix, v2

a=asa for checkin to 1.2 (on behalf of drivers).
Attachment #103353 - Flags: approval+
(Assignee)

Comment 24

15 years ago
Fix checked in the trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 25

15 years ago
Using trunk build 20021029 on linux and trunk build 20021028 on winxp and trunk
build 20021025 on Mac OSX this is fixed. Verified
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.