Summary: | svg/as-image/svg-image-with-data-uri-use-data-uri.svg is flaky after r207754 | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||
Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | achristensen, ap, cdumez, commit-queue, dbates, japhet, ryanhaddad, sabouhallawa, sam | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
youenn fablet
2016-10-24 04:13:32 PDT
Created attachment 292601 [details]
add a test expectation
Comment on attachment 292601 [details] add a test expectation Clearing flags on attachment: 292601 Committed r207756: <http://trac.webkit.org/changeset/207756> All reviewed patches have been landed. Closing bug. This test should be fixed properly <rdar://problem/29266436> Keeping a test as a flaky crash for so long is undesirable, as crashing impacts stability of other tests. Youenn, is this something that you can look into soon? (In reply to comment #5) > <rdar://problem/29266436> > > Keeping a test as a flaky crash for so long is undesirable, as crashing > impacts stability of other tests. Youenn, is this something that you can > look into soon? The issue might be due to the fact that a SVGImage/Image can be owned by several CachedImages. But the observer system which assumes a 1-to-1 relationship. The straightforward solution is to make the 1-to-1 relationship back again by cloning the SVGImage/Image when needed. I'll try to fix it next week. Created attachment 296895 [details]
Patch
Created attachment 296914 [details]
Patch
Comment on attachment 296914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296914&action=review Seems good, but I'm not awake enough right now to give this an r+ > LayoutTests/http/tests/security/cross-origin-cached-images-with-memory-pressure.html:99 > + if (window.internals) This can be merged with the previous statement. Created attachment 297305 [details]
Patch for landing
Comment on attachment 297305 [details] Patch for landing Clearing flags on attachment: 297305 Committed r209914: <http://trac.webkit.org/changeset/209914> All reviewed patches have been landed. Closing bug. Comment on attachment 296914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296914&action=review > Source/WebCore/loader/cache/CachedImage.cpp:361 > + for (auto cachedImage : m_cachedImages) > + cachedImage->decodedSizeChanged(image, delta); This loop is confusing. All the m_cachedImages must have references to the same Image object. When BitmapImage decodes a frame it calls this function which will call decodedSizeChanged() of all m_cachedImages. CachedImages:: decodedSizeChanged() will call CachedResource::setDecodedSize() to update its m_decodedSize. If I have an image whose ImageFrame size is say 2MB and which is referenced by 10 CachedImages, will the memoryCache see a 2MB or 20MB allocation? Comment on attachment 296914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296914&action=review >> Source/WebCore/loader/cache/CachedImage.cpp:361 >> + cachedImage->decodedSizeChanged(image, delta); > > This loop is confusing. All the m_cachedImages must have references to the same Image object. When BitmapImage decodes a frame it calls this function which will call decodedSizeChanged() of all m_cachedImages. CachedImages:: decodedSizeChanged() will call CachedResource::setDecodedSize() to update its m_decodedSize. If I have an image whose ImageFrame size is say 2MB and which is referenced by 10 CachedImages, will the memoryCache see a 2MB or 20MB allocation? I think this function should be written like this: m_cachedImages[0]->decodedSizeChanged(image, delta); |