Bug 8626 - Strict mode erroneously triggered by a broken comment
Summary: Strict mode erroneously triggered by a broken comment
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 418.x
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Alexey Proskuryakov
URL: http://mayan.tzo.com:888/dreamsp/drea...
Depends on:
Reported: 2006-04-27 01:32 PDT by Matt Gough
Modified: 2006-07-11 03:40 PDT (History)
2 users (show)

See Also:

A reduction (209 bytes, text/html)
2006-04-27 01:44 PDT, Matt Gough
no flags Details
proposed fix (3.33 KB, patch)
2006-04-28 11:50 PDT, Alexey Proskuryakov
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Gough 2006-04-27 01:32:03 PDT
On this page there is a comment at the start : <!--@@Advance "Dreamspell"-> and right near the bottom there is another one : <!--@@Count "Dreamspell"->.

WebKit ignores everything between these comments, whereas other browsers (Firefox, opera and IE Mac) display the page as the author 'intended'.
Comment 1 Matt Gough 2006-04-27 01:44:13 PDT
Created attachment 7991 [details]
A reduction
Comment 2 Alexey Proskuryakov 2006-04-27 21:43:58 PDT
The symptom gets fixed with reverting the SGML comment parsing fix made for acid2 (cf. bug 5855). However, there is another related problem - the test is parsed in strict mode, while it apparently shouldn't.

Here, strict mode is triggered by having the <!--@@Advance "Dreamspell"-> comment anywhere before BODY.
Comment 3 Alexey Proskuryakov 2006-04-28 11:50:43 PDT
Created attachment 8023 [details]
proposed fix

Properly determine the parsing mode if we have reached Frame::endIfNotLoading() without ever seeing any data from the decoder, which is the case when a broken comment is somewhere before <body>.
Comment 4 Darin Adler 2006-04-28 17:13:07 PDT
Comment on attachment 8023 [details]
proposed fix

Why does this new code belong here rather than in Frame::write? What's different about end as opposed to write that means we should do setParseMode in there, but determineParseMode here?
Comment 5 Alexey Proskuryakov 2006-04-28 22:40:26 PDT
There are two Frame::write() methods, one also calls determineParseMode(), and another calls setParseMode(Strict). When loading data, it is the first that gets called, so we end up with calling determineParseMode() consistently.

I didn't investigate why the second Frame::write() just sets strict mode, as it's not directly related to this issue. Looking at it now, it is used for documents created via document.write(), window.open(), DOMImplementation::createHTMLDocument(), Frame::replaceContentsWithScriptResult(), Frame::changeLocation(), and XSLTProcessor. Presumably, some or all of these uses imply strict mode. It's not immediately obvious if this is always correct (and some of these actually call determineParseMode() on their own), but I don't think this patch changes their behavior anyway.
Comment 6 Maciej Stachowiak 2006-05-04 13:32:03 PDT
Comment on attachment 8023 [details]
proposed fix

Comment 7 David Kilzer (:ddkilzer) 2006-07-11 03:40:05 PDT
Note that this bug apparently fixed Bug 7102 as well.