Bug 31227 - Web site is stuck loading forever
Summary: Web site is stuck loading forever
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL: http://www.parentsdanslesparages.com/
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-07 10:16 PST by Dimitri Glazkov (Google)
Modified: 2010-03-29 02:06 PDT (History)
6 users (show)

See Also:


Attachments
fix-forever-page-loading (1.29 KB, patch)
2010-02-05 08:49 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
fix-forever-page-loading-with-test (3.99 KB, patch)
2010-02-07 23:28 PST, Hayato Ito
darin: review+
eric: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2009-11-07 10:16:51 PST
There's this cute web site that gets stuck loading forever in WebKit -- unless you click on it. The symptoms are:

1) go to http://www.parentsdanslesparages.com/
2) witness the page being stuck in loading, painting white emptiness.
3) click on the page.
4) observe page loading

What happens here is that the HTMLTokenizer::continueProcessing (http://trac.webkit.org/browser/trunk/WebCore/html/HTMLTokenizer.cpp#L1597) falls through without setting a timer to process another chunk of data, but there's nothing that picks up the processing. So we're stuck until you try to focus or trigger a hit test on the page, thus restarting processing cycle.
Comment 1 Hayato Ito 2010-02-04 23:25:28 PST
I can reproduce the issue in the following simple html:


<!DOCTYPE html>
<html>
  <head>
    <title>Stall Test</title>
     <script type="text/javascript">
//<![CDATA[
for (var i = 0; i < 10; ++i) {
   var req = new window.XMLHttpRequest();
   req.open("GET", "hello.txt");
   req.send();
}
//]]>
</script>
  </head>
  <body>
    <script type="text/javascript" src="./after.js"></script>
    <div>Finished</div>
  </body>
</html>


The point is that requesting a lot of XMLHTTPRequests, and then tring to load a JS file (after.js) after that.

I guess the root cause exists in WebCore::Loader.
Please take a look at Loader::Host::servePendingRequests:

http://trac.webkit.org/browser/trunk/WebCore/loader/loader.cpp#L312

void Loader::Host::servePendingRequests(RequestQueue& requestsPending, bool& serveLowerPriority)
{
    while (!requestsPending.isEmpty()) {        
        .....
        bool shouldLimitRequests = !m_name.isNull() || docLoader->doc()->parsing() || !docLoader->doc()->haveStylesheetsLoaded();
        if (shouldLimitRequests && m_requestsLoading.size() + m_nonCachedRequestsInFlight >= m_maxRequestsInFlight) {
            serveLowerPriority = false;
            return;
        }


Here, the Loader::Host finds that theare are a number of pending XHRs (=10) beyond the threshold (m_maxRequestsInFlight(=4)) and gives up servePendingRequests, which includes a request for loading 'after.js'.

It seems that the loader gives up here *WITHOUT* putting another callback function (Loader::requestTimerFired) to timerHeap() (threadGlobalData().threadTimers().timerHeap()).

So Loader::requestTimerFired won't be called again, and 'after.js' won't be loaded.

As a result, HTMLTokenizer has a pending src (m_pendingSrc = "<div>Finished<div></body></html>" and it remains to be unprocessed.
If loading 'after.js' were finished, it calls HTMLTokenizer::executeExternalScriptIfReady, and it calls HTMLTokenizer::write. But it is not the case.

I am now thinking how to fix the issue.
Please let me know if you guys have a nice idea to fix the issue.
Comment 2 Hayato Ito 2010-02-05 08:49:32 PST
Created attachment 48235 [details]
fix-forever-page-loading
Comment 3 Darin Adler 2010-02-05 09:20:27 PST
Comment on attachment 48235 [details]
fix-forever-page-loading

Fix looks great. Thanks for tackling it.

We require test cases for bug fixes.

So this patch will need either a test case for the LayoutTests directory -- presumably it would be one of the http tests -- or an explanation of how we tried and were unable to make an automated test case and a manual test case of some sort.

review- because of the lack of a test case
Comment 4 Hayato Ito 2010-02-07 23:28:59 PST
Created attachment 48322 [details]
fix-forever-page-loading-with-test
Comment 5 Hayato Ito 2010-02-07 23:31:50 PST
Thank you for the review. I added a http test. Could you review it?

(In reply to comment #3)
> (From update of attachment 48235 [details])
> Fix looks great. Thanks for tackling it.
> 
> We require test cases for bug fixes.
> 
> So this patch will need either a test case for the LayoutTests directory --
> presumably it would be one of the http tests -- or an explanation of how we
> tried and were unable to make an automated test case and a manual test case of
> some sort.
> 
> review- because of the lack of a test case
Comment 6 Hayato Ito 2010-02-08 10:03:34 PST
Comment on attachment 48322 [details]
fix-forever-page-loading-with-test

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index a514359..5c266f3 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2010-02-07  Hayato Ito  <hayato@chromium.org>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Schedule a loading request when there are many in-flight requests beyond
>+        the limit to avoid forever page loading.
>+
>+        https://bugs.webkit.org/show_bug.cgi?id=31227
>+
>+        * http/tests/loading/load-javascript-after-many-xhrs-expected.txt: Added.
>+        * http/tests/loading/load-javascript-after-many-xhrs.html: Added.
>+        * http/tests/loading/resources/zero-length.js: Added.
>+        * http/tests/loading/resources/zero-length.txt: Added.
>+
> 2010-01-31  Kent Tamura  <tkent@chromium.org>
> 
>         Reviewed by Darin Adler.
>diff --git a/LayoutTests/http/tests/loading/load-javascript-after-many-xhrs-expected.txt b/LayoutTests/http/tests/loading/load-javascript-after-many-xhrs-expected.txt
>new file mode 100644
>index 0000000..24597fe
>--- /dev/null
>+++ b/LayoutTests/http/tests/loading/load-javascript-after-many-xhrs-expected.txt
>@@ -0,0 +1,6 @@
>+main frame - didStartProvisionalLoadForFrame
>+main frame - didCommitLoadForFrame
>+main frame - didFinishDocumentLoadForFrame
>+main frame - didHandleOnloadEventsForFrame
>+main frame - didFinishLoadForFrame
>+Test for https://bugs.webkit.org/show_bug.cgi?id=31227. If it didn't crash, the browser passed the test.
>diff --git a/LayoutTests/http/tests/loading/load-javascript-after-many-xhrs.html b/LayoutTests/http/tests/loading/load-javascript-after-many-xhrs.html
>new file mode 100644
>index 0000000..1a2d474
>--- /dev/null
>+++ b/LayoutTests/http/tests/loading/load-javascript-after-many-xhrs.html
>@@ -0,0 +1,21 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<script type="text/javascript">
>+for (var i = 0; i < 10; ++i) {
>+   var req = new window.XMLHttpRequest();
>+   req.open("GET", "resources/zero-length.txt");
>+   req.send();
>+}
>+</script>
>+<script>
>+if (window.layoutTestController)
>+    layoutTestController.dumpAsText();
>+</script>
>+</head>
>+<body>
>+<script type="text/javascript" src="resources/zero-length.js"></script>
>+Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=31227">https://bugs.webkit.org/show_bug.cgi?id=31227</a>.
>+If it didn't crash, the browser passed the test.
>+</body>
>+</html>
>diff --git a/LayoutTests/http/tests/loading/resources/zero-length.js b/LayoutTests/http/tests/loading/resources/zero-length.js
>new file mode 100644
>index 0000000..e69de29
>diff --git a/LayoutTests/http/tests/loading/resources/zero-length.txt b/LayoutTests/http/tests/loading/resources/zero-length.txt
>new file mode 100644
>index 0000000..e69de29
>diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
>index f2e6b80..cdd3c5c 100644
>--- a/WebCore/ChangeLog
>+++ b/WebCore/ChangeLog
>@@ -1,3 +1,17 @@
>+2010-02-07  Hayato Ito  <hayato@chromium.org>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Schedule a loading request when there are many in-flight requests beyond
>+        the limit to avoid forever page loading.
>+
>+        https://bugs.webkit.org/show_bug.cgi?id=31227
>+
>+        Test: http/tests/loading/load-javascript-after-many-xhrs.html
>+
>+        * loader/loader.cpp:
>+        (WebCore::Loader::Host::servePendingRequests):
>+
> 2010-01-31  Kent Tamura  <tkent@chromium.org>
> 
>         Unreviewed. Revert r54112 and r54124 because of Windows build error.
>diff --git a/WebCore/loader/loader.cpp b/WebCore/loader/loader.cpp
>index d693341..4d2b474 100644
>--- a/WebCore/loader/loader.cpp
>+++ b/WebCore/loader/loader.cpp
>@@ -322,6 +322,7 @@ void Loader::Host::servePendingRequests(RequestQueue& requestsPending, bool& ser
>         bool shouldLimitRequests = !m_name.isNull() || docLoader->doc()->parsing() || !docLoader->doc()->haveStylesheetsLoaded();
>         if (shouldLimitRequests && m_requestsLoading.size() + m_nonCachedRequestsInFlight >= m_maxRequestsInFlight) {
>             serveLowerPriority = false;
>+            cache()->loader()->scheduleServePendingRequests();
>             return;
>         }
>         requestsPending.removeFirst();
Comment 7 Hayato Ito 2010-02-08 10:11:59 PST
Hmm.. Every bot's result colors are purple.
It seems svn-apply doesn't work for a patch generated by git if a patch contains new empty file, isn't it?

If commit-queue can't process the patch I attached, I'll make a patch using 'svn' instead of 'git'.  I'll update the patch in a day.
Comment 8 Shinichiro Hamaji 2010-02-08 10:24:18 PST
(In reply to comment #7)
> Hmm.. Every bot's result colors are purple.
> It seems svn-apply doesn't work for a patch generated by git if a patch
> contains new empty file, isn't it?
> 
> If commit-queue can't process the patch I attached, I'll make a patch using
> 'svn' instead of 'git'.  I'll update the patch in a day.

I think your guess is correct. You don't need to update your patch - I'll manually land this patch tomorrow (or, you may be able to ask other committers to land).
Comment 9 Dimitri Glazkov (Google) 2010-02-08 10:35:02 PST
I just wanted to say that you guys rock!
Comment 10 Eric Seidel (no email) 2010-02-08 15:25:38 PST
Comment on attachment 48322 [details]
fix-forever-page-loading-with-test

Given that all of the EWS bubbles show purple for this, that would suggest the commit-queue won't be able to process this.
Comment 11 Hayato Ito 2010-02-08 21:05:55 PST
Thank you, hamaji-san. Could you land it?

The issue that 'svn-apply' doesn't work here is already filed at
https://bugs.webkit.org/show_bug.cgi?id=29684

(In reply to comment #8)
> (In reply to comment #7)
> > Hmm.. Every bot's result colors are purple.
> > It seems svn-apply doesn't work for a patch generated by git if a patch
> > contains new empty file, isn't it?
> > 
> > If commit-queue can't process the patch I attached, I'll make a patch using
> > 'svn' instead of 'git'.  I'll update the patch in a day.
> 
> I think your guess is correct. You don't need to update your patch - I'll
> manually land this patch tomorrow (or, you may be able to ask other committers
> to land).
Comment 12 Shinichiro Hamaji 2010-02-08 22:17:19 PST
Committed r54526: <http://trac.webkit.org/changeset/54526>
Comment 13 Shinichiro Hamaji 2010-02-08 23:15:31 PST
Committed r54529: <http://trac.webkit.org/changeset/54529>
Comment 14 Shinichiro Hamaji 2010-02-08 23:17:24 PST
(In reply to comment #13)
> Committed r54529: <http://trac.webkit.org/changeset/54529>

This was an unreviewed attempt to make gtk bot green.

The failure: http://build.webkit.org/results/GTK%20Linux%2032-bit%20Debug/r54527%20(3057)/http/tests/loading/

I think Bug 32170 is fixing this issue.
Comment 15 Antti Koivisto 2010-03-29 02:06:16 PDT
This patch introduces a timer restart loop that makes CPU peak at 100% during loading. 

requestTimerFired() -> servePendingRequests() -> scheduleServePendingRequests() and repeat

Bug 36703