Bug 106733 - CachedImage's container size may not survive cached reloads
Summary: CachedImage's container size may not survive cached reloads
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 420+
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philip Rogers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-12 18:50 PST by Philip Rogers
Modified: 2013-01-24 14:29 PST (History)
8 users (show)

See Also:


Attachments
Preserve container size requests across image loads (8.17 KB, patch)
2013-01-21 19:11 PST, Philip Rogers
thorton: review+
Details | Formatted Diff | Diff
Updated patch per reviewer comments (8.14 KB, patch)
2013-01-24 11:56 PST, Philip Rogers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Rogers 2013-01-12 18:50:23 PST
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.
Comment 1 Tim Horton 2013-01-12 18:52:25 PST
Aha! I'm pretty sure I've seen something that could be explained by this when reloading some of our layout tests.
Comment 2 Philip Rogers 2013-01-15 22:25:48 PST
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.
Comment 3 Tom Hudson 2013-01-21 08:20:28 PST
#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?
Comment 4 Philip Rogers 2013-01-21 19:11:05 PST
(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.
Comment 5 Philip Rogers 2013-01-21 19:11:45 PST
Created attachment 183869 [details]
Preserve container size requests across image loads
Comment 6 Tim Horton 2013-01-24 11:17:41 PST
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.
Comment 7 Philip Rogers 2013-01-24 11:56:59 PST
Created attachment 184547 [details]
Updated patch per reviewer comments
Comment 8 Philip Rogers 2013-01-24 12:00:14 PST
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.
Comment 9 WebKit Review Bot 2013-01-24 14:29:13 PST
Comment on attachment 184547 [details]
Updated patch per reviewer comments

Clearing flags on attachment: 184547

Committed r140722: <http://trac.webkit.org/changeset/140722>
Comment 10 WebKit Review Bot 2013-01-24 14:29:17 PST
All reviewed patches have been landed.  Closing bug.