Changeset 143664 in webkit

Timestamp:

Feb 21, 2013 4:39:36 PM (11 years ago)

Author:

eric@webkit.org

Message:

LayoutTests/fast/encoding/parser-tests-*.html timeout with threaded HTML parser
​https://bugs.webkit.org/show_bug.cgi?id=109995

Reviewed by Adam Barth.

Source/WebCore:

In the case where during main document onload, we
load a new iframe, and then from within that iframe
we run script to remove the iframe and call testRunner.notifyDone()
the notifyDone() will not correctly dump because
the testRunner does not yet realize that the main resource
has completed loading.

In the main-thread parser, the testRunner does correctly know
that the main thread has completed, because removing the iframe
causes a didFailLoad callback to the embedder, because when
the iframe is being removed, the DocumentLoader for that iframe
is still on the stack and believe's its loading
(because it has a MainResourceLoader which is also on the stack
delivering us the bytes which contain this inline script).

In the threaded-parser case, the DocumentLoader and MainResourceLoader
are no longer on the stack, as we are parsing the iframe asynchronously
after all the bytes have been delivered, and the MainResourceLoader destroyed.
Thus when DocumentLoader::stopLoading() is called, loading() returns
false, and it returns early. One might argue that we should remove that
early return entirely, but it seemed safer to extend the idea of
when we're loading to include the time when the parser is active.

This patch solves this by teaching the DocumentLoader that it is still
"loading" so long as the parser is still active.

Also added a call to DocumentLoader::checkLoadComplete from
Document::decrementActiveParserCount which seemed to cause
http/tests/multipart/policy-ignore-crash.php to pass.

This causes http/tests/security/feed-urls-from-remote.html to timeout
on chromium (but no other platforms that I'm aware of). I believe this
is due to a bug in our DRT implementation in the policyDelegate case
(which AFAIK is not a codepath which Chromium actually uses in the wild).
The test already times out on TOT if you remove the setCustomPolicyDelegate calls!

• loader/DocumentLoader.cpp:

(WebCore::DocumentLoader::isLoading):
(WebCore):

• loader/DocumentLoader.h:

(DocumentLoader):

LayoutTests:

Mark http/tests/security/feed-urls-from-remote.html as timeout on chromium.
I believe this is due to a bug in our DRT implementation in the policyDelegate case
(which AFAIK is not a codepath which Chromium actually uses in the wild).
The test already times out on TOT if you remove the setCustomPolicyDelegate calls.

• platform/chromium/TestExpectations:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/chromium/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/Document.cpp (modified) (1 diff)
• Source/WebCore/loader/DocumentLoader.cpp (modified) (1 diff)
• Source/WebCore/loader/DocumentLoader.h (modified) (3 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2013-02-21  Eric Seidel  <eric@webkit.org>
+
+        LayoutTests/fast/encoding/parser-tests-*.html timeout with threaded HTML parser
+        https://bugs.webkit.org/show_bug.cgi?id=109995
+
+        Reviewed by Adam Barth.
+
+        Mark http/tests/security/feed-urls-from-remote.html as timeout on chromium.
+        I believe this is due to a bug in our DRT implementation in the policyDelegate case
+        (which AFAIK is not a codepath which Chromium actually uses in the wild).
+        The test already times out on TOT if you remove the setCustomPolicyDelegate calls.
+
+        * platform/chromium/TestExpectations:
+
 2013-02-21  Erik Arvidsson  <arv@chromium.org>
 

trunk/LayoutTests/platform/chromium/TestExpectations

@@ -587 +587 @@
 storage/indexeddb/basics-shared-workers.html [ WontFix ]
 
+webkit.org/b/110401 http/tests/security/feed-urls-from-remote.html [ Timeout ]
+
 # test_shell does not support message ports
 webkit.org/b/74459 fast/workers/termination-with-port-messages.html

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2013-02-21  Eric Seidel  <eric@webkit.org>
+
+        LayoutTests/fast/encoding/parser-tests-*.html timeout with threaded HTML parser
+        https://bugs.webkit.org/show_bug.cgi?id=109995
+
+        Reviewed by Adam Barth.
+
+        In the case where during main document onload, we
+        load a new iframe, and then from within that iframe
+        we run script to remove the iframe and call testRunner.notifyDone()
+        the notifyDone() will not correctly dump because
+        the testRunner does not yet realize that the main resource
+        has completed loading.
+
+        In the main-thread parser, the testRunner does correctly know
+        that the main thread has completed, because removing the iframe
+        causes a didFailLoad callback to the embedder, because when
+        the iframe is being removed, the DocumentLoader for that iframe
+        is still on the stack and believe's its loading
+        (because it has a MainResourceLoader which is also on the stack
+        delivering us the bytes which contain this inline script).
+
+        In the threaded-parser case, the DocumentLoader and MainResourceLoader
+        are no longer on the stack, as we are parsing the iframe asynchronously
+        after all the bytes have been delivered, and the MainResourceLoader destroyed.
+        Thus when DocumentLoader::stopLoading() is called, loading() returns
+        false, and it returns early.  One might argue that we should remove that
+        early return entirely, but it seemed safer to extend the idea of
+        when we're loading to include the time when the parser is active.
+
+        This patch solves this by teaching the DocumentLoader that it is still
+        "loading" so long as the parser is still active.
+
+        Also added a call to DocumentLoader::checkLoadComplete from
+        Document::decrementActiveParserCount which seemed to cause
+        http/tests/multipart/policy-ignore-crash.php to pass.
+
+        This causes http/tests/security/feed-urls-from-remote.html to timeout
+        on chromium (but no other platforms that I'm aware of).  I believe this
+        is due to a bug in our DRT implementation in the policyDelegate case
+        (which AFAIK is not a codepath which Chromium actually uses in the wild).
+        The test already times out on TOT if you remove the setCustomPolicyDelegate calls!
+
+        * loader/DocumentLoader.cpp:
+        (WebCore::DocumentLoader::isLoading):
+        (WebCore):
+        * loader/DocumentLoader.h:
+        (DocumentLoader):
+
 2013-02-21  Erik Arvidsson  <arv@chromium.org>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -5774 +5774 @@
     if (!frame())
         return;
+    loader()->checkLoadComplete();
     frame()->loader()->checkLoadComplete();
 }

trunk/Source/WebCore/loader/DocumentLoader.cpp

@@ -279 +279 @@
 }
 
+bool DocumentLoader::isLoading() const
+{
+    if (m_frame->document() && m_frame->document()->hasActiveParser())
+        return true;
+    return isLoadingMainResource() || !m_subresourceLoaders.isEmpty() || !m_plugInStreamLoaders.isEmpty();
+}
+
 void DocumentLoader::finishedLoading()
 {

trunk/Source/WebCore/loader/DocumentLoader.h

@@ -115 +115 @@
         void setCommitted(bool committed) { m_committed = committed; }
         bool isCommitted() const { return m_committed; }
-        bool isLoading() const { return isLoadingMainResource() || !m_subresourceLoaders.isEmpty() || !m_plugInStreamLoaders.isEmpty(); }
+        bool isLoading() const;
         void receivedData(const char*, int);
         void setupForReplace();
@@ -248 +248 @@
 
         virtual void reportMemoryUsage(MemoryObjectInfo*) const;
+        void checkLoadComplete();
 
     protected:
@@ -258 +259 @@
         void setMainDocumentError(const ResourceError&);
         void commitLoad(const char*, int);
-        void checkLoadComplete();
         void clearMainResourceLoader();