Summary: | CachedImage's container size may not survive cached reloads | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philip Rogers <pdr> | ||||||
Component: | Images | Assignee: | Philip Rogers <pdr> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ap, japhet, junov, koivisto, m.goleb+bugzilla, thorton, tomhudson, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 420+ | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Philip Rogers
2013-01-12 18:50:23 PST
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. |