Bug 51134 – Move loading related code from MemoryCache to CachedResourceLoader

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Antti Koivisto]

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.

[Antti Koivisto]

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

[WebKit Review Bot]

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.

[Antti Koivisto]

Created attachment 76761 [details]
fix style

[Nate Chapin]

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?

[Antti Koivisto]

(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.

[Alexey Proskuryakov]

> > > -    // 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.

[Darin Adler]

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.

[Antti Koivisto]

http://trac.webkit.org/changeset/74807

[Antti Koivisto]

> > 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.

[Dimitri Glazkov (Google)]

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

[Dimitri Glazkov (Google)]

(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?

[Antti Koivisto]

(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?