RESOLVED FIXED 113626
Resources from non-HTTP schemes should not be cached indefinitely
https://bugs.webkit.org/show_bug.cgi?id=113626
Summary Resources from non-HTTP schemes should not be cached indefinitely
Andy Estes
Reported 2013-03-29 16:23:21 PDT
Let cached resources from file: schemes expire immediately
Attachments
Patch (2.34 KB, patch)
2013-03-29 16:29 PDT, Andy Estes
no flags
Patch (47.80 KB, patch)
2013-05-15 18:23 PDT, Andy Estes
rniwa: review+
Andy Estes
Comment 1 2013-03-29 16:29:57 PDT
Andy Estes
Comment 2 2013-03-29 16:30:30 PDT
Andy Estes
Comment 3 2013-03-29 17:02:51 PDT
WebKit Review Bot
Comment 4 2013-03-29 18:58:31 PDT
Re-opened since this is blocked by bug 113632
Andy Estes
Comment 5 2013-03-29 20:19:23 PDT
(In reply to comment #4) > Re-opened since this is blocked by bug 113632 This failure is expected given how the test is written. This needs to be an http test to verify caching behavior. I'll fix it.
Andy Estes
Comment 6 2013-05-15 18:23:21 PDT
Ryosuke Niwa
Comment 7 2013-05-15 19:38:38 PDT
Comment on attachment 201905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201905&action=review Why don’t you use the short format: https://webkit.org/b/116199 > Source/WebCore/ChangeLog:12 > + Writing a test for this is blocked on <https://bugs.webkit.org/show_bug.cgi?id=116199>. Why don’t you use the short format: https://webkit.org/b/116199 and we don’t need <> around the url.
Ryosuke Niwa
Comment 8 2013-05-15 19:41:08 PDT
Comment on attachment 201905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201905&action=review > Source/WebCore/platform/SchemeRegistry.cpp:130 > +#if PLATFORM(MAC) > + indefinitelyCacheableSchemes.add("applewebdata"); > +#endif > + indefinitelyCacheableSchemes.add("blob"); > + indefinitelyCacheableSchemes.add("data"); Since there are only 3 items, it’s probably faster to string compare instead as in scheme == “applewebdata” || scheme == “blob” || scheme == “data”.
Andy Estes
Comment 9 2013-05-15 19:53:30 PDT
Alexey Proskuryakov
Comment 10 2013-05-15 21:40:05 PDT
Comment on attachment 201905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201905&action=review > Source/WebCore/platform/SchemeRegistry.cpp:129 > + indefinitelyCacheableSchemes.add("blob"); This is not correct. When a blob is backed by a file, and the file changes, loading form the blob should return an error.
Filip Pizlo
Comment 11 2013-05-25 18:39:10 PDT
This is a *MASSIVE* regression in script caching performance. SunSpider, V8Spider, and JSBench all get a lot slower. I've narrowed it down to this revision.
Ryosuke Niwa
Comment 12 2013-05-25 18:44:28 PDT
(In reply to comment #11) > This is a *MASSIVE* regression in script caching performance. SunSpider, V8Spider, and JSBench all get a lot slower. I've narrowed it down to this revision. Oops!
Filip Pizlo
Comment 13 2013-05-25 18:45:37 PDT
(In reply to comment #12) > (In reply to comment #11) > > This is a *MASSIVE* regression in script caching performance. SunSpider, V8Spider, and JSBench all get a lot slower. I've narrowed it down to this revision. > > Oops! Wait, I'm still confirming. It looks like it regresses performance with how we measure in DumpRenderTree, but it's not clearly a regression in browser. I'm investigating...
Filip Pizlo
Comment 14 2013-05-25 18:51:56 PDT
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > This is a *MASSIVE* regression in script caching performance. SunSpider, V8Spider, and JSBench all get a lot slower. I've narrowed it down to this revision. > > > > Oops! > > Wait, I'm still confirming. It looks like it regresses performance with how we measure in DumpRenderTree, but it's not clearly a regression in browser. > > I'm investigating... Yeah, this is DRT-only.
Filip Pizlo
Comment 15 2013-05-25 18:52:55 PDT
Comment on attachment 201905 [details] Patch View in context: http://bugs.webkit.org/attachment.cgi?id=201905&action=review > Source/WebCore/loader/cache/CachedResource.cpp:423 > + // Don't cache other non-HTTP resources since we can't check for freshness. > + if (!m_response.url().protocolIsInHTTPFamily()) > + return 0; What effect does this have on file://?
Andy Estes
Comment 16 2013-05-25 21:53:07 PDT
(In reply to comment #15) > (From update of attachment 201905 [details]) > View in context: http://bugs.webkit.org/attachment.cgi?id=201905&action=review > > > Source/WebCore/loader/cache/CachedResource.cpp:423 > > + // Don't cache other non-HTTP resources since we can't check for freshness. > > + if (!m_response.url().protocolIsInHTTPFamily()) > > + return 0; > > What effect does this have on file://? The effect is that file: resources in the memory cache expire immediately, so subsequent loads will hit the filesystem. This change was made to mitigate the compatibility fallout from caching main resources. Some WebKit clients that we can't break update their UI by overwriting a file on disk and reloading their WebView, so we can't assume that file: main resources can be cached indefinitely. And since many custom schemes are backed by files, we can't assume if for those either. However, I think this change has had a significant impact on our testing infrastructure (there's also been some additional test flakiness) and so we might want to consider a different approach. One solution would be to implement a freshness check for files, but that would involve calling stat() on each memory cache load, which isn't cheap and might even hang the main thread on a network filesystem. The other solution might be to limit this change to the specific apps that we know need it for correctness.
Ryosuke Niwa
Comment 17 2013-05-25 21:59:00 PDT
(In reply to comment #16) > One solution would be to implement a freshness check for files, but that would involve calling stat() on each memory cache load, which isn’t cheap and might even hang the main thread on a network filesystem. Can’t we do this only for local (file protocol) resources? > The other solution might be to limit this change to the specific apps that we know need it for correctness. Maybe we can add a setting to turn this on/off?
Filip Pizlo
Comment 18 2013-05-25 22:16:43 PDT
(In reply to comment #16) > (In reply to comment #15) > > (From update of attachment 201905 [details] [details]) > > View in context: http://bugs.webkit.org/attachment.cgi?id=201905&action=review > > > > > Source/WebCore/loader/cache/CachedResource.cpp:423 > > > + // Don't cache other non-HTTP resources since we can't check for freshness. > > > + if (!m_response.url().protocolIsInHTTPFamily()) > > > + return 0; > > > > What effect does this have on file://? > > The effect is that file: resources in the memory cache expire immediately, so subsequent loads will hit the filesystem. > > This change was made to mitigate the compatibility fallout from caching main resources. Some WebKit clients that we can't break update their UI by overwriting a file on disk and reloading their WebView, so we can't assume that file: main resources can be cached indefinitely. And since many custom schemes are backed by files, we can't assume if for those either. > > However, I think this change has had a significant impact on our testing infrastructure (there's also been some additional test flakiness) and so we might want to consider a different approach. What about giving the test infrastructure a testFile:// protocol that works like file:// did before? > > One solution would be to implement a freshness check for files, but that would involve calling stat() on each memory cache load, which isn't cheap and might even hang the main thread on a network filesystem. > > The other solution might be to limit this change to the specific apps that we know need it for correctness.
Andy Estes
Comment 19 2013-05-25 23:30:08 PDT
(In reply to comment #17) > (In reply to comment #16) > > One solution would be to implement a freshness check for files, but that would involve calling stat() on each memory cache load, which isn’t cheap and might even hang the main thread on a network filesystem. > > Can’t we do this only for local (file protocol) resources? Well, file: URLs can still reference network resources (e.g. file:///Volumes/SomeAFPMount/...). > > > The other solution might be to limit this change to the specific apps that we know need it for correctness. > > Maybe we can add a setting to turn this on/off? Yeah, this is a good idea. That lets us turn the new behavior on/off per-client, and it sounds like DRT/WKTR/TestWebKitAPI will want to turn it off.
Andy Estes
Comment 20 2013-05-25 23:35:38 PDT
(In reply to comment #18) > (In reply to comment #16) > > (In reply to comment #15) > > > (From update of attachment 201905 [details] [details] [details]) > > > View in context: http://bugs.webkit.org/attachment.cgi?id=201905&action=review > > > > > > > Source/WebCore/loader/cache/CachedResource.cpp:423 > > > > + // Don't cache other non-HTTP resources since we can't check for freshness. > > > > + if (!m_response.url().protocolIsInHTTPFamily()) > > > > + return 0; > > > > > > What effect does this have on file://? > > > > The effect is that file: resources in the memory cache expire immediately, so subsequent loads will hit the filesystem. > > > > This change was made to mitigate the compatibility fallout from caching main resources. Some WebKit clients that we can't break update their UI by overwriting a file on disk and reloading their WebView, so we can't assume that file: main resources can be cached indefinitely. And since many custom schemes are backed by files, we can't assume if for those either. > > > > However, I think this change has had a significant impact on our testing infrastructure (there's also been some additional test flakiness) and so we might want to consider a different approach. > > What about giving the test infrastructure a testFile:// protocol that works like file:// did before? I actually really like this idea, because it makes better sense to think about all this in terms of schemes. However, I don't think we have the concept of aliasing file: to another scheme in WebCore, so it'd require a port-by-port effort to implement the testFile: scheme handler.
Ryosuke Niwa
Comment 21 2013-05-25 23:49:19 PDT
(In reply to comment #20) > (In reply to comment #18) > > (In reply to comment #16) > > > > What about giving the test infrastructure a testFile:// protocol that works like file:// did before? > > I actually really like this idea, because it makes better sense to think about all this in terms of schemes. However, I don't think we have the concept of aliasing file: to another scheme in WebCore, so it'd require a port-by-port effort to implement the testFile: scheme handler. For that reason, I prefer adding settings because we can easily override settings via internals.settings.*.
Andy Estes
Comment 22 2013-05-25 23:59:10 PDT
(In reply to comment #21) > (In reply to comment #20) > > (In reply to comment #18) > > > (In reply to comment #16) > > > > > > What about giving the test infrastructure a testFile:// protocol that works like file:// did before? > > > > I actually really like this idea, because it makes better sense to think about all this in terms of schemes. However, I don't think we have the concept of aliasing file: to another scheme in WebCore, so it'd require a port-by-port effort to implement the testFile: scheme handler. > > For that reason, I prefer adding settings because we can easily override settings via internals.settings.*. I agree. I can do this now.
Alexey Proskuryakov
Comment 23 2013-05-26 00:11:56 PDT
> Some WebKit clients that we can't break update their UI by overwriting a file on disk and reloading their WebView Are they actually doing a reload? Because supporting that does not necessarily mean never caching any non-http resources. We can differentiate between normal loads and reloads.
Andy Estes
Comment 24 2013-05-29 13:50:36 PDT
(In reply to comment #23) > > Some WebKit clients that we can't break update their UI by overwriting a file on disk and reloading their WebView > > Are they actually doing a reload? Because supporting that does not necessarily mean never caching any non-http resources. We can differentiate between normal loads and reloads. No, not an actual reload in the API sense. Just loading the same main resource again.
Andy Estes
Comment 25 2013-05-29 13:52:06 PDT
(In reply to comment #11) > This is a *MASSIVE* regression in script caching performance. SunSpider, V8Spider, and JSBench all get a lot slower. I've narrowed it down to this revision. <http://trac.webkit.org/changeset/150863> should have fixed the regression.
Ryosuke Niwa
Comment 26 2013-05-30 12:37:11 PDT
Note You need to log in before you can comment on or make changes to this bug.