Changeset 144714 in webkit

Timestamp:

Mar 4, 2013 8:57:15 PM (11 years ago)

Author:

abarth@webkit.org

Message:

Background HTML parser can rewind the tokenizer after end-of-file
​https://bugs.webkit.org/show_bug.cgi?id=111365

Reviewed by Eric Seidel.

Source/WebCore:

Prior to this patch, it was possible to call didFailSpeculation after
processing the end-of-file token because checkForSpeculationFailure
didn't zero out m_tokenizer in some control paths.

This patch renames checkForSpeculationFailure to validateSpeculations
and ensures that it always takes ownership of the main thread's
HTMLTokenizer.

This patch also adds a number of ASSERTs to make sure the parser state
machine stays in the correct configuration (e.g., that we don't have a
main thread tokenizer while we're supposed to be tokenizing on the
background thread).

Test: fast/parser/document-write-fighting-eof.html

• html/parser/BackgroundHTMLInputStream.cpp:

(WebCore::BackgroundHTMLInputStream::rewindTo):

• html/parser/BackgroundHTMLParser.cpp:

(WebCore::BackgroundHTMLParser::append):

• html/parser/HTMLDocumentParser.cpp:

(WebCore::HTMLDocumentParser::validateSpeculations):
(WebCore::HTMLDocumentParser::processParsedChunkFromBackgroundParser):
(WebCore::HTMLDocumentParser::pumpPendingSpeculations):
(WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution):

• html/parser/HTMLDocumentParser.h:

(HTMLDocumentParser):

LayoutTests:

• fast/parser/document-write-fighting-eof.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/parser/document-write-fighting-eof-expected.txt (added)
• LayoutTests/fast/parser/document-write-fighting-eof.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/html/parser/BackgroundHTMLInputStream.cpp (modified) (1 diff)
• Source/WebCore/html/parser/BackgroundHTMLParser.cpp (modified) (1 diff)
• Source/WebCore/html/parser/HTMLDocumentParser.cpp (modified) (9 diffs)
• Source/WebCore/html/parser/HTMLDocumentParser.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2013-03-04  Adam Barth  <abarth@webkit.org>
+
+        Background HTML parser can rewind the tokenizer after end-of-file
+        https://bugs.webkit.org/show_bug.cgi?id=111365
+
+        Reviewed by Eric Seidel.
+
+        * fast/parser/document-write-fighting-eof.html: Added.
+
 2013-03-04  Tim 'mithro' Ansell  <mithro@mithis.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2013-03-04  Adam Barth  <abarth@webkit.org>
+
+        Background HTML parser can rewind the tokenizer after end-of-file
+        https://bugs.webkit.org/show_bug.cgi?id=111365
+
+        Reviewed by Eric Seidel.
+
+        Prior to this patch, it was possible to call didFailSpeculation after
+        processing the end-of-file token because checkForSpeculationFailure
+        didn't zero out m_tokenizer in some control paths.
+
+        This patch renames checkForSpeculationFailure to validateSpeculations
+        and ensures that it always takes ownership of the main thread's
+        HTMLTokenizer.
+
+        This patch also adds a number of ASSERTs to make sure the parser state
+        machine stays in the correct configuration (e.g., that we don't have a
+        main thread tokenizer while we're supposed to be tokenizing on the
+        background thread).
+
+        Test: fast/parser/document-write-fighting-eof.html
+
+        * html/parser/BackgroundHTMLInputStream.cpp:
+        (WebCore::BackgroundHTMLInputStream::rewindTo):
+        * html/parser/BackgroundHTMLParser.cpp:
+        (WebCore::BackgroundHTMLParser::append):
+        * html/parser/HTMLDocumentParser.cpp:
+        (WebCore::HTMLDocumentParser::validateSpeculations):
+        (WebCore::HTMLDocumentParser::processParsedChunkFromBackgroundParser):
+        (WebCore::HTMLDocumentParser::pumpPendingSpeculations):
+        (WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution):
+        * html/parser/HTMLDocumentParser.h:
+        (HTMLDocumentParser):
+
 2013-03-04  Tim 'mithro' Ansell  <mithro@mithis.com>
 

trunk/Source/WebCore/html/parser/BackgroundHTMLInputStream.cpp

@@ -72 +72 @@
         m_current.close();
 
+    ASSERT(m_current.isClosed() == isClosed);
+
     m_segments.clear();
     m_checkpoints.clear();

trunk/Source/WebCore/html/parser/BackgroundHTMLParser.cpp

@@ -147 +147 @@
 void BackgroundHTMLParser::append(const String& input)
 {
+    ASSERT(!m_input.current().isClosed());
     m_input.append(input);
     pumpTokenizer();

trunk/Source/WebCore/html/parser/HTMLDocumentParser.cpp

@@ -329 +329 @@
 }
 
-void HTMLDocumentParser::checkForSpeculationFailure()
-{
-    if (!m_tokenizer)
-        return;
-    if (!m_currentChunk)
-        return;
+void HTMLDocumentParser::validateSpeculations()
+{
+    OwnPtr<HTMLTokenizer> tokenizer = m_tokenizer.release();
+    OwnPtr<HTMLToken> token = m_token.release();
+
+    if (!tokenizer) {
+        // There must not have been any changes to the HTMLTokenizer state on
+        // the main thread, which means the speculation buffer is correct.
+        return;
+    }
+
+    if (!m_currentChunk) {
+        // If there is no m_currentChunk, we must have already called didFailSpeculation
+        // for this chunk.
+        // FIXME: In this case, we're losing whatever state has been changed since
+        // we called didFailSpeculation. See https://bugs.webkit.org/show_bug.cgi?id=110546
+        return;
+    }
+
     // Currently we're only smart enough to reuse the speculation buffer if the tokenizer
     // both starts and ends in the DataState. That state is simplest because the HTMLToken
@@ -340 +353 @@
     // speculation buffer in other states, but we'd likely need to do something more
     // sophisticated with the HTMLToken.
-    if (m_currentChunk->tokenizerState == HTMLTokenizer::DataState && m_tokenizer->state() == HTMLTokenizer::DataState && m_input.current().isEmpty()) {
-        ASSERT(m_token->isUninitialized());
-        m_tokenizer.clear();
-        m_token.clear();
-        return;
-    }
-    didFailSpeculation(m_token.release(), m_tokenizer.release());
+    if (m_currentChunk->tokenizerState == HTMLTokenizer::DataState
+        && tokenizer->state() == HTMLTokenizer::DataState
+        && m_input.current().isEmpty()) {
+        ASSERT(token->isUninitialized());
+        return;
+    }
+
+    didFailSpeculation(token.release(), tokenizer.release());
 }
 
@@ -373 +387 @@
     ASSERT(refCount() >= 2);
     ASSERT(shouldUseThreading());
+    ASSERT(!m_tokenizer);
+    ASSERT(!m_token);
 
     ActiveParserSession session(contextForParsingSession());
@@ -386 +402 @@
         if (XSSInfo* xssInfo = it->xssInfo())
             m_xssAuditorDelegate.didBlockScript(*xssInfo);
+
         constructTreeFromCompactHTMLToken(*it);
 
@@ -396 +413 @@
             // To match main-thread parser behavior (which never checks locationChangePending on the EOF path)
             // we peek to see if this chunk has an EOF and process it anyway.
-            if (tokens->last().type() == HTMLToken::EndOfFile)
+            if (tokens->last().type() == HTMLToken::EndOfFile) {
+                ASSERT(m_speculations.isEmpty());
                 prepareToStopParsing();
+            }
             break;
         }
@@ -404 +423 @@
             ASSERT(it + 1 == tokens->end()); // The </script> is assumed to be the last token of this bunch.
             runScriptsForPausedTreeBuilder();
+            validateSpeculations();
             break;
         }
@@ -409 +429 @@
         if (it->type() == HTMLToken::EndOfFile) {
             ASSERT(it + 1 == tokens->end()); // The EOF is assumed to be the last token of this bunch.
+            ASSERT(m_speculations.isEmpty());
             prepareToStopParsing();
             break;
         }
-    }
-
-    checkForSpeculationFailure();
+
+        ASSERT(!m_tokenizer);
+        ASSERT(!m_token);
+    }
 }
 
@@ -424 +446 @@
     // ASSERT that this object is both attached to the Document and protected.
     ASSERT(refCount() >= 2);
+    // If this assert fails, you need to call validateSpeculations to make sure
+    // m_tokenizer and m_token don't have state that invalidates m_speculations.
+    ASSERT(!m_tokenizer);
+    ASSERT(!m_token);
 
     // FIXME: Pass in current input length.
@@ -847 +873 @@
 #if ENABLE(THREADED_HTML_PARSER)
     if (m_haveBackgroundParser) {
-        checkForSpeculationFailure();
-
+        validateSpeculations();
         // processParsedChunkFromBackgroundParser can cause this parser to be detached from the Document,
         // but we need to ensure it isn't deleted yet.

trunk/Source/WebCore/html/parser/HTMLDocumentParser.h

@@ -141 +141 @@
     void startBackgroundParser();
     void stopBackgroundParser();
-    void checkForSpeculationFailure();
+    void validateSpeculations();
     void didFailSpeculation(PassOwnPtr<HTMLToken>, PassOwnPtr<HTMLTokenizer>);
     void processParsedChunkFromBackgroundParser(PassOwnPtr<ParsedChunk>);