Bug 31227

Summary:

Web site is stuck loading forever

Product:

WebKit

Reporter:

Dimitri Glazkov (Google) <dglazkov>

Component:

Page Loading

Assignee:

Dimitri Glazkov (Google) <dglazkov>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

hamaji, hayato, hyatt, koivisto, ojan, sam

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

URL:

http://www.parentsdanslesparages.com/

Attachments:

Description	Flags
fix-forever-page-loading	none
fix-forever-page-loading-with-test	darin: review+, eric: commit-queue-

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.