72249 - end tag of <script> and <style> can't contain whitespace (</script > </style >)

Status: VERIFIED FIXED

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

Description

[Roland Mainz]

Mozilla build 2001-03-12-08-Mtrunk build with Sun Workshop 6 Update 2
EarlyAccess/1 running on Solaris 7 fails to load the URL
http://www.pro-sieben.de/buffyundangel/
Terminal shows:
-- snip --
WEBSHELL- = 9
WEBSHELL- = 8
nsWidget::~nsWidget() of toplevel: 42 widgets still exist.
nsWidget::~nsWidget() of toplevel: 40 widgets still exist.

Gdk-CRITICAL **: file ../../gdk/gdkwindow.c: line 716: assertion `window !=
NULL' failed.
WEBSHELL- = 7
WEBSHELL- = 6
WEBSHELL- = 5
WEBSHELL- = 4
WEBSHELL- = 3
nsWidget::~nsWidget() of toplevel: 27 widgets still exist.
Enabling Quirk StyleSheet
JavaScript error: 
http://www.pro-sieben.de/buffyundangel/ line 168: syntax error

Error loading URL http://www.pro-sieben.de/buffyundangel: 804b0002 
-- snip --

Does that mean a simple JavaScript error can block the whole page being loaded ?

Comment 1

[Boris Zbarsky [:bzbarsky]]

The problem seems to be the following:

Mozilla gets to the </SCRIPT > (note the space) tag and decides that that's not
the end of the script.  So all the content is interpreted as part of the script
and naturally none of it is rendered. Over to parser.  Also a problem on Linux
and Mac, so Platform/OS = All/All

Comment 2

[harishd]

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another 
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration. 

Comment 3

[Andreas M. "Clarence" Schneider]

*** Bug 75353 has been marked as a duplicate of this bug. ***

Comment 4

[Andreas M. "Clarence" Schneider]

2001-04-15-20, Win NT:
Script is not executed, but treated as body text. Same for </style >.

Changing summary to make this findable in bugzilla.

Attaching testcase. Note that view-source shows not the real source. It inserts
a </script> / </style> right after the start tag.

Comment 5

[Andreas M. "Clarence" Schneider]

Attachment: testcase

Comment 6

[Boris Zbarsky [:bzbarsky]]

*** Bug 78342 has been marked as a duplicate of this bug. ***

Comment 7

[Jesse Ruderman]

It looks like using </script > to end a script block is valid in HTML:
http://www.w3.org/TR/REC-html40/types.html#type-cdata

The HTML spec says the parser should treat any "</" as the end of a script 
block, but that would break a lot of scripts.  Stopping on "</script >" 
shouldn't break anything, though.

Comment 8

[Boris Zbarsky [:bzbarsky]]

*** Bug 83290 has been marked as a duplicate of this bug. ***

Comment 9

[Phil Schwartau]

*** Bug 86402 has been marked as a duplicate of this bug. ***

Comment 10

[Aaron Kaluszka]

*** Bug 87635 has been marked as a duplicate of this bug. ***

Comment 11

[Aaron Kaluszka]

adding some keywords
nscatfood and dataloss because valid HTML pages do not even render

Comment 12

[timeless]

removing dataloss, there isn't any afaik.
removing nsCatFood, you aren't netscape and that's a netscape keyword.
clobbering target milestone due to conflict w/ keywords.
tagging qawanted, can someone find out if this occurs in top100?
... we also need to find out if people every write code that might trip over 
this, i can't think of way to trip over this, so hopefully that's not a 
concern.

ah the wonders of parser patches, i already have one sitting in my tree.

fwiw, patches for problems like this should be <10 lines and really easy to 
write, someone should try to write it (helpwanted, cc me=someone).

Comment 13

[Paul McGarry]

Attachment: HTML 4.01 Transitional compliant testcase

Comment 14

[harishd]

yup, looks like this ought to be fixed. As jesse pointed out, </script > is
valid html. Setting pririty to P1 and TM to m0.9.3.

Comment 15

[Jesse Ruderman]

Wow, our parser really screws up the view-source of Paul's testcase.  It adds a
</script> tag right after the <script> (bug 83221), and doesn't show the space
in the </script > that's actually in the testcase (filed bug 87726).

Here's the HTML of the testcase:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
            "http://www.w3.org/TR/html4/loose.dtd">
<html>
<head>
<script type="text/javascript">
alert("test");
</script >
<title>test</title>
</head>
<body>
This is some text within &lt;body&gt;.
</body>
</html>

Comment 16

[David Einstein]

Attachment: Patch

Comment 17

[David Einstein]

Ok, this seems to work.  It even seems to behave sanely in the face of unclosed
comments, or if not sanely, it behaves as if the script or style tags had been
closed properly.
I set it to work in all cases, should it work in STRICT mode?

Comment 18

[timeless]

long English comments should be
/* ... 
   ...
   or something like this
   and run through a spellchecker :(
   Of course, the entire file should probably be run through the spellchecker 
   :(
*/
Too bad there's no FindCharNotInSet (there once was a FindCharInSet) ...

i'm recoding on the fly in the report because the parser code in my  tree 
doesn't compile for other reasons... (this of course means my patch isn't 
valid) [which is clear from line 1 *sigh*]

@@ -643,14 +644,29 @@
-          // do a case-insensitive comparision
+          // do a case-insensitive comparison
           nsAutoString str;
 
-          nsReadingIterator<PRUnichar> start(tempOffset), end(tempOffset);
+          nsReadingIterator<PRUnichar> penult(tempOffset);
+          /*
+             XXX  Should we be doing this in strict mode?
+             OK, there may be trailing ws behind the end tag, and we must
+             ignore it - bug 72249.
+          */
+          PRInt32   termStrWSLen(termStrLen);
/* w/ my patch there's no need for @@ -605,6 +605,7 @@
I'd suggest PRUint32 termStrWSLen because it's pretty clear that we don't want 
negative numbers, so it makes sense to have more space, unfortunately this 
isn't really practical because .Length() returns PRInt32, so termStrLen must be 
PRInt32, so using PRUint32 for termStrWSLen would cause stupid sign warnings... 
ah the wonders of foolish starting points.

Code following was wrapped to the silly dimensions of the Additional Comments 
Field.
*/
+          PRUnichar ch=*(--penult);
+          while (
+            (ch==PRUnichar('\b'))||(ch==PRUnichar('\n'))||
+            (ch==PRUnichar('\r'))||(ch==PRUnichar('\t'))||
+            (ch==PRUnichar(' '))
+          ){
+            termStrWSLen++;
+            start--;
+            ch=*(--penult);
+          }
+          nsReadingIterator<PRUnichar> end(penult);
+          end++;
/*
End is one char after penult, but it doesn't need to be live until after 
penult goes out of use.  As such, I can't see a real reason for end and penult 
to both exist, which probably means i'm missing something.  Either way, I can't 
justify moving both end and penult around in the loop.

you wrote:
 However, because of the architecture of
 this algorithm we must ignore it in a bunch of places.
 and so we keep track of the amount of ws skipped.
which i'm guessing is the reason for the additional character movement, so 
please tell me if my changes match your intentions. (I'll need to read this 
code tomorrow morning to see if I can figure out why we need to do it.)
*/
+          nsReadingIterator<PRUnichar> start(end);
/*
Start was also moving for no particular reason, if that is justified, the 
behavior is chained from end.
Unfortunately, the movement of start is the only other usage of
termStrLen (disparate from termStrWSLen) which necessitates the additional 
variable.
Actually this is wrong, some magic form of
           nsReadingIterator<PRUnichar> start(tempOffset);
           start.advance(-termStrWSLen); would probably work.
in which case we can combine the two variables.
Verification of this is left as an exercise for me tomorrow.
*/
           start.advance(-termStrLen);
 
           CopyUnicodeTo(start, end, str);
 
/* based on your repeated usage of termStrLen+termStrWSLen, i'd suggest making 
termStrWSLen(termStrLen) and using it instead. */
           if (str.EqualsIgnoreCase(aTerminalString)) {
             theTermStrPos = tempOffset;
-            theTermStrPos.advance(-termStrLen);
+            theTermStrPos.advance(-termStrWSLen);
             break;
           }
           tempOffset.advance(1);
@@ -689,7 +705,7 @@
               
               // We did not find '-->' so keep searching for terminal string.
               theCurrOffset = theTermStrPos;
-              theCurrOffset.advance(termStrLen);
+              theCurrOffset.advance(termStrWSLen);
               continue;
             }
           }
@@ -699,7 +715,7 @@
 
         aScanner.BindSubstring(mTextValue, theStartOffset, theTermStrPos);
 
-        theTermStrPos.advance(termStrLen+1);
+        theTermStrPos.advance(termStrWSLen+1);
         aScanner.SetPosition(theTermStrPos);   

/*end of very corrupt patch*/

Probably half of the style used in my code is taboo in parser and mozilla code 
in general (eg int a(0);) but that's ok, this is just conceptual code.

Comment 19

[harishd]

Attachment: Patch v1.2 [ my 2 cents ;-) ]

Comment 20

[harishd]

A pathological test case:

<html>
 <head>
  <script>
   <!--
    document.write("<pre>hi there");
   //-->
  </script
 </head>
 <body>
 </body>
</html>

Comment 21

[harishd]

Attachment: patch v1.3 [ fixes viewsource problem too ]

Comment 22

[Boris Zbarsky [:bzbarsky]]

*** Bug 87885 has been marked as a duplicate of this bug. ***

Comment 23

[David Einstein]

*** Bug 43513 has been marked as a duplicate of this bug. ***

Comment 24

[David Einstein]

Attachment: An even more pathological testcase

Comment 25

[David Einstein]

Attachment: Patch 1.3.1

Comment 26

[David Einstein]

Ok, I'm not trying to be annoying.
I think that a missing > at the end of a tag is just that a missing >.  I know
that nobody in their right minds edits html by hand anymore, but there a lot
more web pages out there than there are right minds.
This causes view source to display modified source in the case of the mising end
tag, but does do a better job of syntax highlighting.
It is possible that bug 43820 should get duped here.

Comment 27

[harishd]

David: Nav 4.x and IE do not display the text between </script and </head> and
therefore we shouldn't either.

Comment 28

[Heikki Toivonen (remove -bugzilla when emailing directly)]

David: give me an HTML editor that does not suck, is easy to use, works on all
platforms, writes precisely the markup I want and is free, and I'll switch to
that (I have always edited HTML by hand;)

Comment 29

[David Einstein]

OK, I can see the logic of following the IE and NS4 behavior.
(I should have said "entirely by hand". If you're not using either a validator,
and probably a validator integrated into your text editor at that, or something
like 'HTML tidy', and you manage to produce bug free HTML, you have my undying
admiration.)

Comment 30

[Heikki Toivonen (remove -bugzilla when emailing directly)]

nsBranch ok, affects existing pages. Get this on the trunk and verified, and we
can think about branch then.

Comment 31

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comments:

1. There might be a case insensitive pattern matching for iterators (minor
optimization).

2. What if there was <script> ... </scriptno > (i.e. user invented tag name that
started with like "script"?

If 2. works ok, I am fine with the patch (v1.3).

Comment 32

[Boris Zbarsky [:bzbarsky]]

To address 1:

Compare(str1, str2, nsCaseInsensitiveStringComparator())

In this case:

if(Compare(theTerminalString, Substring(start, end),
        nsCaseInsensitiveStringComparator()) == 0) {
 // we have a match
}

Comment 33

[harishd]

Attachment: Patch v1.4 [ addressing heikki's concern ]

Comment 34

[Heikki Toivonen (remove -bugzilla when emailing directly)]

With the check-for-end added on line

+      if (CaseInsensitiveFindInReadable(theTerminalString,start,end) &&

r=heikki (you seemed to have that in your tree already).

Comment 35

[harishd]

Attachment: patch v1.5

Comment 36

[vidur (gone)]

sr=vidur assuming thorough testing of the various codepaths and the top 100.

Comment 37

[vidur (gone)]

Fix checked in to the TRUNK.

Comment 38

[harishd]

Previous comment was from me not vidur's :-)

Comment 39

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Verified FIXED on today's trunk builds on Win2k and Linux.

Comment 40

[Nisheeth Ranjan]

After discussing this further with Harish, we both think that it is too late for
this change to go into the branch.  It will be hard to justify taking this fix
to the PDT.  Removing nsBranch, vtrunk, qawanted, review keywords.

Marking fixed because this is fixed on the trunk.

Comment 41

[Heikki Toivonen (remove -bugzilla when emailing directly)]

And it was also verified so marking as such.