WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(47.80 KB, patch)
2013-05-15 18:23 PDT
,
Andy Estes
rniwa
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Andy Estes
Comment 1
2013-03-29 16:29:57 PDT
Created
attachment 195817
[details]
Patch
Andy Estes
Comment 2
2013-03-29 16:30:30 PDT
<
rdar://problem/13313769
>
Andy Estes
Comment 3
2013-03-29 17:02:51 PDT
Committed
r147263
: <
http://trac.webkit.org/changeset/147263
>
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
Created
attachment 201905
[details]
Patch
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
Committed
r150169
: <
http://trac.webkit.org/changeset/150169
>
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
http://trac.webkit.org/changeset/150863
fixed this problem.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug