Closed Bug 2749 Opened 26 years ago Closed 23 years ago

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

Categories

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

Product:
Core
Component:
DOM: HTML Parser
Platform:
x86
Windows 98
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9

People

(Reporter: ian, Assigned: harishd)

References

(
URL
)

Details

(Keywords: regression, testcase, Whiteboard: [19990901])

Attachments

(4 files)

Testcase for quirks mode coments
262 bytes, text/html
Details
Testcase
192 bytes, text/html
Details
Yet another testcase
509 bytes, text/html
Details
Proposed patch
465 bytes, patch
Details | Diff | Splinter Review 
Comment delimiters are "--" while inside tags.

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

On the test page quoted, all is explained.
fixed the URL to what was intended
Eek! Thanks.
Status: NEW → ASSIGNED
Whiteboard: Test: Need to update test page - ieh
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.
Whiteboard: Test: Need to update test page - ieh
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.
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! <!-- -- -->
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!
Setting all current Open Critical and Major to M3
Status: ASSIGNED → RESOLVED
Closed: 26 years ago
Resolution: --- → FIXED
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.
QA Contact: 4141
Will not verify until strict parsing mode enabled.
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! <!-- -- -->
Whiteboard: Verification is awaiting strict DTD parsing mode.
QA Contact: 4141 → 3847
Query: Is "->" supposed to end comments in NavQuirks mode?
Whiteboard: Verification is awaiting strict DTD parsing mode. → 6/18 Verification is awaiting strict DTD parsing mode.
Whiteboard: 6/18 Verification is awaiting strict DTD parsing mode. → 7/9 Reporter awaiting developer response.
Reporter awaiting developer response.
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.
Status: RESOLVED → VERIFIED
verified
1999-07-09-08-M8
Status: VERIFIED → REOPENED
Depends on: 1312
Whiteboard: 7/9 Reporter awaiting developer response. → waiting for non-quirks parsing mode
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.)
Status: REOPENED → RESOLVED
Closed: 26 years ago → 25 years ago
Status: RESOLVED → REOPENED
This was verified per the developer.
Clearing "Resolved Fixed" and re-opening bug.
Resolution: FIXED → ---
Clearing Fixed resolution due to reopen.
Target Milestone: M3 → M9
Moving from M3 to M9 milestone since reopen.  rickg, please set to later
milestone if more appropriate.
Status: REOPENED → ASSIGNED
Target Milestone: M9 → M12
Status: ASSIGNED → RESOLVED
Closed: 25 years ago → 25 years ago
Resolution: --- → REMIND
I'll move this to REMIND, since we're working per Nav.
Whiteboard: waiting for non-quirks parsing mode → [19990901] Resolution: remind
Status: RESOLVED → REOPENED
Whiteboard: [19990901] Resolution: remind → [19990901]
Resolution: REMIND → ---
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: rickg → harishd
Status: REOPENED → NEW
*** Bug 12025 has been marked as a duplicate of this bug. ***
Status: NEW → RESOLVED
Closed: 25 years ago → 25 years ago
Resolution: --- → FIXED
Strict comment handling has been hooked up to the noquirks.
Status: RESOLVED → REOPENED
Even if quirks mode, "->" should not terminate comments
because "->" do not end comments both on Nav4 and on IE.
reopen.
Resolution: FIXED → ---
Clearing FIXED resolution due to reopen.
Status: REOPENED → ASSIGNED
Target Milestone: M12 → M11
Status: ASSIGNED → RESOLVED
Closed: 25 years ago → 25 years ago
Resolution: --- → FIXED
Checked in a fix..
Status: RESOLVED → VERIFIED
Verified fixed.
Status: VERIFIED → REOPENED
Oops. I found standards mode comment handling is still incorrect.
Reopening.
Attached file Testcase — 

    
  
Status: REOPENED → ASSIGNED

    
  
Resolution: FIXED → ---

    
  
Status: ASSIGNED → RESOLVED
Closed: 25 years ago → 25 years ago
Resolution: --- → FIXED

    
    

    
  
Good catch.

Tweaked strict comment handling.  Marking FIXED.
Status: RESOLVED → REOPENED

    
    

    
  
Still incorrect using 1999102908 build. Reopening.

    
    

    
  
The fix is already in.. which build did you check?

    
    

    
  
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.

    
  
Status: REOPENED → ASSIGNED

    
  
Target Milestone: M11 → M12

    
  
Resolution: FIXED → ---

    
    

    
  
Clearing FIXED resolution due to reopen of this bug.

    
    

    
  
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

    
  
Status: ASSIGNED → RESOLVED
Closed: 25 years ago → 25 years ago
Resolution: --- → FIXED

    
    

    
  
Marking FIXED.

    
  
Status: RESOLVED → VERIFIED

    
    

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

    
  
Depends on: 34662

    
    

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

    
  
Target Milestone: --- → mozilla0.9

    
    

    
  
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?

    
    

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

    
    

    
  
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

    
    

    
  
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

    
    

    
  
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.

    
    

    
  
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

    
    

    
  
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

    
    

    
  
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.)

    
    

    
  
Enabled strict comment parsing. Marking FIXED.
Status: ASSIGNED → RESOLVED
Closed: 25 years ago → 23 years ago
Resolution: --- → FIXED

    
    

    
  
Horraahh! VERIFIED FIXED.
Status: RESOLVED → VERIFIED

    
    

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: