Comments are incorrectly parsed: <!-- -- --> inside comment! <!-- -- -->

VERIFIED FIXED in mozilla0.9

Status

()

Core
HTML: Parser
P2
normal
VERIFIED FIXED
19 years ago
8 years ago

People

(Reporter: Hixie (not reading bugmail), Assigned: harishd)

Tracking

({regression, testcase})

Trunk
mozilla0.9
x86
Windows 98
regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [19990901], URL)

Attachments

(4 attachments)

(Reporter)

Description

19 years ago
Comment delimiters are "--" while inside tags.

Thus: <!-- in --  -- in --  -- in -->
where "in" shows what is commented.

On the test page quoted, all is explained.

Updated

19 years ago

Comment 1

19 years ago
fixed the URL to what was intended
(Reporter)

Updated

19 years ago
(Reporter)

Comment 2

19 years ago
Eek! Thanks.

Updated

19 years ago
Status: NEW → ASSIGNED
(Reporter)

Updated

19 years ago
Whiteboard: Test: Need to update test page - ieh
(Reporter)

Comment 3

19 years ago
Incidentally, the test page doesn't (yet) test this:

 <A name=abc -- href="comment" --> </A>

That is, "--" delimiters also delimit comments inside element tags. Thus the
above should be parsed as:

 <A name=abc > </A>

...and the following:

 <A -- href="hello.my. -- name=fred -- /world.com"--> </A>

...should be parsed as:

 <A  name=fred > </A>

...and the following:

 <DEL datetime="1999-01-30T1325Z" --> Never mind. --> </DEL>

...should be parsed as:

 <DEL datetime="1999-01-30T1325Z" > </DEL>

Eventually I'll add these to the test page quoted above.
Are you sure about those examples above, Ian?  I think the -- only applies when
one is within SGML processing instructions, i.e, <! ... >, not any old tags.  I
don't have a copy of any SGML standard (I think it's on paper only!), so I'm
not sure.  But nsgmls (via http://www.htmlhelp.com/tools/validator/) agrees
with me.
(Reporter)

Updated

19 years ago
Whiteboard: Test: Need to update test page - ieh
(Reporter)

Comment 5

19 years ago
If nsgmls says I'm wrong, then I'm wrong.

The SGML spec is indeed paper only, I was basing my assertion on what
I could remember of a tutorial I read a few months back. Please therefore
disregard anything I previously said about comments in any old tags.
(Reporter)

Updated

19 years ago
Severity: minor → major
Priority: P4 → P2
Summary: Comments are incorrectly parsed: <!-- -- --> inside comment! <!-- -- --> → Comment parsing VERY broken: -> should not terminate comment!
Whiteboard: Less Serious Summary: Comments are incorrectly parsed: <!-- -- --> inside comment! <!-- -- -->
(Reporter)

Comment 6

19 years ago
See the end of the quoted test page:
  http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/comments-evil.html
Basically, "->" is terminating comments, instead of "--".

This is causing pages to break, for example:
  http://www.bath.ac.uk/%7Esu0bufs/
...has its comment visible at the top!

Comment 7

19 years ago
Setting all current Open Critical and Major to M3

Updated

18 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 8

18 years ago
Fixed by update to comment parsing. We should now be handling common practice of
comments correctly now, but still not to spec. That won't get enabled until we
actually have a strict dtd.

Updated

18 years ago
QA Contact: 4141
(Reporter)

Comment 9

18 years ago
Will not verify until strict parsing mode enabled.
(Reporter)

Updated

18 years ago
Hardware: Other → PC
Summary: Comment parsing VERY broken: -> should not terminate comment! → Comments are incorrectly parsed: <!-- -- --> inside comment! <!-- -- -->
Whiteboard: Less Serious Summary: Comments are incorrectly parsed: <!-- -- --> inside comment! <!-- -- -->
(Reporter)

Updated

18 years ago
Whiteboard: Verification is awaiting strict DTD parsing mode.

Updated

18 years ago
QA Contact: 4141 → 3847
(Reporter)

Comment 10

18 years ago
Query: Is "->" supposed to end comments in NavQuirks mode?

Updated

18 years ago
Whiteboard: Verification is awaiting strict DTD parsing mode. → 6/18 Verification is awaiting strict DTD parsing mode.

Updated

18 years ago
Whiteboard: 6/18 Verification is awaiting strict DTD parsing mode. → 7/9 Reporter awaiting developer response.

Comment 11

18 years ago
Reporter awaiting developer response.

Comment 12

18 years ago
We run in quirks mode by default -- and so yes, -> can close a comment. In
non-quirks mode you will have to use correctly formed comments.

Updated

18 years ago
Status: RESOLVED → VERIFIED

Comment 13

18 years ago
verified
1999-07-09-08-M8
(Reporter)

Updated

18 years ago
Status: VERIFIED → REOPENED
Depends on: 1312
Whiteboard: 7/9 Reporter awaiting developer response. → waiting for non-quirks parsing mode
(Reporter)

Comment 14

18 years ago
janc: Strict mode parsing is not yet enabled. This bug cannot be verified until
it is. I am adding a dependency to indicate this and removing the verified
notation on this bug for now.

In any case, how exactly did you verify that the bug was fixed? Viewer still
fails two out of three of the tests on the test page for this bug. (It fails
them because it parses in quirk mode.)
(Reporter)

Updated

18 years ago
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago18 years ago

Updated

18 years ago
Status: RESOLVED → REOPENED

Comment 15

18 years ago
This was verified per the developer.
Clearing "Resolved Fixed" and re-opening bug.

Updated

18 years ago
Resolution: FIXED → ---

Comment 16

18 years ago
Clearing Fixed resolution due to reopen.

Updated

18 years ago
Target Milestone: M3 → M9

Comment 17

18 years ago
Moving from M3 to M9 milestone since reopen.  rickg, please set to later
milestone if more appropriate.

Updated

18 years ago
Status: REOPENED → ASSIGNED
Target Milestone: M9 → M12

Updated

18 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → REMIND

Comment 18

18 years ago
I'll move this to REMIND, since we're working per Nav.

Updated

18 years ago
Whiteboard: waiting for non-quirks parsing mode → [19990901] Resolution: remind
(Reporter)

Updated

18 years ago
Status: RESOLVED → REOPENED
Whiteboard: [19990901] Resolution: remind → [19990901]
(Reporter)

Updated

18 years ago
Resolution: REMIND → ---
(Reporter)

Comment 19

18 years ago
Ok, DOCTYPE-linked standard mode is now hooked up. We still fail these tests,
even though the test page's DOCTYPE should trigger standard mode.

Reopening.
(Assignee)

Updated

18 years ago
Assignee: rickg → harishd
Status: REOPENED → NEW
*** Bug 12025 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

18 years ago
Status: NEW → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED
(Assignee)

Comment 21

18 years ago
Strict comment handling has been hooked up to the noquirks.

Updated

18 years ago
Status: RESOLVED → REOPENED
Even if quirks mode, "->" should not terminate comments
because "->" do not end comments both on Nav4 and on IE.
reopen.
Created attachment 2027 [details]
Testcase for quirks mode coments

Updated

18 years ago
Resolution: FIXED → ---

Comment 24

18 years ago
Clearing FIXED resolution due to reopen.
(Assignee)

Updated

18 years ago
Status: REOPENED → ASSIGNED
Target Milestone: M12 → M11
(Assignee)

Updated

18 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED
(Assignee)

Comment 25

18 years ago
Checked in a fix..

Updated

18 years ago
Status: RESOLVED → VERIFIED
Verified fixed.

Updated

18 years ago
Status: VERIFIED → REOPENED
Oops. I found standards mode comment handling is still incorrect.
Reopening.
Created attachment 2396 [details]
Testcase
(Assignee)

Updated

18 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Updated

18 years ago
Resolution: FIXED → ---
(Assignee)

Updated

18 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED
(Assignee)

Comment 29

18 years ago
Good catch.

Tweaked strict comment handling.  Marking FIXED.

Updated

18 years ago
Status: RESOLVED → REOPENED
Still incorrect using 1999102908 build. Reopening.
Created attachment 2503 [details]
Yet another testcase
Created attachment 2504 [details] [diff] [review]
Proposed patch
(Assignee)

Comment 33

18 years ago
The fix is already in.. which build did you check?
(Assignee)

Comment 34

18 years ago
Oops, I'm sorry...I see what you're saying.  When I tweaked the strict comment
I, some how, forgot to comment out the line that you'd proposed...my bad :(

Will checkin the change Monday.  Thanx for catching it.
(Assignee)

Updated

18 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Updated

18 years ago
Target Milestone: M11 → M12

Updated

18 years ago
Resolution: FIXED → ---

Comment 35

18 years ago
Clearing FIXED resolution due to reopen of this bug.
(Reporter)

Comment 36

18 years ago
Note. The eviltest has been updated to contain all permutations mentioned
herein. Thanks to VYV03354@nifty.ne.jp for spotting them. :-)

   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/comments-evil.html
(Assignee)

Updated

18 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED
(Assignee)

Comment 37

18 years ago
Marking FIXED.
(Reporter)

Updated

18 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Comment 38

18 years ago
All tests on the evil test (a superset of the attachements) pass correctly.
Marking verified.
(Reporter)

Updated

17 years ago
Depends on: 34662
(Reporter)

Comment 39

16 years ago
Reopening.
http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/comments-evil.html fails again.
Blocks: 7954
Status: VERIFIED → REOPENED
Keywords: regression
QA Contact: janc → ian
Resolution: FIXED → ---
Keywords: mozilla0.9
Target Milestone: M12 → ---
(Assignee)

Updated

16 years ago
Target Milestone: --- → mozilla0.9

Comment 40

16 years ago
Ian, would you try to identify the time frame in which the regression was 
reintroduced?  If it appeared recently it might be much easier to find and fix. 
 
How critical is this?  It says 'major' severity, it that accurate?
(Reporter)

Comment 41

16 years ago
I would bet that this regressed when we turned off the Strict HTML parser.
->normal; this isn't major any more.
Severity: major → normal
(Assignee)

Comment 42

16 years ago
Ian, you're right. It did regress when the strict DTD got turned off. It's still 
possible to turn on strict comment parsing but I'm not sure if it's worth doing 
so because we now display exactly the way Nav4.x and IE do. Ian, what is your 
take on this?
Status: REOPENED → ASSIGNED
(Reporter)

Comment 43

16 years ago
If it is easy to turn on in standards mode, then we should. How easy is it?

I would say it's worth fixing, but not necessarily for mozilla0.9. If it is not
an easy fix, then I propose targetting mozilla1.0.
Keywords: mozilla1.0
(Assignee)

Comment 44

16 years ago
Ian, refer to bug 53011. How do we handle cases like this ( as in bug 53011 ) if 
the strict comment parsing is turned on?  evangelize?

Btw, it's a onliner to turn on strict comment parsing.
(Reporter)

Comment 45

16 years ago
Yes, we would evangelise. This is just like we do with all the other "bugs" that
pages that trigger standard mode uncover, such as the inline box model issues.

Thankfully, very few broken pages trigger standard mode.

If it's a one line fix, I would recommend we do it for 0.9.
Keywords: mozilla1.0 → testcase

Comment 46

16 years ago
I agree with Ian, if you can get this one in that would be a good thing. I was 
working with another bug and found this to happen:

original reduced test:
<html>
<head>
  <title>test</title>
<META NAME=GENERATOR CONTENT="Microsoft FrontPage 4.0">
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
</head>

commented it out, but placed the close comment as a single dash:
<html>
<head>
  <title>test</title>
<!--
<META NAME=GENERATOR CONTENT="Microsoft FrontPage 4.0">
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
->
</head>

and when I dumped the html, this is what the content was:
<head>
<title>test</title>
<!--
<META NAME=GENERATOR CONTENT="Microsoft FrontPage 4.0">-->
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
</head>
<body>
-&gt; 

Note the closing comment is placed after the first meta element -- why is that? 
In addition, note that the -> is escaped and within the body

Not sure if the resolution of this bug will resolve that -- Ian, is this similar 
to issues that you encountered?

Should this be a new bug? If so, I will gladly open one up
(Reporter)

Comment 47

16 years ago
beppe: I'm not sure that's a bug, but file it or mail me directly so that we can
take the discussion out of this bug. (per HTML/SGML/XML, "->" is not a comment
delimiter, so it shouldn't close the comment, IMHO; hence why I say this is 
correct. I dunno, I might be wrong.)
(Assignee)

Comment 48

16 years ago
Enabled strict comment parsing. Marking FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago16 years ago
Resolution: --- → FIXED
(Reporter)

Comment 49

16 years ago
Horraahh! VERIFIED FIXED.
Status: RESOLVED → VERIFIED

Comment 50

8 years ago
I still get it with version 3.5 for Mac see http://alix.guillard.fr/index2.php
(In reply to comment #50)
> I still get it with version 3.5 for Mac see http://alix.guillard.fr/index2.php

You're actually complaining about the opposite, i.e., bug 214476.  (This bug fixed us to behave as SGML specifies, or something like that, but that breaks your page.  Bug 214476 is a request to undo that, which we will when we switch to our HTML5 parser.)

It's an interesting case, since it involves commenting out IDN in the xn-- form, though.
You need to log in before you can comment on or make changes to this bug.