Cache should not be the component that initiates loads. MemoryCache::requestResource and similar should be moved/merged to CachedResourceLoader. This is likely to simplify the code too.
Created attachment 76707 [details] refactor loading code out of MemoryCache Move loading related code from MemoryCache to CachedResourceLoader - Merge MemoryCache::requestResource to CachedResourceLoader::requestResource - Merge MemoryCache::requestUserCSSStyleSheet to CachedResourceLoader::requestUserCSSStyleSheet - Move MemoryCache::revalidateResource to CachedResourceLoader::revalidateResource - Add MemoryCache::add - Refactor the decision on whether to reload, revalidate or use the existing resource to a single function, CachedResourceLoader::determineRevalidationPolicy
Attachment 76707 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/css/CSSImageValue.cpp', u'WebCore/loader/ImageLoader.cpp', u'WebCore/loader/cache/CachedImage.cpp', u'WebCore/loader/cache/CachedResource.cpp', u'WebCore/loader/cache/CachedResource.h', u'WebCore/loader/cache/CachedResourceLoader.cpp', u'WebCore/loader/cache/CachedResourceLoader.h', u'WebCore/loader/cache/MemoryCache.cpp', u'WebCore/loader/cache/MemoryCache.h']" exit_code: 1 WebCore/loader/cache/CachedResourceLoader.cpp:57: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 76761 [details] fix style
Comment on attachment 76761 [details] fix style View in context: https://bugs.webkit.org/attachment.cgi?id=76761&action=review Looks good to me, just a couple pedantic questions/comments. I'm going to refrain from r+ing to give other folks a chance to chime in, especially since I'm not very familiar with the MemoryCache piece of this. > WebCore/loader/cache/CachedResourceLoader.cpp:76 > + default: > + break; > + } > + return 0; Should we have an ASSERT_NOT_REACHED() here? > WebCore/loader/cache/CachedResourceLoader.cpp:259 > - // FIXME: Consider letting the embedder block mixed content loads. > + I'm not really familiar with the mixed content policy code, is this FIXME not valid anymore? > WebCore/loader/cache/CachedResourceLoader.cpp:276 > + if (!url.isValid()) > + return 0; > + > + if (!canRequest(type, url)) > + return 0; Is there any particular reason to split this out into 2 if statements?
(In reply to comment #4) > (From update of attachment 76761 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76761&action=review > > Looks good to me, just a couple pedantic questions/comments. > > I'm going to refrain from r+ing to give other folks a chance to chime in, especially since I'm not very familiar with the MemoryCache piece of this. > > > WebCore/loader/cache/CachedResourceLoader.cpp:76 > > + default: > > + break; > > + } > > + return 0; > > Should we have an ASSERT_NOT_REACHED() here? Copy code but yeah, we should. > > > WebCore/loader/cache/CachedResourceLoader.cpp:259 > > - // FIXME: Consider letting the embedder block mixed content loads. > > + > > I'm not really familiar with the mixed content policy code, is this FIXME not valid anymore? That was removed by accident (I still don't know what it means). > > > WebCore/loader/cache/CachedResourceLoader.cpp:276 > > + if (!url.isValid()) > > + return 0; > > + > > + if (!canRequest(type, url)) > > + return 0; > > Is there any particular reason to split this out into 2 if statements? I thought a security check deserves a line of it's own. As the comment below says, this should merge with the other similar security check but I didn't want to change the existing behavior.
> > > - // FIXME: Consider letting the embedder block mixed content loads. > That was removed by accident (I still don't know what it means). That comment is easier to understand when considered together with one before the switch block: // Note: Currently, we always allow mixed content, but we generate a // callback to the FrameLoaderClient in case the embedder wants to // update any security indicators. For example, an https page that includes non-https scripts (and really, any subresources) should not be considered secure, and Safari doesn't display a lock icon for that. But loading non-https content is always risky, letting the party in control of your network connection run any sorts of attacks on authenticated https pages. I'm not sure why there is anything to consider - maybe the person who wrote this could tell why a way to block such loads wasn't added right away.
Comment on attachment 76761 [details] fix style View in context: https://bugs.webkit.org/attachment.cgi?id=76761&action=review > WebCore/css/CSSImageValue.cpp:75 > + CachedImage* cachedImage = loader->requestImage(url); > if (cachedImage) { Could put the definition inside the if statement. > WebCore/loader/cache/CachedResource.h:54 > + friend class CachedResourceLoader; Is this really necessary? Can we do something to avoid it? > WebCore/loader/cache/CachedResourceLoader.cpp:74 > + default: > + break; Normally we try to avoid having a default. That way, the compiler will warn us if we forget to include a case. Just leaving out "default: break" should do the trick.
http://trac.webkit.org/changeset/74807
> > WebCore/loader/cache/CachedResource.h:54 > > + friend class CachedResourceLoader; > > Is this really necessary? Can we do something to avoid it? I gather you are not a fan of using friends for subsystem internal API? I made some more functions public (with a comment) and got rid of the friend.
Comment on attachment 76761 [details] fix style View in context: https://bugs.webkit.org/attachment.cgi?id=76761&action=review > WebCore/ChangeLog:19 > + * css/CSSImageValue.cpp: > + (WebCore::CSSImageValue::cachedImage): > + > + Remove a code path that called MemoryCache::requestResource directly. This code path would have crashed > + if ever taken (since it passes null CachedResourceLoader pointer). Are you sure this is the case? I have a sneaking suspicion that this change is causing random memory heap corruption issues, as being investigated here: http://code.google.com/p/chromium/issues/detail?id=68516
(In reply to comment #10) > (From update of attachment 76761 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76761&action=review > > > WebCore/ChangeLog:19 > > + * css/CSSImageValue.cpp: > > + (WebCore::CSSImageValue::cachedImage): > > + > > + Remove a code path that called MemoryCache::requestResource directly. This code path would have crashed > > + if ever taken (since it passes null CachedResourceLoader pointer). > > Are you sure this is the case? I have a sneaking suspicion that this change is causing random memory heap corruption issues, as being investigated here: > > http://code.google.com/p/chromium/issues/detail?id=68516 Following this trail: CSSImageValue::cachedImage is only called from CSSStyleSelector::loadPendingImages, which is in turn called by: CSSStyleSelector::styleForElement CSSStyleSelector::styleForPage CSSStyleSelector::pseudoStyleForElement CSSStyleSelector::keyframeStylesForAnimation Since, in cachedImage(), the "loader" is document()->cachedResourceLoader() and returns an OwnPtr, could any of the methods above be invoked after the Document is destroyed? If they could, we can have memory corruption. Right?
(In reply to comment #10) > Are you sure this is the case? I have a sneaking suspicion that this change is causing random memory heap corruption issues, as being investigated here: > > http://code.google.com/p/chromium/issues/detail?id=68516 If CSSImageValue::cachedImage was called with null loader (which is the case I removed), it seems certain it would crash immediately when dereferencing in if (CachedImage* cachedImage = loader->requestImage(url)) { I can't see how it could survive that to corrupt memory. Can you enable that ASSERT(loader) crash in your reliability bot build to be sure?