Bug 163887

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 Flags
add a test expectation
none
Patch
none
Patch
none
Patch for landing none

Comment 1 youenn fablet 2016-10-24 04:17:26 PDT
Created attachment 292601 [details]
add a test expectation
Comment 2 WebKit Commit Bot 2016-10-24 04:53:17 PDT
Comment on attachment 292601 [details]
add a test expectation

Clearing flags on attachment: 292601

Committed r207756: <http://trac.webkit.org/changeset/207756>
Comment 3 WebKit Commit Bot 2016-10-24 04:53:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 4 youenn fablet 2016-10-24 04:54:07 PDT
This test should be fixed properly
Comment 5 Alexey Proskuryakov 2016-11-15 14:54:34 PST
<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?
Comment 6 youenn fablet 2016-11-16 07:56:25 PST
(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.
Comment 7 youenn fablet 2016-12-12 00:02:17 PST
Created attachment 296895 [details]
Patch
Comment 8 youenn fablet 2016-12-12 06:26:05 PST
Created attachment 296914 [details]
Patch
Comment 9 Alex Christensen 2016-12-12 23:41:29 PST
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.
Comment 10 youenn fablet 2016-12-16 00:07:15 PST
Created attachment 297305 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2016-12-16 00:43:06 PST
Comment on attachment 297305 [details]
Patch for landing

Clearing flags on attachment: 297305

Committed r209914: <http://trac.webkit.org/changeset/209914>
Comment 12 WebKit Commit Bot 2016-12-16 00:43:12 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Said Abou-Hallawa 2017-02-17 12:30:42 PST
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 14 Said Abou-Hallawa 2017-02-17 12:33:17 PST
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);