The default bug view has changed. See this FAQ.

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

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

Comment 8

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

Updated

11 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

11 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

11 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

11 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

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

Comment 14

11 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

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

Updated

11 years ago
Blocks: 325500

Updated

11 years ago
No longer blocks: 325500
Depends on: 325500
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

11 years ago
No longer depends on: 325500

Updated

11 years ago
Blocks: 325500
(Assignee)

Updated

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

Comment 18

11 years ago
Filed one on Opera.

Updated

11 years ago
Depends on: 327796

Comment 19

11 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

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