Closed Bug 144257 Opened 23 years ago Closed 23 years ago

reply a html attatchment, 1/2 of content is cut off

Categories

(MailNews Core :: Attachments, defect)

Product:
MailNews Core
Component:
Attachments
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: cathleennscp, Assigned: bugzilla)

References

Details

(Whiteboard: [adt2 rtm] Have fix)

Attachments

(4 files)

macia's original email
77.42 KB, image/jpeg
Details
replied email
57.86 KB, image/jpeg
Details
html source
3.58 KB, patch
Details | Diff | Splinter Review
Proposed fix, v1
1.28 KB, patch
vparthas
: review+
mscott
: superreview+
scc
: approval+
Details | Diff | Splinter Review
using friday's commercial build, 5/10/2002. forwarded marcia's techtalk message to all, message gets cut off in the middle. attatchment coming up.
Attached image macia's original email —
snap shot of original email
Attached image replied email —
snap shot of replied email, notice the attachment seemed cut off. Note: 1) view source on the email does show html code has full content 2) when replying the email, compose window does display attchment in full 3) when viewing the replied email, <!doctype html public "-//w3c//dtd html 4.0 transitional//en"> became h!doctype html public "-//w3c//dtd html 4.0 transitional//en">
Keywords: nsbeta1
Attached patch html source — — Splinter Review
html page source.
Using the Beta Candidate build from Sunday night: the forward is not chopping off the message but reply is.
Blocks: 143047
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [adt1]
reassigning to ducarroz.
Assignee: mscott → ducarroz
Whiteboard: [adt1] → [adt2 rtm]
accepting and looking into it...
Status: NEW → ASSIGNED
cathleen, did you reply or forward the original message. Looks like reply to me! Please clarify
Using a recent Mozilla trunk debug build on Windows, I can forward Marcia's message without problem. However, I can reproduce the truncated HTML when replying (after send). The HTML containd a part which has been out commented using "<!--" and "-->". But in the data received, the "-->" get replaced by "--&gt;" and an extra "-->" has been added at the end of the document! My first guess is that it's an editor problem!
I can definitely reproduce with reply. I thought I forwarded the message, but I can't reproduce it now that I updated to more recent build. Change summary to reply only.
Summary: forward/reply a html attatchment, 1/2 of content is cut off → reply a html attatchment, 1/2 of content is cut off
This is not an editor problem but rather a problem with mozTXTToHTMLConv. We call ScanHTML to convert special characters into entities but it convert the end-of-comment tag as well!
Attached patch Proposed fix, v1 — — Splinter Review
When we scan the HTML for entitie to encode, we need to skip over out-commented code.
Whiteboard: [adt2 rtm] → [adt2 rtm] Have fix
Comment on attachment 83589 [details] [diff] [review] Proposed fix, v1 I am not sure about commenting the rest of the code when we cannot find the end tag for the comments but I guess layout does the same thing too. Patch looks good to me. R=varada
Attachment #83589 - Flags: review+
Comment on attachment 83589 [details] [diff] [review] Proposed fix, v1 Hmm I wonder if we should use a macro for the phrase "-->" and the length of that phrase. Just so it's easier to maintain later on.
That would make sense if: 1) we were using that string several time 2) we would also use a macro/define for the opening tag (<!--) 3) this marker could changes in the future Also, to be consistant, we would have to to the same with the marker </a>. I don't fell that using a macro would give any advantage here! I pesonnaly avoid using macro as they make harder to debug and sometime to read the code.
Comment on attachment 83589 [details] [diff] [review] Proposed fix, v1 sr=mscott
Attachment #83589 - Flags: superreview+
Fix checked in the trunk
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Keywords: adt1.0.0
Keywords: approval
Whiteboard: [adt2 rtm] Have fix → [adt2 rtm] Have fix [Needs a= & ADT+]
trix, when you get a chance, could you verify this on the trunk?
Would this be good to get into the 1.0 branch today, for rc3? Drivers are looking at it, want advice. Why would we not fix this for 1.0, but then take it for rtm? /be
Blocks: 143200
verified on trunk build 2002052008
Status: RESOLVED → VERIFIED
An issue with this patch (and the function in general): The code for mozTXTToHTMLConv::ScanHTML uses aInString[i+1] (and i+2 or i+3). Nothing checks that i+1 (2,3) is less than lengthOfInString. This could cause a segfault on some architectures (or if it's not null-terminated, it could mistakenly match). This exists in spots other than the patch too.
As long the string is null terminated we are fine, we will never read pass the buffer. The Real question is: Could the string be not null terminated?
From nsStr.h: ASSUMPTIONS: 1. nsStrings and nsAutoString are always null terminated. However, since it maintains a length byte, you can store NULL's inside the string. Just be careful passing such buffers to 3rd party API's that assume that NULL always terminate the buffer. Therefore, this fix is safe and will not cause a crash. Sure I could check for the length for every if statment but this is a waiste of (cpu) time especially when it's not needed. Also, as all we do is reading the memory, there is no risk of buffer overrun.
Since we guarantee there's a null at the end, the order of the tests will guarantee that we never go past the null. Thanks.
Comment on attachment 83589 [details] [diff] [review] Proposed fix, v1 a=scc (on behalf of drivers) for checkin to the mozilla1.0 branch
Attachment #83589 - Flags: approval+
adding adt1.0.0+
Keywords: adt1.0.0 → adt1.0.0+
Keywords: fixed1.0.0
Whiteboard: [adt2 rtm] Have fix [Needs a= & ADT+] → [adt2 rtm] Have fix
Fix checked in the branch
verified on branch build 2002061108 changing keyword fixed1.0.0 to verified1.0.0
Keywords: fixed1.0.0 → verified1.0.0
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: