[Webkit-unassigned] [Bug 49548] WebCore cache stores duplicate copies of subresources with URL fragments

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=49548


Antti Koivisto <koivisto at iki.fi> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|koivisto at iki.fi             |webkit-unassigned at lists.web
                   |                            |kit.org




--- Comment #6 from Antti Koivisto <koivisto at iki.fi>  2010-12-14 15:42:36 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.