Closed Bug 133044 Opened 23 years ago Closed 23 years ago

Single line HTML comments get mangled in textarea

Categories

(Core :: DOM: HTML Parser, defect, P1)

Product:
Core
Component:
DOM: HTML Parser
Platform:
x86
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: bugzilla, Assigned: hjtoi-bugzilla)

References

Details

(4 keywords, Whiteboard: [ADT1 RTM][Fixed on the trunk and branch 05/22 ],custrtm-)

Attachments

(4 files, 2 obsolete files)

Simple Test Case
183 bytes, text/html
Details
testcase
308 bytes, text/html
Details
patch v1.1
9.73 KB, patch
hjtoi-bugzilla
: review+
jst
: superreview+
brendan
: approval+
Details | Diff | Splinter Review
testcase: view source, save as complete
323 bytes, text/html
Details
When editing HTML in a text area, single line comments have the opening and closing comment tags removed. <!-- this is fine, cuz it's multi-line --> <!-- THISLLBREAK --> The latter turns into: THISLLBREAK This has the potential for pretty big data corruption.
Sorry Build ID: 2002032306
not critical
Severity: critical → normal
You should escape the "<" and ">" because as the HTML spec says, "comments are markup". Over to parser.
Assignee: rods → harishd
Status: UNCONFIRMED → NEW
Component: HTML Form Controls → Parser
Ever confirmed: true
QA Contact: madhur → moied
Probably related to bug 105973.
The problem is that the parser should be ignoring any HTML within the <textarea></textarea>. This is critical as it can result in dataloss! This will make Mozilla unsafe to use for managing webpages, from a webpage. I am attaching a more complete test case. In my experience, multi-line doesn't make a difference (build 2002032803).
Attached file Simple Test Case —
Possible dupe of bug 64799 or bug 127043 particularly the bits to do with <SCRIPT> tags within TEXTAREAs from those bugs.
I still don't find any reference in the HTML 4.01 Specification that says there can't be html within a <textarea> although I did find it in the 3.2 Spec and I suppose that it probably does make sense. However, this is not consistent with any other browsers and WILL RESULT IN DATALOSS in many applications. Therefore, IMHO, it should only be enforced in Strict mode.
Marking "compat". The only tags in which unescaped HTML is not parsed are those with declared content CDATA, such as <script> and <style>. <textarea> is not one of them.
Keywords: compat, testcase
If you are writing for one spec and put it on a different doctype you are gonna get problems.. especially with depreciated tags in newer versions of standards.
Heikki, could you please take a look at this? Thanx.
Assignee: harishd → heikki
*** Bug 135364 has been marked as a duplicate of this bug. ***
Hmm... seems I broke this with my comment optimization :(
Status: NEW → ASSIGNED
Keywords: dataloss, nsbeta1+, regression
OS: Linux → All
Priority: -- → P1
Target Milestone: --- → mozilla1.0
Whiteboard: [ADT2]
*** Bug 138494 has been marked as a duplicate of this bug. ***
I get something similar. Go to http://chuckr.bravepages.com for an example. Sometimes the page is rendered and missing the whole table, a single line comment seems to mess it up. Other times it works. Using Mozilla 1.0rc1. Here is a partial code sample as viewed from Mozilla 1.0rc1. Everything after the BODY tag seems to be commented out, until you get to the end of the table. <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN"> <html> <head> <title>Freezone freeware</title> <meta name="keywords" content="freeware freezone computer software free Unrealed Unrealed2"> <meta NAME="description" CONTENT="Freezone Freeware, 600+ freeware programs"> <meta NAME="author" CONTENT="croberts@gilsongfx.com"> <body bgcolor="#c0c0c0" TOPMARGIN=0 LEFTMARGIN=5 MARGINHEIGHT=0 MARGINWIDTH=5> <!--></noscript></div></xmp></style> <center> <!partner is bravepages 192.168.100.13> <SCRIPT SRC='http://www.bravenet.com/jsbanner.php?size=666'></SCRIPT><SCRIPT SRC='http://www.bravenet.com/jsbanner.php?size=999'></SCRIPT> </center> <table border=1> <tr> <td width=20% bgcolor=lightblue valign=top> <p> <center> <a href="http://www.cdrinfo.com"><img border="0" src="http://www.cdrinfo.com/images/button6.gif" alt="Click here to Visit CDR-Info!"></a> First view the page with IE, and view source in IE. Now do the same in Mozilla. You'll get different results. croberts@gilsongfx.com
I support Stuart Johnston in that it's vital that HTML should not be parsed with &lt;textarea&gt; tags. IMHO this should extend beyond HTML comments, and include HTML characters (the ones that begin with "&" and end with ";", and have "nbsp" or "lt" etc. in the middle).
This is a showstopper for our use of the browser. Can someone specify what we need to do to proceed? If unescaped HTML inside a TEXTAREA is actually in conformance with the HTML spec, do we need to approach W3C to discuss the issue and possibly request a modification the spec?
*** Bug 144013 has been marked as a duplicate of this bug. ***
Whiteboard: [ADT2] → [ADT1 RTM]
*** Bug 145238 has been marked as a duplicate of this bug. ***
Blocks: 143200
Attached patch WIP, does not work (obsolete) — — Splinter Review
This shows the idea I am after. New comment parsing has been implemented for strict mode (not really new, just create comment string between '<' and '>' and hold iters into the string for actual data start and end). Will do quirks after I see this works in practice. Create comment node with that string and iters. To minimize changes, don't change GetStringValue() member, but expose the new stuff with a new method that gets the iters (or possibly substring). Most callers of GetStringValue are fine with just that value (for example view source and serialization), but the data that is exposed to DOM must be the substring between iters. I think we'll need a new interface, nsINSDOMComment or something, that we use internally for comments instead of nsICOMComment. Additional twist is if someone edits the comment data via script: we must create new data, but retain the old delimiters (for serialization).
In my opinion, it would be sufficient if there was a clear rule like "HTML has to be quoted in textareas". At the moment, it is neither parsed correctly nor displayed as is. If it was parsed correctly, then a comment would be removed completely, but in the current situation, the parser just removes the opening and closing tags. On the other hand, I would like it if textareas just would allow unparsed content, simply for practical reasons. I run a web form where people can modify XML documents (which are then evaluated) and it would be rather inconvenient to quote every brace and ampersand. P.S. Mozilla only began behaving this way in 0.9.9 (or RC1?) ...
That won't do. There are several things that go wrong, all of which were caused by bug 110544. The things that come to mind right now are: * Comments in textarea elements * View source (noticeable with bad comments) * Serialization My fix will fix all of them. To put it simply, the problem is that we need to retain information about how the comments started and ended in order to make round tripping possible. All of the above problems are caused by this basic problem, or they are variations of it. Currently the code only stores what is between the comment delimiters.
Turns out the full fix seems to be pretty big, with some non-trivial changes. While I am working on that, harishd will be working on making a smaller band-aid fix to fix just the textarea part.
Attached patch patch v1.0 (obsolete) — — Splinter Review
This patch should fix the problem with comments in textarea and in viewsource. I've added an extra member to the comment token that stores the complete comment. AFAIK, the only overhead is the extra member there is no extra copy ( I think :-/ ) because the member variable is a slidingsubstring. FYI: The patch needs clean up and extensive testing.
is backing out bug 110544 an option for rc3/1.0? and we could land the optimization and the bigger clean up patches for 1.01?
Backing out is not a good option in my opinion: in addition to some performance, the fix did fix some pages we had never gotten right, including pages on top100 sites, and it made us more standards compliant and more tolerant to really bad comments. Also there have been quite a bit of changes on top of these changes as well. I think harishd's patch would be good enough for RC3/1.0, need to confirm with a bit of testing tomorrow.
Attached file testcase —
8 different comments in textarea
I can understand how the patch fixes view source, and that looks good. However, I don't know how this would fix textareas. Maybe something was missing from the patch?
>I can understand how the patch fixes view source, and that looks good. However, >I don't know how this would fix textareas. Maybe something was missing from the >patch? Textarea content is collected in CNavDTD::CollectSkippedContent() by calling AppendSourceTo on the token ( comment in our case ). CCommentToken::AppendSourceTo will provide us with the comment as seen on the source.
Attached patch patch v1.1 — — Splinter Review
This patch in addition to fixing the stated problem it also fixes a problem, pertaining to comments, in the viewsource. I've replaced the nsString member, in CCommentToken, with nsSlidingSubstring. This would avoid one extra copy! Heikki: I have also removed a hack that was in place to avoid a hang ( bug 112943 ) since using nsSlidingSubstring avoids the problem.
Attachment #84404 - Attachment is obsolete: true
Attachment #84575 - Attachment is obsolete: true
Comment on attachment 84675 [details] [diff] [review] patch v1.1 >+ mNewlineCount = !(aFlag & NS_IPARSER_FLAG_VIEW_SOURCE) ? mComment.CountChar(kNewLine) : -1; We should count newlines from mCommentDecl member. With that, r=heikki.
Attachment #84675 - Flags: review+
>We should count newlines from mCommentDecl member. Yes it should be. Made that change on my local tree.
Comment on attachment 84675 [details] [diff] [review] patch v1.1 sr=jst
Attachment #84675 - Flags: superreview+
Oh, and get rid of the old unused member variable in CCommentToken :)
This testcase shows the minor dataloss problem that still remains after this fix. The issue is that if you have malformed comments and you save page as complete, we always save with the correct comment open/close so your bad markup probably changed to better.
Comment on attachment 84675 [details] [diff] [review] patch v1.1 with all of heikki's comments attended to, a=brendan@mozilla.org, please get this in tonight. /be
Attachment #84675 - Flags: approval+
adding adt1.0.0+ for checkin to the 1.0 branch.
Keywords: adt1.0.0 → adt1.0.0+
Fix landed ( 05/22 )on the trunk and branch.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Whiteboard: [ADT1 RTM] → [ADT1 RTM][Fixed on the trunk and branch 05/22 ]
*** Bug 146576 has been marked as a duplicate of this bug. ***
Verified fixed with trunk and branch build 20020524 on winxp
Status: RESOLVED → VERIFIED
Whiteboard: [ADT1 RTM][Fixed on the trunk and branch 05/22 ] → [ADT1 RTM][Fixed on the trunk and branch 05/22 ],custrtm-
Keywords: verified1.0.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: