Bug 61318

Created attachment 94513 [details] patch

Comment on attachment 94513 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=94513&action=review I really like this direction. Some minor questions / comments below. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:135 > - KURL completeURL = m_document->completeURL(url); > + KURL completeURL = m_document->completeURL(request.url()); Presumably request.url() is already complete, right? > Source/WebCore/loader/cache/CachedResourceLoader.cpp:341 > + if (request.url() != url) > + request.setURL(url); This basically has the effect of stripping the fragment identifier, right? > Source/WebCore/loader/cache/CachedResourceLoader.cpp:-373 > - CachedResource* newResource = createResource(resource->type(), KURL(ParsedURLString, url), resource->encoding()); Yay. Every time you remove ParsedURLString, good things happen. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:683 > - CachedResource* resource = requestResource(type, url, encoding, ResourceLoadPriorityUnresolved, true); > + ResourceRequest request(m_document->completeURL(url)); > + CachedResource* resource = requestResource(type, request, encoding, ResourceLoadPriorityUnresolved, true); Here the caller isn't supposed to provide a ResourceRequest? > Source/WebCore/loader/cache/CachedImage.cpp:228 > - m_shouldPaintBrokenImage = frame->loader()->client()->shouldPaintBrokenImage(KURL(ParsedURLString, m_url)); > + m_shouldPaintBrokenImage = frame->loader()->client()->shouldPaintBrokenImage(KURL(ParsedURLString, m_resourceRequest.url())); Can't we get rid of the ParsedURLString here? m_resourceRequest.url() should already be a KURL, right? > Source/WebCore/loader/cache/CachedResourceLoader.h:45 > +class CachedRawResource; CachedRawResource <-- That's from the future, right? > Source/WebCore/loader/cache/CachedResource.cpp:268 > +static bool reuseRequest(const ResourceRequest& request) reuseRequest => canReuseRequest ? > Source/WebCore/loader/cache/CachedResource.cpp:281 > +// FIXME: This doesn't handle Access-Control headers yet, but when it does, it will need the > +// currently unused Document* to get a SecurityOrigin. > +bool CachedResource::allowReuseOfRequest(Document*, const ResourceRequest& newRequest) const I don't fully understand what this function does. It seems like these cases can't occur yet, right? > Source/WebCore/loader/cache/CachedResource.h:99 > + const String& url() const { return m_resourceRequest.url();} Oh, I see. This returns a string. Can we return the underlying KURL from m_resourceRequest instead?

Comment on attachment 94513 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=94513&action=review > Source/WebCore/loader/cache/CachedResource.h:51 > +class ResourceRequest; > +class ResourceResponse; You are adding both a forward declaration and an include of ResourceRequest.h. And there has already been an include of ResourceResponse. > Source/WebCore/loader/cache/CachedResource.h:85 > - CachedResource(const String& url, Type); > + CachedResource(ResourceRequest, Type); Does it need to be copied? > Source/WebCore/loader/ImageLoader.cpp:172 > + ResourceRequest request(document->completeURL(sourceURI(attr))); > + newImage = document->cachedResourceLoader()->requestImage(request); Here and elsewhere, we may be constructing ResourceRequest from invalid URLs. I'm not sure if it's so great - perhaps we should have an assertion in ResourceRequest constructor. I don't think if we have a clear design of who checks for invalid URLs.

(In reply to comment #2) > (From update of attachment 94513 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94513&action=review > > Source/WebCore/loader/cache/CachedResource.cpp:268 > > +static bool reuseRequest(const ResourceRequest& request) > > reuseRequest => canReuseRequest ? > > > Source/WebCore/loader/cache/CachedResource.cpp:281 > > +// FIXME: This doesn't handle Access-Control headers yet, but when it does, it will need the > > +// currently unused Document* to get a SecurityOrigin. > > +bool CachedResource::allowReuseOfRequest(Document*, const ResourceRequest& newRequest) const > > I don't fully understand what this function does. It seems like these cases can't occur yet, right? Yeah, I'm going to remove these two functions from this version, since they won't do anything meaningful yet. (In reply to comment #3) > (From update of attachment 94513 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94513&action=review > > > Source/WebCore/loader/cache/CachedResource.h:85 > > - CachedResource(const String& url, Type); > > + CachedResource(ResourceRequest, Type); > > Does it need to be copied? CachedResource needs to make a copy internally, but that's a red herring. I had left this as a copy because there's one subclass constructor CachedImage(Image*) that doesn't take a ResourceRequest and I didn't see an obvious way to get it to play nicely with a ResourceRequest& (probably a failure of my C++ knowledge). Alternatively, I can just make this constructor take a dummy ResourceRequest. > > > Source/WebCore/loader/ImageLoader.cpp:172 > > + ResourceRequest request(document->completeURL(sourceURI(attr))); > > + newImage = document->cachedResourceLoader()->requestImage(request); > > Here and elsewhere, we may be constructing ResourceRequest from invalid URLs. I'm not sure if it's so great - perhaps we should have an assertion in ResourceRequest constructor. > > I don't think if we have a clear design of who checks for invalid URLs. When I passed an incomplete URL to ResourceRequest's constructor, it ASSERTed in KURL due to the url being invalid. Is that sufficient?

> I didn't see an obvious way to get it to play nicely with a ResourceRequest& You can't bind a temporary to a non-const reference, but a const reference should be OK. > When I passed an incomplete URL to ResourceRequest's constructor, it ASSERTed in KURL due to the url being invalid. Is that sufficient? What was the assertion? There is none in KURL::string(), and relying on some other method being called and asserting could be weak.

> > When I passed an incomplete URL to ResourceRequest's constructor, it ASSERTed in KURL due to the url being invalid. Is that sufficient? Yeah, it should be. Doh. > > What was the assertion? There is none in KURL::string(), and relying on some other method being called and asserting could be weak. The interesting bit of the stack: ASSERTION FAILED: !url.length() || isSchemeFirstChar(url[0]) /Volumes/MacintoshHD2/src/webkit3/Source/WebCore/platform/KURL.cpp(292) : void WebCore::checkEncodedString(const WTF::String&) 1 WebCore::checkEncodedString(WTF::String const&) 2 WebCore::KURL::parse(WTF::String const&) 3 WebCore::KURL::KURL(WebCore::ParsedURLStringTag, WTF::String const&) 4 WebCore::ResourceRequest::ResourceRequest(WTF::String const&)

> 2 WebCore::KURL::parse(WTF::String const&) > 3 WebCore::KURL::KURL(WebCore::ParsedURLStringTag, WTF::String const&) > 4 WebCore::ResourceRequest::ResourceRequest(WTF::String const&) So, a string version of the constructor is used here, meaning that the KURL is converted to string and then back to KURL. This is certainly not how it should be! Relying on this bug for diagnostics is not right. In the code that I quoted originally, we directly pass a KURL from completeURL. Hopefully, we don't create another ResourceRequest from scratch with a stringified URL later? > > + ResourceRequest request(document->completeURL(sourceURI(attr))); > > + newImage = document->cachedResourceLoader()->requestImage(request);

(In reply to comment #7) > > 2 WebCore::KURL::parse(WTF::String const&) > > 3 WebCore::KURL::KURL(WebCore::ParsedURLStringTag, WTF::String const&) > > 4 WebCore::ResourceRequest::ResourceRequest(WTF::String const&) > > So, a string version of the constructor is used here, meaning that the KURL is converted to string and then back to KURL. This is certainly not how it should be! Relying on this bug for diagnostics is not right. With a few extra lines this time, sorry for cutting the stack too soon: ASSERTION FAILED: !url.length() || isSchemeFirstChar(url[0]) /Volumes/MacintoshHD2/src/webkit3/Source/WebCore/platform/KURL.cpp(292) : void WebCore::checkEncodedString(const WTF::String&) 1 WebCore::checkEncodedString(WTF::String const&) 2 WebCore::KURL::parse(WTF::String const&) 3 WebCore::KURL::KURL(WebCore::ParsedURLStringTag, WTF::String const&) 4 WebCore::ResourceRequest::ResourceRequest(WTF::String const&) 5 WebCore::ScriptElement::requestScript(WTF::String const&) 6 WebCore::ScriptElement::prepareScript(WTF::TextPosition<WTF::OneBasedNumber> const&, WebCore::ScriptElement::LegacyTypeSupport) 7 WebCore::HTMLScriptRunner::runScript(WebCore::Element*, WTF::TextPosition<WTF::OneBasedNumber> const&) 8 WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element>, WTF::TextPosition<WTF::OneBasedNumber> const&) We don't appear to be doing a KURL->String->KURL conversion here, this is where we do the initial String->KURL conversion (but we get this stack with the completeURL() omitted). > > In the code that I quoted originally, we directly pass a KURL from completeURL. Hopefully, we don't create another ResourceRequest from scratch with a stringified URL later? > > > > + ResourceRequest request(document->completeURL(sourceURI(attr))); > > > + newImage = document->cachedResourceLoader()->requestImage(request); We should just be copying ResourceRequests once we enter CachedResourceLoader.

Created attachment 94629 [details] patch #2 Ok, I think I've addressed all comments. Please let me know if I wasn't clear (or was obviously wrong) in my explanation of what we appear to be doing in the String->KURL->ResourceRequest(KURL&) flow.

Comment on attachment 94629 [details] patch #2 View in context: https://bugs.webkit.org/attachment.cgi?id=94629&action=review > Source/WebCore/svg/SVGFEImageElement.cpp:76 > - m_cachedImage = ownerDocument()->cachedResourceLoader()->requestImage(href()); > + ResourceRequest request(ownerDocument()->completeURL(href())); > + m_cachedImage = ownerDocument()->cachedResourceLoader()->requestImage(request); Was there some particular reason to change CachedResourceLoader::requestSomeConcreteType() methods take ResourceRequest too? This complicates the call sites.

(In reply to comment #10) > (From update of attachment 94629 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94629&action=review > > > Source/WebCore/svg/SVGFEImageElement.cpp:76 > > - m_cachedImage = ownerDocument()->cachedResourceLoader()->requestImage(href()); > > + ResourceRequest request(ownerDocument()->completeURL(href())); > > + m_cachedImage = ownerDocument()->cachedResourceLoader()->requestImage(request); > > Was there some particular reason to change CachedResourceLoader::requestSomeConcreteType() methods take ResourceRequest too? This complicates the call sites. We're going to use at least the image call sites for Bug 61015

Comment on attachment 94629 [details] patch #2 View in context: https://bugs.webkit.org/attachment.cgi?id=94629&action=review > Source/WebCore/loader/cache/CachedResourceLoader.h:67 > + CachedImage* requestImage(ResourceRequest&); > + CachedCSSStyleSheet* requestCSSStyleSheet(ResourceRequest&, const String& charset, ResourceLoadPriority = ResourceLoadPriorityUnresolved); > + CachedCSSStyleSheet* requestUserCSSStyleSheet(ResourceRequest&, const String& charset); > + CachedScript* requestScript(ResourceRequest&, const String& charset); > + CachedFont* requestFont(ResourceRequest&); It's slightly subtle that these take non-const references. It's not super intuitive that issuing the resource request can change the request itself.

(In reply to comment #10) > (From update of attachment 94629 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94629&action=review > > > Source/WebCore/svg/SVGFEImageElement.cpp:76 > > - m_cachedImage = ownerDocument()->cachedResourceLoader()->requestImage(href()); > > + ResourceRequest request(ownerDocument()->completeURL(href())); > > + m_cachedImage = ownerDocument()->cachedResourceLoader()->requestImage(request); > > Was there some particular reason to change CachedResourceLoader::requestSomeConcreteType() methods take ResourceRequest too? This complicates the call sites. The fundamental problem is that when calling CachedResourceLoader::requestSomeConcreteType(), the request will either fail or get sent to ResourceLoadScheduler before the function returns, so if a callsite needs to have custom headers, they need to be set before calling requestSomeConcreteType(). In addition to this implementation, we could have two version of each requestSomeConreteType(), one that takes a String and one that takes a ResourceRequest. That seems sort of nasty. I suppose we could also leave them as String until we need to switch any given one to ResourceRequest, but I don't like being inconsistent between types.

Comment on attachment 94629 [details] patch #2 I agree that this makes the call sites slightly more verbose, but that seems better than having some of functions use URLs and others use ResourceRequests. We can either to the API conversion in this patch or the next patch, but doing it in this patch is probably best since this patch shouldn't have any behavior changes and the next patches all introduce new behavior.

I don't think that I've taken my point across well enough. Let's look at the same code snippet that I quoted before: > + ResourceRequest request(document->completeURL(sourceURI(attr))); > + newImage = document->cachedResourceLoader()->requestImage(request); What is going to assert if the URL is invalid? Even more importantly, what is going to happen at runtime if it is invalid? Will we be making client calls with an invalid URL, will we pipe it down to network stack, potentially even loading something if it has a different idea of invalid URL? You said that you had an assertion failure, but that was some entirely different code path.

Comment on attachment 94629 [details] patch #2 Feel free to cq+ back if you think this should be landed as is - but I'd like to take initiative away from robot hands.

(In reply to comment #15) > I don't think that I've taken my point across well enough. Let's look at the same code snippet that I quoted before: > > > + ResourceRequest request(document->completeURL(sourceURI(attr))); > > + newImage = document->cachedResourceLoader()->requestImage(request); > > What is going to assert if the URL is invalid? Even more importantly, what is going to happen at runtime if it is invalid? Will we be making client calls with an invalid URL, will we pipe it down to network stack, potentially even loading something if it has a different idea of invalid URL? > > You said that you had an assertion failure, but that was some entirely different code path. We should still be able to make use of the url validity check in CachedResourceLoadeer::requestResource() (http://trac.webkit.org/browser/trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp#L320). That check is now using the KURL from the ResourceRequest (with fragments removed). So I think that even if we do end up with an invalid KURL on a ResourceRequest, we'll fail in CachedResourceLoader and exit before it goes to the network stack. Did I understand this time?

That makes sense. Not all requests go through CachedResourceLoader::requestResource() though. I think that it would be better if we could assert that the URL is valid in ResourceRequest constructor, so that they wouldn't enter low level loader interface at all. There is never a reason to actually have an invalid URL in ResourceRequest, right?

> Not all requests go through CachedResourceLoader::requestResource() though. I think that it would be better if we could assert that the URL is valid in ResourceRequest constructor, so that they wouldn't enter low level loader interface at all. There is never a reason to actually have an invalid URL in ResourceRequest, right? Alexey, if it's ok with you, I'd like to land this patch to unblock the other three bugs. I'll post a patch adding the ASSERT shortly thereafter.

Comment on attachment 94629 [details] patch #2 After discussing this on #webkit, it sounds like we probably have enough guarantees of the validity of the URL. ResourceHandle's constructor guarantees we won't go to the network stack with an invalid url, and it was already possible to have a ResourceRequest with an invalid KURL via its 0-argument constructor.

Comment on attachment 94629 [details] patch #2 Clearing flags on attachment: 94629 Committed r87239: <http://trac.webkit.org/changeset/87239>

All reviewed patches have been landed. Closing bug.