24747 – preloading logic caused the same resource was loaded and reloaded.(sent two requests for same resource)

RESOLVED FIXED 24747

preloading logic caused the same resource was loaded and reloaded.(sent two requests for same resource)

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

Summary preloading logic caused the same resource was loaded and reloaded.(sent two r...

johnnyding

Reported 2009-03-22 06:43:12 PDT

Please refer to the following html content <HTML><HEAD><META http-equiv="Content-Type" content="text/html; charset=UTF-8"> <SCRIPT type="text/javascript" src="test.js"></SCRIPT> </HEAD><BODY><IMG src="test.jpg"> </BODY></HTML> The following is bug analysis: 1)When requiring "test.js", WebKit started a PreloadScanner to see whether there were some resources written in m_pendingSrc which could be preloaded in function "HTMLTokenizer::scriptHandler". 2)In "DocLoader::preload", it put resources in pending list when they were preload images or body resources before WebKit had something to draw, which caused "test.jpg" was put in DocLoader's m_pendingPreloads. 3)When finishing "test.js", WebKit continued parsing m_pendingSrc in "HTMLTokenizer::notifyFinished", which caused WebKit called "requestResource" to get a CachedResource for "test.jpg" (parameter isPreload is false). 4)After calling "HTMLTokenizer::notifyFinished", the DocLoader's checkForPendingPreloads was called(see Loader::Host::didFinishLoading). In this function, it called "requestResource" again to get a CachedResource for "test.jpg" (parameter isPreload is true). Inside "DocLoader::requestResource", the function "checkForReload" removed the existing CachedResource for "test.jpg" created in step3 and started to re-create a CachedResource to reload "test.jpg", which is totally unnecessary. The reason of this problem is that WebKit created CachedResource for one resource and reloaded it because of preloading logic. I think we should check isPreload parameter in function "checkForReload", ignore the reload request if the isPreload is true when there is one CachedResource existed for same resource. The patch for the fix and corresponding testcase will be sent soon. Thanks!

Attachments
HTML file for reproducing this problem (186 bytes, text/html) 2009-03-22 06:44 PDT, johnnyding	no flags	Details
image file for the test HTML (3.10 KB, image/jpeg) 2009-03-22 06:45 PDT, johnnyding	no flags	Details
empty js file for the test HTML (23 bytes, text/plain) 2009-03-22 06:46 PDT, johnnyding	no flags	Details
patch V1 for fixing this bug, so far no test (2.28 KB, patch) 2009-03-23 05:40 PDT, johnnyding	no flags	Details Formatted Diff Diff
patch V2 to fix this bug (14.06 KB, patch) 2009-04-02 02:25 PDT, johnnyding	no flags	Details Formatted Diff Diff
latest V2 patch (13.90 KB, patch) 2009-04-02 03:24 PDT, johnnyding	no flags	Details Formatted Diff Diff
update patch v2 (14.06 KB, patch) 2009-04-12 04:48 PDT, johnnyding	no flags	Details Formatted Diff Diff
patch for test of this issue (9.42 KB, patch) 2009-05-27 11:37 PDT, johnnyding	ap: review+	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

johnnyding

Comment 1 2009-03-22 06:44:28 PDT

Created attachment 28831 [details] HTML file for reproducing this problem

johnnyding

Comment 2 2009-03-22 06:45:07 PDT

Created attachment 28832 [details] image file for the test HTML

johnnyding

Comment 3 2009-03-22 06:46:00 PDT

Created attachment 28833 [details] empty js file for the test HTML

johnnyding

Comment 4 2009-03-23 05:40:54 PDT

Created attachment 28851 [details] patch V1 for fixing this bug, so far no test I did not add test for this patch. It's because to reproduce this problem, I have to rewrite my own HTTP server to catch up the image reload and return a 401 error to let page trigger the error condition. I am not sure whether you guys have concerns on it or not. If not, I will add it on patch V2. It's highly appreciated if someone can point me out the similar existing testcase, then I can leverage it. Thanks!

Alexey Proskuryakov

Comment 5 2009-03-23 09:02:47 PDT

> 401 error You could use technique from http/tests/resources/network-simulator.php to make a stateful resource that would generate an error every other time. Some caution should be taken to reset the state before and/or after the test, so that each run of the test would be guaranteed to be the same, even if code were temporarily broken.

johnnyding

Comment 6 2009-04-02 02:25:11 PDT

Created attachment 29186 [details] patch V2 to fix this bug V2 is ready. sorry for being late.

johnnyding

Comment 7 2009-04-02 03:24:22 PDT

Created attachment 29187 [details] latest V2 patch

Mark Rowe (bdash)

Comment 8 2009-04-02 18:22:34 PDT

*** Bug 25015 has been marked as a duplicate of this bug. ***

johnnyding

Comment 9 2009-04-04 01:50:10 PDT

Would anyone take a look this bug and patch? Thanks!

johnnyding

Comment 10 2009-04-07 18:35:50 PDT

This patch is simple. I will highly appreciate if someone who has review authority can take a look this patch. Thanks!

Eric Seidel (no email)

Comment 11 2009-04-08 03:54:42 PDT

(In reply to comment #10) > This patch is simple. I will highly appreciate if someone who has review > authority can take a look this patch. > > Thanks! There is no patch on this bug which is marked for review. Please mark your patches r=? if you'd like someone to review them. :) http://webkit.org/coding/contributing.html

Eric Seidel (no email)

Comment 12 2009-04-08 03:56:15 PDT

Comment on attachment 29187 [details] latest V2 patch You ChangeLog entry shoudl also have a description of what you're changing (why your'e making the change, how you're fixing the bug, etc.) and the URL of the bugzilla bug.

johnnyding

Comment 13 2009-04-12 04:48:15 PDT

Created attachment 29419 [details] update patch v2 Thank Eric. I have added the bug URL and simple description for the patch.

Alexey Proskuryakov

Comment 14 2009-05-21 02:47:51 PDT

> ignore the reload request if the isPreload is true when there is one > CachedResource existed for same resource. I don't think that this is correct. When reloading a page with Cmd+R, you need to reload (revalidate) all resources, preloaded or not. This bug might have been fixed in <http://trac.webkit.org/changeset/43831>, but I'm somewhat confused since in that case, it was essential that the document was a result of posting a form.

Maciej Stachowiak

Comment 15 2009-05-22 02:20:53 PDT

It does look to me like Darin's patch should have fixed it. Johnny, can you check if that's the case?

Darin Adler

Comment 16 2009-05-22 09:15:51 PDT

(In reply to comment #15) > It does look to me like Darin's patch should have fixed it. Johnny, can you > check if that's the case? Maciej means Alexey's patch <http://trac.webkit.org/changeset/43831>. I was just the reviewer.

johnnyding

Comment 17 2009-05-22 10:17:48 PDT

Yes, Alexey's patch has fixed it. I will close this bug.

johnnyding

Comment 18 2009-05-22 10:29:05 PDT

(In reply to comment #14) > > ignore the reload request if the isPreload is true when there is one > > CachedResource existed for same resource. > > I don't think that this is correct. When reloading a page with Cmd+R, you need > to reload (revalidate) all resources, preloaded or not. > > This bug might have been fixed in <http://trac.webkit.org/changeset/43831>, but > I'm somewhat confused since in that case, it was essential that the document > was a result of posting a form. > I don't see the original bug related with Alexey's patch. But this case were because WebKit called requestPreload for a preloaded resource in DocLoader::checkForPendingPreloads without checking whether the resource was loaded or not. So yes, Alexey's patch did fix it.

Alexey Proskuryakov

Comment 19 2009-05-22 11:01:03 PDT

We should land a test from this bug - we have very few tests for preloading, so it would be very useful. Would you be willing to prepare a patch with just the test, and submit it for review?

johnnyding

Comment 20 2009-05-23 09:57:50 PDT

Sure, I think the test in my previous patch should be OK. I will extract it and send it as a new patch for test.

johnnyding

Comment 21 2009-05-27 11:37:56 PDT

Created attachment 30712 [details] patch for test of this issue

Alexey Proskuryakov

Comment 22 2009-05-27 12:24:36 PDT

Comment on attachment 30712 [details] patch for test of this issue > + Reviewed by NOBODY (OOPS!). > + https://bugs.webkit.org/show_bug.cgi?id=24747 > + > + Add a test case for multiple requests for same sub-resource due to preload. We usually put an empty line after "Reviewed by" line, not between bug reference and title. > + > + * http\tests\loading\preload-img-test-expected.txt: Added. > + * http\tests\loading\preload-img-test.html: Added. > + * http\tests\loading\resources\preload-test.jpg: Added. > + * http\tests\resources\network-simulator.php: One day we should fix prepare-ChangeLog to not emit Windows paths. > +main frame - didStartProvisionalLoadForFrame > +main frame - didCommitLoadForFrame > +main frame - didFinishDocumentLoadForFrame > +main frame - didHandleOnloadEventsForFrame > +main frame - didFinishLoadForFrame These lines are useless - but I know that they are generated because the test in in a loading/ directory. Maybe we just need a new directory for preloading tests. > + document.getElementById("outputPanel").innerHTML = "FALIED"; Typo: "FAILED". + <span id="outputPanel">PASSED</span> It's not good to have "PASSED" output from the beginning - if the test doesn't run to completion, that could mask the failure. These are all extremely minor nitpicks, just wanted to mention them for the future. No need to fix them to land this patch. It is really great to have a framework for preloading tests, and thanks for refactoring network-simulator! r=me

johnnyding

Comment 23 2009-05-28 17:41:59 PDT

Thanks, Alexey! I keep your comments in mind for future work.

Adam Barth

Comment 24 2009-06-02 00:12:58 PDT

Will land. I'll try to fix some of the nits too.

Adam Barth

Comment 25 2009-06-02 00:28:13 PDT

Sending LayoutTests/ChangeLog Adding LayoutTests/http/tests/loading/preload-img-test-expected.txt Adding LayoutTests/http/tests/loading/preload-img-test.html Adding LayoutTests/http/tests/loading/resources/preload-test.jpg Sending LayoutTests/http/tests/resources/network-simulator.php Transmitting file data ..... Committed revision 44350.

Alexey Proskuryakov

Comment 26 2009-06-03 00:34:13 PDT

Follow-up fix in <http://trac.webkit.org/changeset/44380>.

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 PC

OS Windows Vista

Product WebKit

Component Page Loading

Assignee

Adam Barth

Reported

2009-03-22 06:43 PDT

Modified

2009-06-03 00:34 PDT History

CC List

8 users Show

URL

Keywords

Duplicates (1)

25015 View as bug list

Depends on

Blocks