Summary: | WebCore cache stores duplicate copies of subresources with URL fragments | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ap, buildbot, eric, koivisto | ||||||
Priority: | P2 | Keywords: | HasReduction, InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2010-11-15 10:31:12 PST
Created attachment 73909 [details]
Test case (zip archive)
Created attachment 76566 [details]
patch
- Strip fragment identifiers from HTTP and file URLs for the memory cache.
- Changed some CachedResourceLoader and MemoryCache interfaces to use KURLs
instead of strings to reduce repeated URL parsing.
Comment on attachment 76566 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=76566&action=review r=me. Please double check for invalid URLs being passed around - I don't think that we have a good general policy of who should check for those, so they can creep up. Can any assertions for !hasFragmentIdentifier() be added in code that's behind MemoryCache interface? I think that these could be quite valuable. > WebCore/loader/cache/CachedResource.cpp:119 > - ASSERT(url().isNull() || cache()->resourceForURL(url()) != this); > + ASSERT(url().isNull() || cache()->resourceForURL(KURL(ParsedURLString, url())) != this); Is url() always a valid parsed URL string here? Otherwise, KURL(ParsedURLString, url()) can assert. > WebCore/loader/cache/CachedResourceLoader.cpp:160 > + return cache()->requestUserCSSStyleSheet(this, KURL(ParsedURLString, url), charset); Is url guaranteed to be a parsed URL string? If it is, perhaps the caller could pass a KURL object? This code looks dangerous. Note that we use completeURL() in other CachedResourceLoader methods. > WebCore/loader/cache/MemoryCache.cpp:225 > + if (!originalURL.hasFragmentIdentifier()) > + return originalURL; I would add a comment explaining that invalid URLs are returned unchanged. Perhaps even to a declaration in header, not sure. > WebCore/loader/cache/MemoryCache.cpp:227 > + // Data urls must be unmodified and it is also safer to keep them for custom protocols. Isn't this a bug in data URL parsing? Sounds like these shouldn't have fragments. Attachment 76566 [details] was posted by a committer and has review+, assigning to Antti Koivisto for commit.
(In reply to comment #4) > (From update of attachment 76566 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76566&action=review > > r=me. Please double check for invalid URLs being passed around - I don't think that we have a good general policy of who should check for those, so they can creep up. Ok. > Can any assertions for !hasFragmentIdentifier() be added in code that's behind MemoryCache interface? I think that these could be quite valuable. I wouldn't be surprised if there were WebKit clients using custom protocols that expect to see everything in the request URL. To be safe, this patch still allow frag ids in the cache so simple !hasFragmentIdentifier() would not work. I'll see if there is some other asserts that could be useful. > > WebCore/loader/cache/CachedResource.cpp:119 > > - ASSERT(url().isNull() || cache()->resourceForURL(url()) != this); > > + ASSERT(url().isNull() || cache()->resourceForURL(KURL(ParsedURLString, url())) != this); > > Is url() always a valid parsed URL string here? Otherwise, KURL(ParsedURLString, url()) can assert. If not that is likely to be a bug that we want to catch. I was thinking of switching CachedResource to KURL too later. > > WebCore/loader/cache/CachedResourceLoader.cpp:160 > > + return cache()->requestUserCSSStyleSheet(this, KURL(ParsedURLString, url), charset); > > Is url guaranteed to be a parsed URL string? If it is, perhaps the caller could pass a KURL object? > > This code looks dangerous. Note that we use completeURL() in other CachedResourceLoader methods. It seems it will only ever gets invoked if you have a user stylesheets with @import rules. I like that code ends up guaranteeing that the url has been parsed and resolved absolute, though it is rather messy. I'll see about passing KURL. > > WebCore/loader/cache/MemoryCache.cpp:227 > > + // Data urls must be unmodified and it is also safer to keep them for custom protocols. > > Isn't this a bug in data URL parsing? Sounds like these shouldn't have fragments. Yeah, I think so. Data and javascript URLs (and maybe others) should never have fragment identifiers. That seemed like something better fixed separately as it might expand quite a bit. Attachment 76566 [details] did not build on win: Build output: http://queues.webkit.org/results/7118009 |