Load event fires too early with threaded HTML parser
authorabarth@webkit.org <abarth@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 9 Feb 2013 19:34:08 +0000 (19:34 +0000)
committerabarth@webkit.org <abarth@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 9 Feb 2013 19:34:08 +0000 (19:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=108984

Reviewed by Eric Seidel.

Previously, the DocumentLoader would always be on the stack when the
HTMLDocumentParser was processing data from the network.  The
DocumentLoader would then tell isLoadingInAPISense not to fire the load
event.  Now that we process data asynchronously with the threaded
parser, the DocumentLoader is not always on the stack, which means we
need to delay the load event using the clause that asks the parser
whether it is processing data.

Unfortunately, that clause is fragile because we can check for load
completion while we're switching parsers between the network-created
parser and a script-created parser. To avoid accidentially triggerin
the load event during these "gaps," this patch introduces a counter on
document to record how many parsers are active on the stack.  While
that numer is non-zero, we'll delay the load event. When that number
reaches zero, we'll check for load complete.

That last step is required because the DocumentLoader::finishLoading
method is no longer guarunteed to check for load complete after calling
finish on the parser because the finish operation might complete
asynchronously.

After this patch, the threaded parser passes all but four fast/parser
tests.

* dom/Document.cpp:
(WebCore::Document::Document):
(WebCore::Document::hasActiveParser):
(WebCore):
(WebCore::Document::decrementActiveParserCount):
* dom/Document.h:
(Document):
(WebCore::Document::incrementActiveParserCount):
* html/parser/HTMLDocumentParser.cpp:
(WebCore::HTMLDocumentParser::processParsedChunkFromBackgroundParser):
(WebCore::HTMLDocumentParser::pumpTokenizer):
* html/parser/HTMLParserScheduler.cpp:
(WebCore::ActiveParserSession::ActiveParserSession):
(WebCore):
(WebCore::ActiveParserSession::~ActiveParserSession):
(WebCore::PumpSession::PumpSession):
(WebCore::PumpSession::~PumpSession):
* html/parser/HTMLParserScheduler.h:
(WebCore):
(ActiveParserSession):
(PumpSession):
* loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::isLoadingInAPISense):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@142378 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Document.h
Source/WebCore/html/parser/HTMLDocumentParser.cpp
Source/WebCore/html/parser/HTMLDocumentParser.h
Source/WebCore/html/parser/HTMLParserScheduler.cpp
Source/WebCore/html/parser/HTMLParserScheduler.h
Source/WebCore/loader/DocumentLoader.cpp

index a2014f8..ae207c4 100644 (file)
@@ -1,3 +1,58 @@
+2013-02-09  Adam Barth  <abarth@webkit.org>
+
+        Load event fires too early with threaded HTML parser
+        https://bugs.webkit.org/show_bug.cgi?id=108984
+
+        Reviewed by Eric Seidel.
+
+        Previously, the DocumentLoader would always be on the stack when the
+        HTMLDocumentParser was processing data from the network.  The
+        DocumentLoader would then tell isLoadingInAPISense not to fire the load
+        event.  Now that we process data asynchronously with the threaded
+        parser, the DocumentLoader is not always on the stack, which means we
+        need to delay the load event using the clause that asks the parser
+        whether it is processing data.
+
+        Unfortunately, that clause is fragile because we can check for load
+        completion while we're switching parsers between the network-created
+        parser and a script-created parser. To avoid accidentially triggerin
+        the load event during these "gaps," this patch introduces a counter on
+        document to record how many parsers are active on the stack.  While
+        that numer is non-zero, we'll delay the load event. When that number
+        reaches zero, we'll check for load complete.
+
+        That last step is required because the DocumentLoader::finishLoading
+        method is no longer guarunteed to check for load complete after calling
+        finish on the parser because the finish operation might complete
+        asynchronously.
+
+        After this patch, the threaded parser passes all but four fast/parser
+        tests.
+
+        * dom/Document.cpp:
+        (WebCore::Document::Document):
+        (WebCore::Document::hasActiveParser):
+        (WebCore):
+        (WebCore::Document::decrementActiveParserCount):
+        * dom/Document.h:
+        (Document):
+        (WebCore::Document::incrementActiveParserCount):
+        * html/parser/HTMLDocumentParser.cpp:
+        (WebCore::HTMLDocumentParser::processParsedChunkFromBackgroundParser):
+        (WebCore::HTMLDocumentParser::pumpTokenizer):
+        * html/parser/HTMLParserScheduler.cpp:
+        (WebCore::ActiveParserSession::ActiveParserSession):
+        (WebCore):
+        (WebCore::ActiveParserSession::~ActiveParserSession):
+        (WebCore::PumpSession::PumpSession):
+        (WebCore::PumpSession::~PumpSession):
+        * html/parser/HTMLParserScheduler.h:
+        (WebCore):
+        (ActiveParserSession):
+        (PumpSession):
+        * loader/DocumentLoader.cpp:
+        (WebCore::DocumentLoader::isLoadingInAPISense):
+
 2013-02-09  Mike West  <mkwst@chromium.org>
 
         Use IGNORE_EXCEPTION for initialized, but unused, ExceptionCodes.
index bf546c3..4d62963 100644 (file)
@@ -412,6 +412,7 @@ Document::Document(Frame* frame, const KURL& url, bool isXHTML, bool isHTML)
     , m_guardRefCount(0)
     , m_styleResolverThrowawayTimer(this, &Document::styleResolverThrowawayTimerFired)
     , m_lastStyleResolverAccessTime(0)
+    , m_activeParserCount(0)
     , m_contextFeatures(ContextFeatures::defaultSwitch())
     , m_compatibilityMode(NoQuirksMode)
     , m_compatibilityModeLocked(false)
@@ -5768,6 +5769,19 @@ void Document::adjustFloatRectForScrollAndAbsoluteZoomAndFrameScale(FloatRect& r
         rect.scale(inverseFrameScale);
 }
 
+bool Document::hasActiveParser()
+{
+    return m_activeParserCount || (m_parser && m_parser->processingData());
+}
+
+void Document::decrementActiveParserCount()
+{
+    --m_activeParserCount;
+    if (!frame())
+        return;
+    frame()->loader()->checkLoadComplete();
+}
+
 void Document::setContextFeatures(PassRefPtr<ContextFeatures> features)
 {
     m_contextFeatures = features;
index 7bd9f19..72c8399 100644 (file)
@@ -1168,6 +1168,10 @@ public:
     void adjustFloatQuadsForScrollAndAbsoluteZoomAndFrameScale(Vector<FloatQuad>&, RenderObject*);
     void adjustFloatRectForScrollAndAbsoluteZoomAndFrameScale(FloatRect&, RenderObject*);
 
+    bool hasActiveParser();
+    void incrementActiveParserCount() { ++m_activeParserCount; }
+    void decrementActiveParserCount();
+
     void setContextFeatures(PassRefPtr<ContextFeatures>);
     ContextFeatures* contextFeatures() { return m_contextFeatures.get(); }
 
@@ -1307,6 +1311,7 @@ private:
 
     RefPtr<CachedResourceLoader> m_cachedResourceLoader;
     RefPtr<DocumentParser> m_parser;
+    unsigned m_activeParserCount;
     RefPtr<ContextFeatures> m_contextFeatures;
 
     bool m_wellFormed;
index da8463c..870e034 100644 (file)
@@ -313,6 +313,8 @@ void HTMLDocumentParser::processParsedChunkFromBackgroundParser(PassOwnPtr<Parse
     // but we need to ensure it isn't deleted yet.
     RefPtr<HTMLDocumentParser> protect(this);
 
+    ActiveParserSession session(contextForParsingSession());
+
     m_currentChunk = chunk;
     OwnPtr<CompactHTMLTokenStream> tokens = m_currentChunk->tokens.release();
 
@@ -370,6 +372,15 @@ void HTMLDocumentParser::forcePlaintextForTextDocument()
         m_tokenizer->setState(HTMLTokenizerState::PLAINTEXTState);
 }
 
+Document* HTMLDocumentParser::contextForParsingSession()
+{
+    // The parsing session should interact with the document only when parsing
+    // non-fragments. Otherwise, we might delay the load event mistakenly.
+    if (isParsingFragment())
+        return 0;
+    return document();
+}
+
 void HTMLDocumentParser::pumpTokenizer(SynchronousMode mode)
 {
     ASSERT(!isStopped());
@@ -379,7 +390,7 @@ void HTMLDocumentParser::pumpTokenizer(SynchronousMode mode)
 
     ASSERT(!shouldUseThreading() || mode == ForceSynchronous);
 
-    PumpSession session(m_pumpSessionNestingLevel);
+    PumpSession session(m_pumpSessionNestingLevel, contextForParsingSession());
 
     // We tell the InspectorInstrumentation about every pump, even if we
     // end up pumping nothing.  It can filter out empty pumps itself.
index 19c8d76..0f6a2e5 100644 (file)
@@ -137,6 +137,8 @@ private:
     void processParsedChunkFromBackgroundParser(PassOwnPtr<ParsedChunk>);
 #endif
 
+    Document* contextForParsingSession();
+
     enum SynchronousMode {
         AllowYield,
         ForceSynchronous,
index a9dd24a..9ecacc1 100644 (file)
@@ -61,6 +61,38 @@ static int parserChunkSize(Page* page)
     return defaultParserChunkSize;
 }
 
+ActiveParserSession::ActiveParserSession(Document* document)
+    : m_document(document)
+{
+    if (!m_document)
+        return;
+    m_document->incrementActiveParserCount();
+}
+
+ActiveParserSession::~ActiveParserSession()
+{
+    if (!m_document)
+        return;
+    m_document->decrementActiveParserCount();
+}
+
+PumpSession::PumpSession(unsigned& nestingLevel, Document* document)
+    : NestingLevelIncrementer(nestingLevel)
+    , ActiveParserSession(document)
+    // Setting processedTokens to INT_MAX causes us to check for yields
+    // after any token during any parse where yielding is allowed.
+    // At that time we'll initialize startTime.
+    , processedTokens(INT_MAX)
+    , startTime(0)
+    , needsYield(false)
+    , didSeeScript(false)
+{
+}
+
+PumpSession::~PumpSession()
+{
+}
+
 HTMLParserScheduler::HTMLParserScheduler(HTMLDocumentParser* parser)
     : m_parser(parser)
     , m_parserTimeLimit(parserTimeLimit(m_parser->document()->page()))
index e52f3ef..9112c58 100644 (file)
 #ifndef HTMLParserScheduler_h
 #define HTMLParserScheduler_h
 
-#include <limits.h>
-
 #include "NestingLevelIncrementer.h"
 #include "Timer.h"
+#include <limits.h>
 #include <wtf/CurrentTime.h>
 #include <wtf/PassOwnPtr.h>
+#include <wtf/RefPtr.h>
 
 namespace WebCore {
 
+class Document;
 class HTMLDocumentParser;
 
-class PumpSession : public NestingLevelIncrementer {
+class ActiveParserSession {
 public:
-    PumpSession(unsigned& nestingLevel)
-        : NestingLevelIncrementer(nestingLevel)
-        // Setting processedTokens to INT_MAX causes us to check for yields
-        // after any token during any parse where yielding is allowed.
-        // At that time we'll initialize startTime.
-        , processedTokens(INT_MAX)
-        , startTime(0)
-        , needsYield(false)
-        , didSeeScript(false)
-    {
-    }
+    explicit ActiveParserSession(Document*);
+    ~ActiveParserSession();
+
+private:
+    RefPtr<Document> m_document;
+};
+
+class PumpSession : public NestingLevelIncrementer, public ActiveParserSession {
+public:
+    PumpSession(unsigned& nestingLevel, Document*);
+    ~PumpSession();
 
     int processedTokens;
     double startTime;
index 9a98bb7..fbc1e8b 100644 (file)
@@ -478,9 +478,8 @@ bool DocumentLoader::isLoadingInAPISense() const
             return true;
         if (m_cachedResourceLoader->requestCount())
             return true;
-        if (DocumentParser* parser = doc->parser())
-            if (parser->processingData())
-                return true;
+        if (doc->hasActiveParser())
+            return true;
     }
     return frameLoader()->subframeIsLoading();
 }