REGRESSION: Reloading a local file doesn't pick up changes

• Component: Page Loading
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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.

[Radar WebKit Bug Importer]

<rdar://problem/13186490>

[Brady Eidson]

If the user refreshes the whole page, we *MUST* ignore the cached main resource and load from scratch.

[Nate Chapin]

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

[Nate Chapin]

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. :(

[Alexey Proskuryakov]

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.

[Nate Chapin]

Created attachment 187910 [details]
with test this time

[Alexey Proskuryakov]

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?

[Nate Chapin]

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

[Alexey Proskuryakov]

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 :)

[Nate Chapin]

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

[Nate Chapin]

Created attachment 187979 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 187979 [details]
Patch for landing

Clearing flags on attachment: 187979

Committed r142707: <http://trac.webkit.org/changeset/142707>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

With r142758 I still see that reloading doesn't pick up changes to a local file.

[Simon Fraser (smfr)]

I was confused by the inspector. It seems to be working.