Bug 163887 – svg/as-image/svg-image-with-data-uri-use-data-uri.svg is flaky after r207754

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

It is now often crashing
https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/r207754%20(15818)/svg/as-image/svg-image-with-data-uri-use-data-uri-crash-log.txt
e.g.

[youenn fablet]

Created attachment 292601 [details]
add a test expectation

[WebKit Commit Bot]

Comment on attachment 292601 [details]
add a test expectation

Clearing flags on attachment: 292601

Committed r207756: <http://trac.webkit.org/changeset/207756>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

This test should be fixed properly

[Alexey Proskuryakov]

<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?

[youenn fablet]

(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.

[youenn fablet]

Created attachment 296895 [details]
Patch

[youenn fablet]

Created attachment 296914 [details]
Patch

[Alex Christensen]

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.

[youenn fablet]

Created attachment 297305 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 297305 [details]
Patch for landing

Clearing flags on attachment: 297305

Committed r209914: <http://trac.webkit.org/changeset/209914>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

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?

[Said Abou-Hallawa]

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);

[Fujii Hironori]

Re-opening for pull request https://github.com/WebKit/WebKit/pull/33372

[EWS]

Test gardening commit 283392@main (92a5820525a6): <https://commits.webkit.org/283392@main>

Reviewed commits have been landed. Closing PR #33372 and removing active labels.