Unclosed script data should not be parsed as HTML

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
HTML: Parser
P4
normal
RESOLVED FIXED
12 years ago
6 years ago

People

(Reporter: Jing Lim, Assigned: mrbkap)

Tracking

({dev-doc-complete, testcase})

Trunk
mozilla1.9alpha1
x86
Windows XP
dev-doc-complete, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3

If a web server that is serving a page shuts down, or if the http connection
gets reset, a </script> may not be sent to the browser.

Firefox should then ignore the <script> block. It SHOULD NOT parse the data as
HTML! e.g.

<script><!--
var x = '><iframe src="http://www.google.com"></iframe>';

This bug is VERY bad, because random web pages may get requested. And there
doesn't seem to be a way from preventing the browser from processing the data as
HTML if the end-script isn't sent.




Reproducible: Always

Steps to Reproduce:
1. Open the HTML shown above. (I'll attach the test case as well)
2.
3.

Actual Results:  
The iframe is shown.

Expected Results:  
The page should be blank. The script data should be ignored. That's what IE does.
(Reporter)

Comment 1

12 years ago
Created attachment 193791 [details]
Demonstrate the SCRIPT parsing problem

Updated

12 years ago
Assignee: nobody → parser
Component: General → HTML: Parser
Product: Firefox → Core
QA Contact: general → mrbkap
Version: unspecified → Trunk

Updated

12 years ago
Keywords: testcase
(Assignee)

Comment 2

12 years ago
I always thought we did this for compat with IE, but investigating shows that IE
actually consumes <script> and <style> non-conservatively (i.e., it eats to the
end of the document if we don't find the closing </script> or </style>).
However, IE refuses to execute such scripts.

Should we follow IE here? Opera does what we do. I'm not sure about other browsers.

Comment 3

12 years ago
Which IE did you test and in standards or quirks mode?  ;)
(Assignee)

Comment 4

12 years ago
This was IE6 on Windows. The behavior is the same in both standards and quirks mode.

Comment 5

12 years ago
Huh.  Do our cvs logs have any indication why we're doing things this way?
(Assignee)

Comment 6

12 years ago
The logs blame bug 35456, where harishd says that he implemented the current
behavior for compatibility with Nav4.x, but when I tested with 4.8, I saw really
weird behavior (a random character appeared on the page that went away when I
tried to select it). So it looks like the only sticking points for emulating IE
are: Opera's behavior (do we care?) and that we shouldn't run the script if
there is a malformed or missing </script> tag.

Updated

12 years ago
Blocks: 311395
(Assignee)

Comment 7

12 years ago
Confirming based on bug 311395.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P4
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Updated

12 years ago
Assignee: parser → mrbkap
QA Contact: mrbkap → parser
(Assignee)

Comment 8

12 years ago
Created attachment 208419 [details] [diff] [review]
Fix
Attachment #208419 - Flags: review?(bugmail)
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
Comment on attachment 208419 [details] [diff] [review]
Fix

this won't cancel enough serialization
Attachment #208419 - Flags: review?(bugmail) → review-
(Assignee)

Comment 10

12 years ago
Created attachment 209903 [details] [diff] [review]
"Fix" serialization

The viewPartialSource.js fixes are really hacks, but they work and they produce the "right" output. If anybody actually does use DOM2 elements to append elements to a document that has an unclosed <script>, they get what's coming to them!
Attachment #208419 - Attachment is obsolete: true
Attachment #209903 - Flags: review?(bugmail)
(Assignee)

Updated

12 years ago
Whiteboard: [patch]
Comment on attachment 209903 [details] [diff] [review]
"Fix" serialization

>Index: content/base/src/nsHTMLContentSerializer.cpp
>+static PRBool
>+HasNextSibling(nsIContent *aContent)

"sibling" isn't really used right here. Alternative names: HasNextInDocumentOrder, HasFollowingInDocumentOrder, ContentOrParentHasFollowingSibling

>@@ -724,36 +751,59 @@ nsHTMLContentSerializer::AppendElementSt
>   SerializeAttributes(content, name, aStr);
> 
>   AppendToString(kGreaterThan, aStr);
> 
>   if (LineBreakAfterOpen(name, hasDirtyAttr)) {
>     AppendToString(mLineBreak, aStr);
>     mMayIgnoreLineBreakSequence = PR_TRUE;
>     mColPos = 0;
>   }
> 
>+  if (name == nsHTMLAtoms::script) {
>+    // Handle malformed script serialization. In general, we don't want to
>+    // serialize end tags for scripts, so that round-tripping a document that

"serialize end tags of malformed scripts"

>+    // doesn't have a </script> won't suddently start executing scripts.
>+    // However, if someone was evil, and appended elements (using DOM2 methods)

What's wrong with DOM1 or DOM0 methods :) just say "DOM"
(Assignee)

Comment 12

12 years ago
Created attachment 209915 [details] [diff] [review]
Updated to comments
Attachment #209903 - Attachment is obsolete: true
Attachment #209915 - Flags: review?(bugmail)
Attachment #209903 - Flags: review?(bugmail)
Comment on attachment 209915 [details] [diff] [review]
Updated to comments

>Index: toolkit/components/viewsource/content/viewPartialSource.js
...
>+    var insertEndMarker = true;
>+    var tmpNode = ancestorContainer;
>+
>+    if (tmpNode instanceof HTMLScriptElement)
>+      tmpNode = tmpNode.parent;
>+
>+    while (tmpNode) {
>+      if (tmpNode.lastChild instanceof HTMLScriptElement) {
>+        var inner = tmpNode.innerHTML;
>+        if (inner != "<script>" && inner.charAt(inner.length - 1) != '>') {
>+          insertEndMarker = false;
>+        }
>+
>+        break;
>+      }
>+      tmpNode = tmpNode.lastChild;
>+    }


You can change this to 

    while (tmpNode) {
      if (tmpNode instanceof HTMLScriptElement && tmpNode.parentNode) {
        var inner = tmpNode.parentNode.innerHTML;
        if (inner != "<script>" && inner.charAt(inner.length - 1) != '>') {
          insertEndMarker = false;
        }

        break;
      }
      tmpNode = tmpNode.lastChild;
    }

To avoid the extra tmpNode = tmpNode.parentNode

r=me either way.
Attachment #209915 - Flags: review?(bugmail) → review+
(Assignee)

Updated

12 years ago
Attachment #209915 - Flags: superreview?(jst)
(Assignee)

Comment 14

12 years ago
Created attachment 210195 [details] [diff] [review]
With different roundtripping

We went back in time to my original serialization behavior. Ugh, here's hoping that this is the final patch.
Attachment #209915 - Attachment is obsolete: true
Attachment #210195 - Flags: superreview?(jst)
Attachment #210195 - Flags: review?(bugmail)
Attachment #209915 - Flags: superreview?(jst)
Comment on attachment 210195 [details] [diff] [review]
With different roundtripping

sr=jst
Attachment #210195 - Flags: superreview?(jst) → superreview+
Attachment #210195 - Flags: review?(bugmail) → review+
(Assignee)

Comment 16

12 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Blocks: 325500

Updated

12 years ago
No longer blocks: 325500
Depends on: 325500

Comment 17

12 years ago
So with this patch we have IE compat and not Opera/Konqueror compat, right?  I think that's fine, but perhaps we should file equivalent bugs on them?  And get Hixie to spec this out in HTML5?

Updated

12 years ago
No longer depends on: 325500

Updated

12 years ago
Blocks: 325500
(Assignee)

Updated

12 years ago
Blocks: 321142
No longer blocks: 325500
Depends on: 325500

Comment 18

12 years ago
Filed one on Opera.

Updated

12 years ago
Depends on: 327796

Comment 19

12 years ago
I filed bug 327796, "Unclosed scripts with src attribute should be allowed to run".  Anne and Hixie, since you're involved in speccing this and/or trying to ensure cross-browser consistency, you might be interested.
Comment on attachment 210195 [details] [diff] [review]
With different roundtripping

Is this one worth fixing for FF2? This particular patch changes the nsIScriptElement interface, though, so a 1.8 patch might not be worth the effort.
Attachment #210195 - Flags: approval-branch-1.8.1?(bugmail)
Comment on attachment 210195 [details] [diff] [review]
With different roundtripping

I'm not sure if it's worth the effort, but if someone wants to do it please go ahead. a=me if so.
Attachment #210195 - Flags: approval-branch-1.8.1?(bugmail) → approval-branch-1.8.1+

Comment 22

11 years ago
*** Bug 362397 has been marked as a duplicate of this bug. ***

Updated

10 years ago
Depends on: 386794

Updated

10 years ago
Blocks: 301375

Updated

10 years ago
Depends on: 393281
Flags: in-testsuite?

Updated

10 years ago
Depends on: 408702
Depends on: 412114

Updated

9 years ago
Keywords: dev-doc-needed
http://developer.mozilla.org/en/docs/Updating_web_applications_for_Firefox_3#Change_to_the_SCRIPT_element

Now documented.
Keywords: dev-doc-needed → dev-doc-complete

Comment 24

9 years ago
You may find the following commentary interesting:

http://slashdot.org/~einhverfr/journal/203094

Maybe others can request that this change be made too.

Comment 25

9 years ago
See bug 327796 and bug 408702.
Depends on: 448895
No longer depends on: 448895

Updated

9 years ago
Depends on: 448895

Updated

6 years ago
Depends on: 651936
You need to log in before you can comment on or make changes to this bug.