Bug 109344 - REGRESSION: Reloading a local file doesn't pick up changes
Summary: REGRESSION: Reloading a local file doesn't pick up changes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Critical
Assignee: Nate Chapin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-02-08 21:43 PST by Ryosuke Niwa
Modified: 2013-02-13 11:58 PST (History)
7 users (show)

See Also:


Attachments
patch (7.71 KB, patch)
2013-02-12 10:23 PST, Nate Chapin
no flags Details | Formatted Diff | Diff
with test this time (10.46 KB, patch)
2013-02-12 12:23 PST, Nate Chapin
no flags Details | Formatted Diff | Diff
Patch for landing (10.74 KB, patch)
2013-02-12 17:41 PST, Nate Chapin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-02-08 21:43:29 PST
Reproduction steps:
1. Create a local .html file
2. Open it in the nightly builds of WebKit
3. Modify the local file.
4. Reload

Expected result:
The modified file is loaded

Actual result:
The file is never reloaded.

This is probably caused by http://trac.webkit.org/changeset/142024.
Comment 1 Radar WebKit Bug Importer 2013-02-08 21:53:03 PST
<rdar://problem/13186490>
Comment 2 Brady Eidson 2013-02-09 10:35:43 PST
If the user refreshes the whole page, we *MUST* ignore the cached main resource and load from scratch.
Comment 3 Nate Chapin 2013-02-11 11:22:07 PST
(In reply to comment #2)
> If the user refreshes the whole page, we *MUST* ignore the cached main resource and load from scratch.

This appears to be an errant dependence on FrameLoader::subresourceCachePolicy() for main resource loads. Working on it.
Comment 4 Nate Chapin 2013-02-12 10:23:04 PST
Created attachment 187891 [details]
patch

I tried and failed to find a suitable layout tests for this.  Finding a suitable artifact of memory cache hits is hard. :(
Comment 5 Alexey Proskuryakov 2013-02-12 10:41:12 PST
Did this only affect local files, or could it be reproduced with http-served ones with appropriate header fields? WE have a number of tests that change content of a resource between http loads.
Comment 6 Nate Chapin 2013-02-12 12:23:28 PST
Created attachment 187910 [details]
with test this time
Comment 7 Alexey Proskuryakov 2013-02-12 15:08:42 PST
Comment on attachment 187910 [details]
with test this time

View in context: https://bugs.webkit.org/attachment.cgi?id=187910&action=review

Looks good to me, but I don't understand the change to existing test results.

> LayoutTests/http/tests/misc/favicon-loads-with-images-disabled-expected.txt:-4
> -http://127.0.0.1:8000/misc/favicon-loads-with-images-disabled.html - didFinishLoading

Why is it OK to no longer receive didFinishLoading? In ChangeLog, you say that this test was being loaded from memory cache, but I don't understand why fixing that removes a didFinishLoading delegate call.

> LayoutTests/http/tests/cache/reload-main-resource.php:4
> +if (file_exists(sys_get_temp_dir() . "/cache.tmp"))

Can this file be named in a way that makes it clear which test it belongs to?
Comment 8 Nate Chapin 2013-02-12 15:14:48 PST
(In reply to comment #7)
> (From update of attachment 187910 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187910&action=review
> 
> Looks good to me, but I don't understand the change to existing test results.
> 
> > LayoutTests/http/tests/misc/favicon-loads-with-images-disabled-expected.txt:-4
> > -http://127.0.0.1:8000/misc/favicon-loads-with-images-disabled.html - didFinishLoading
> 
> Why is it OK to no longer receive didFinishLoading? In ChangeLog, you say that this test was being loaded from memory cache, but I don't understand why fixing that removes a didFinishLoading delegate call.

The timing of memory cache hit callbacks for main resources is slightly earlier than for normal resource loads. I wasn't able to match it exactly. The difference in ordering is most obvious in this diff from enabling cache reuse: http://trac.webkit.org/changeset/141136/trunk/LayoutTests/http/tests/loading/redirect-methods-expected.txt

In the case of the 2 results that change in this patch, the didFinishLoading call was added in http://trac.webkit.org/changeset/141136, where the callback moved from being just after dump to just before dump. This reverts that change, as it appears to depend on erroneous caching in the testRunner.queueReload() case.


> 
> > LayoutTests/http/tests/cache/reload-main-resource.php:4
> > +if (file_exists(sys_get_temp_dir() . "/cache.tmp"))
> 
> Can this file be named in a way that makes it clear which test it belongs to?

Sure, will fix.
Comment 9 Alexey Proskuryakov 2013-02-12 15:39:15 PST
I see, so what you are saying is that these particular tests just suffer from poor interaction between queueReload() and callback dumping? Tests generally shouldn't finish before their content loads :)
Comment 10 Nate Chapin 2013-02-12 17:14:18 PST
(In reply to comment #9)
> I see, so what you are saying is that these particular tests just suffer from poor interaction between queueReload() and callback dumping? Tests generally shouldn't finish before their content loads :)

I think this is a general problem with the few tests that uses dumpResourceLoadCallbacks() but doesn't use waitUntilDone(). DRT's dump keys off of some callback that happens just before didFinishLoading() in the normal case.
Comment 11 Nate Chapin 2013-02-12 17:41:44 PST
Created attachment 187979 [details]
Patch for landing
Comment 12 WebKit Review Bot 2013-02-12 18:43:41 PST
Comment on attachment 187979 [details]
Patch for landing

Clearing flags on attachment: 187979

Committed r142707: <http://trac.webkit.org/changeset/142707>
Comment 13 WebKit Review Bot 2013-02-12 18:43:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Simon Fraser (smfr) 2013-02-13 11:56:11 PST
With r142758 I still see that reloading doesn't pick up changes to a local file.
Comment 15 Simon Fraser (smfr) 2013-02-13 11:58:30 PST
I was confused by the inspector. It seems to be working.