Resources from non-HTTP schemes should not be cached indefinitely

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

Let cached resources from file: schemes expire immediately

[Andy Estes]

Created attachment 195817 [details]
Patch

[Andy Estes]

<rdar://problem/13313769>

[Andy Estes]

Committed r147263: <http://trac.webkit.org/changeset/147263>

[WebKit Review Bot]

Re-opened since this is blocked by bug 113632

[Andy Estes]

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

Created attachment 201905 [details]
Patch

[Ryosuke Niwa]

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 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]

Committed r150169: <http://trac.webkit.org/changeset/150169>

[Alexey Proskuryakov]

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]

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]

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

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

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

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

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

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

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

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

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

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

> 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]

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

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

http://trac.webkit.org/changeset/150863 fixed this problem.