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.
Aha! I'm pretty sure I've seen something that could be explained by this when reloading some of our layout tests.
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.
#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?
(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.
Created attachment 183869 [details] Preserve container size requests across image loads
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.
Created attachment 184547 [details] Updated patch per reviewer comments
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 on attachment 184547 [details] Updated patch per reviewer comments Clearing flags on attachment: 184547 Committed r140722: <http://trac.webkit.org/changeset/140722>
All reviewed patches have been landed. Closing bug.