Bug 106733 – CachedImage's container size may not survive cached reloads

• Component: Images
• Status: RESOLVED FIXED

[Philip Rogers]

CachedImage relies on a call to setContainerSizeForRenderer(...) to set the image's container size. During a cached reload we have two CachedImages (one is used for cache revalidation) and we throw one of these away if the cache revalidation succeeds. Unfortunately, setContainerSizeForRenderer may have been called on the CachedImage that we threw away, and we will lose the container size information.

Here's an example of the sequence of events:

CachedImage ctor (imageA)
CachedImage(imageA) did add client: (rendererA)
CachedImage(imageA) setContainerSizeForRenderer (rendererA)
CachedImage(imageA) lookupOrCreateImageForRenderer (rendererA)
[ page reload occurs here ]
CachedImage(imageA) did remove client: (rendererA)
CachedImage ctor (imageB)
CachedImage(imageB) did add client: (rendererB)
CachedImage(imageB) setContainerSizeForRenderer (rendererB)
CachedImage(imageB) did remove client: (rendererB)
CachedImage(imageA) did add client: (rendererB)
CachedImage dtor (imageB)
CachedImage(imageA) lookupOrCreateImageForRenderer (rendererB) << fails to use the correct container size


A sample repro case is available at http://philbit.com/reloadtest.html. Reload several times and notice that the image size can render incorrectly.

[Tim Horton]

Aha! I'm pretty sure I've seen something that could be explained by this when reloading some of our layout tests.

[Philip Rogers]

Today I experimented with not storing the container size in CachedImage, but instead relying on passing the container size into all of CachedImage's functions. This ends up being incredibly brittle: not all accessors of imageForRenderer or imageSizeForRenderer know the container size and getting this container size wrong (or rounded) in any of the ~30 callsites means a subtle cache miss.

Nikolas attempted to fix this by re-requesting images in https://bugs.webkit.org/attachment.cgi?id=105604 but that approach failed as well.

The two remaining ideas I have are:
1) Instead of blindly swapping the revalidated CachedImage with the cached CachedImage in CachedResource::switchClientsToRevalidatedResource, we could special-case ImageResources and properly transfer over the container size request information.

2) Calling setContainerSizeForRenderer before drawing easily fixes the issue, but that's hacky.

[Tom Hudson]

#2, can you explain why option 2 is hacky? Or why we don't just make sure that when we throw away a CachedImage during cached reload, we first setContainerSizeForRenderer on the other copy?

[Philip Rogers]

(In reply to comment #3)
> #2, can you explain why option 2 is hacky? Or why we don't just make sure that when we throw away a CachedImage during cached reload, we first setContainerSizeForRenderer on the other copy?

#2 is hacky because in calling setContainerSizeForRenderer before drawing, we are admitting that the original call to setContainerSizeForRenderer cannot be trusted. Throughout the code (and these calls are all over! e.g., RenderListMarker and StyleCachedImageSet) we use setContainerSizeForRenderer and all of those callsites can potentially have the same bug.

I'll have a patch up shortly based on #1.

[Philip Rogers]

Created attachment 183869 [details]
Preserve container size requests across image loads

[Tim Horton]

Comment on attachment 183869 [details]
Preserve container size requests across image loads

View in context: https://bugs.webkit.org/attachment.cgi?id=183869&action=review

Seems pretty reasonable. A few comments:

> Source/WebCore/loader/cache/CachedImage.cpp:121
> +    if (!m_pendingContainerSizeRequests.isEmpty() && resourceToRevalidate()->isImage()) {

Is it possible for resourceToRevalidate()->isImage() to be false?

> Source/WebCore/loader/cache/CachedImage.cpp:123
> +        ContainerSizeRequests m_switchContainerSizeRequests;

Why does this local have an m_?

> Source/WebCore/loader/cache/CachedImage.h:112
> +    virtual void switchClientsToRevalidatedResource();

OVERRIDE, please.

> LayoutTests/http/tests/svg/resources/delayCachedLoad.php:4
> +  // Delay load by 0.07s. This was found to be the lowest value
> +  // required for webkit.org/b/106733
> +  usleep(70000);

:\

Is this machine dependent or anything (or, how is it not)? I'm always sad when I see things like this.

[Philip Rogers]

Created attachment 184547 [details]
Updated patch per reviewer comments

[Philip Rogers]

Thanks for the review!

(In reply to comment #6)
> (From update of attachment 183869 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183869&action=review
> 
> Seems pretty reasonable. A few comments:
> 
> > Source/WebCore/loader/cache/CachedImage.cpp:121
> > +    if (!m_pendingContainerSizeRequests.isEmpty() && resourceToRevalidate()->isImage()) {
> 
> Is it possible for resourceToRevalidate()->isImage() to be false?

I don't think so. I've converted this to an ASSERT.

> 
> > Source/WebCore/loader/cache/CachedImage.cpp:123
> > +        ContainerSizeRequests m_switchContainerSizeRequests;
> 
> Why does this local have an m_?

Fixed.

> 
> > Source/WebCore/loader/cache/CachedImage.h:112
> > +    virtual void switchClientsToRevalidatedResource();
> 
> OVERRIDE, please.

Fixed.

> 
> > LayoutTests/http/tests/svg/resources/delayCachedLoad.php:4
> > +  // Delay load by 0.07s. This was found to be the lowest value
> > +  // required for webkit.org/b/106733
> > +  usleep(70000);
> 
> :\
> 
> Is this machine dependent or anything (or, how is it not)? I'm always sad when I see things like this.

I'm not a fan either. I'll keep an eye on how the bots handle this, and I'll increase the time if necessary.

[WebKit Review Bot]

Comment on attachment 184547 [details]
Updated patch per reviewer comments

Clearing flags on attachment: 184547

Committed r140722: <http://trac.webkit.org/changeset/140722>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.