263521 2023-10-23 06:26:18 -0700 Memory consumption/leak with img out of viewport and lazy loading 2023-11-14 19:58:17 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=262238 InRadar P2 Normal --- 1 pgorszkowski pgorszkowski jblas webkit-bug-importer oldest_to_newest 1986798 0 pgorszkowski 2023-10-23 06:26:18 -0700 Originally the problem was reported for WPE 2.38: https://github.com/WebPlatformForEmbedded/WPEWebKit/issues/1194 The same problem can be also reproduced on current WebKitGTK. I create a simple test case on which the problem can be observed: https://people.igalia.com/pgorszkowski/img_lazy_loading.html?useLazy=1 The idea of the simple test case is: 1. create 1000 images with lazy loading(the same src) below the visible viewport 2. after 100ms remove existing images, and create another 1000 new images with lazy loading (the same src) below the visible viewport 3. do this recreation 1000 times With lazy loading the memory grows quite fast from ~200MB to 2000MB after ~500 iterations Without lazy loading (https://people.igalia.com/pgorszkowski/img_lazy_loading.html?useLazy=0) the memory stays on stable level ~200MB (never grows above 250MB) and after 1000 iterations it is still ~200MB 1986799 1 pgorszkowski 2023-10-23 06:43:51 -0700 I found that this fix solves the problem: diff --git a/Source/WebCore/html/HTMLImageElement.cpp b/Source/WebCore/html/HTMLImageElement.cpp index 3e4855fd20e2..5ddc923b1568 100644 --- a/Source/WebCore/html/HTMLImageElement.cpp +++ b/Source/WebCore/html/HTMLImageElement.cpp @@ -514,6 +518,8 @@ void HTMLImageElement::removedFromAncestor(RemovalType removalType, ContainerNod selectImageSource(RelevantMutation::Yes); } + m_imageLoader->clearImage(); + HTMLElement::removedFromAncestor(removalType, oldParentOfRemovedTree); FormAssociatedElement::elementRemovedFromAncestor(*this, removalType); } but I am not sure about possible regressions or other impact which it can have (so I would be glad for any comments or advices). I will elaborate more about my analysis in the next comment, which will be probably quite long because it is rather a complex process. 1986821 2 ap 2023-10-23 08:59:46 -0700 Is this the same as bug 262238? 1986899 3 pgorszkowski 2023-10-23 12:27:16 -0700 Yes, it is. I have checked also my proposal fix and it solves the problem from https://bugs.webkit.org/show_bug.cgi?id=262238 1986901 4 pgorszkowski 2023-10-23 12:28:54 -0700 But I checked it on WebKitGTK. It would be better to confirm it also on Safari. 1987382 5 pgorszkowski 2023-10-25 06:41:18 -0700 So the main problem is when the img element contains loading="lazy" and the element is outside the viewport which causes deferring the loading of the image until it is in visible area. The deferring load also increases the img element reference by m_protectedElement member in ImageLoader (which is kept by HTMLImageElement). m_protectedElement is reset (and additional reference of the image element is decreased) when image is finally loaded. The problem with dangling HTMLImageElement is when we remove it from the document before the image resource is finally loaded. When we remove the HTMLImageElement from document but image loading is deferred, the imageLoader still keeps additional reference for HTMLImageElement in m_protectedElement which causes the GC will never remove it (loading will never be done because HTMLImageElement is removed from document and will never be considered as in visible area). To solve the problem we can add m_imageLoader->clearImage(); in HTMLImageElement::removedFromAncestor if the loading image was deferred. 1987506 6 pgorszkowski 2023-10-25 12:51:48 -0700 Pull request: https://github.com/WebKit/WebKit/pull/19559 1987509 7 pgorszkowski 2023-10-25 13:02:25 -0700 *** Bug 262238 has been marked as a duplicate of this bug. *** 1988473 8 webkit-bug-importer 2023-10-30 06:27:14 -0700 <rdar://problem/117683012> 1992654 9 ews-feeder 2023-11-14 19:58:16 -0800 Committed 270745@main (78e4577732ca): <https://commits.webkit.org/270745@main> Reviewed commits have been landed. Closing PR #19559 and removing active labels.