31227 – Web site is stuck loading forever

RESOLVED FIXED 31227

Web site is stuck loading forever

https://bugs.webkit.org/show_bug.cgi?id=31227

Summary Web site is stuck loading forever

Dimitri Glazkov (Google)

Reported 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.

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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Hayato Ito

Comment 1 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.

Hayato Ito

Comment 2 2010-02-05 08:49:32 PST

Created attachment 48235 [details] fix-forever-page-loading

Darin Adler

Comment 3 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

Hayato Ito

Comment 4 2010-02-07 23:28:59 PST

Created attachment 48322 [details] fix-forever-page-loading-with-test

Hayato Ito

Comment 5 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

Hayato Ito

Comment 6 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();

Hayato Ito

Comment 7 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.

Shinichiro Hamaji

Comment 8 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).

Dimitri Glazkov (Google)

Comment 9 2010-02-08 10:35:02 PST

I just wanted to say that you guys rock!

Eric Seidel (no email)

Comment 10 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.

Hayato Ito

Comment 11 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).

Shinichiro Hamaji

Comment 12 2010-02-08 22:17:19 PST

Committed r54526: <http://trac.webkit.org/changeset/54526>

Shinichiro Hamaji

Comment 13 2010-02-08 23:15:31 PST

Committed r54529: <http://trac.webkit.org/changeset/54529>

Shinichiro Hamaji

Comment 14 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.

Antti Koivisto

Comment 15 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

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component Page Loading

Assignee

Dimitri Glazkov (Google)

Reported

2009-11-07 10:16 PST

Modified

2010-03-29 02:06 PDT History

CC List

6 users Show

URL

http://www.parentsdanslesparages.com/

Keywords

Depends on

Blocks