[webkit-reviews] review granted: [Bug 49548] WebCore cache stores duplicate copies of subresources with URL fragments : [Attachment 76566] patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Antti Koivisto
<koivisto at iki.fi>'s request for review:
Bug 49548: WebCore cache stores duplicate copies of subresources with URL
fragments
https://bugs.webkit.org/show_bug.cgi?id=49548

Attachment 76566: patch
https://bugs.webkit.org/attachment.cgi?id=76566&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.