Memory consumption/leak with img out of viewport and lazy loading

https://bugs.webkit.org/show_bug.cgi?id=263521 Reviewed by Chris Dumez. This change fixes the problem with dangling of dynamically created (in JS) HTMLImageElement when it is detached from the document before loading the resource starts. It happened when img element was created (dynamically) with lazy loading and the element was outside the viewport (the loading of resource is deferred until the img element becomes visible). If the element was removed from document it becomes dangling element and will never be deleted by GC. * Source/WebCore/loader/ImageLoader.cpp: (WebCore::ImageLoader::hasPendingActivity const): To avoid leaking of the dynamically created element, the pending activity of the element should check has the load of the resource actually started. Similar check is done in case of static HTMLImageElement in ImageLoader::updatedHasPendingEvent. * Source/WebCore/loader/ImageLoader.h: (WebCore::ImageLoader::hasPendingActivity const): Deleted. Moved implementation to cpp file. * LayoutTests/fast/dom/dynamic-image-with-lazy-loading-leak-expected.txt: Added. * LayoutTests/fast/dom/dynamic-image-with-lazy-loading-leak.html: Added. Canonical link: https://commits.webkit.org/270745@main
WebKit · Nov 15, 2023 · 78e4577 · 78e4577
1 parent c093fd4
commit 78e4577
Show file tree

Hide file tree

Showing 4 changed files with 93 additions and 1 deletion.
diff --git a/LayoutTests/fast/dom/dynamic-image-with-lazy-loading-leak-expected.txt b/LayoutTests/fast/dom/dynamic-image-with-lazy-loading-leak-expected.txt
@@ -0,0 +1,20 @@
+Tests that lazy image loading doesn't cause leaks when 'img' element is created and removed dynamically.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS internals.isElementAlive(imgElementId) is true
+PASS internals.isElementAlive(imgElementId) is true
+PASS internals.isElementAlive(imgElementId) is true
+PASS internals.isElementAlive(imgElementId) is true
+PASS internals.isElementAlive(imgElementId) is true
+PASS internals.isElementAlive(imgElementId) is true
+PASS internals.isElementAlive(imgElementId) is true
+PASS internals.isElementAlive(imgElementId) is true
+PASS internals.isElementAlive(imgElementId) is true
+PASS internals.isElementAlive(imgElementId) is true
+PASS The img element didn't leak.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/dynamic-image-with-lazy-loading-leak.html b/LayoutTests/fast/dom/dynamic-image-with-lazy-loading-leak.html
@@ -0,0 +1,64 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../../resources/js-test.js"></script>
+<div style="height: 2000px"></div>
+<div id="below-viewport-img-container" style="height: 1000px"></div>    
+<script>
+description("Tests that lazy image loading doesn't cause leaks when 'img' element is created and removed dynamically.");
+jsTestIsAsync = true;
+
+checkCount = 0;
+
+const imgCount = 10;
+imgs = [];
+const image_path = 'image.png';
+imgElementIds = [];
+
+function runTest()
+{
+    handle = setInterval(() => {
+        gc();
+        for (let imgElementId of imgElementIds) {
+            if (!internals.isElementAlive(imgElementId)) {
+                clearInterval(handle);
+                testPassed("The img element didn't leak.");
+                finishJSTest();
+                return;
+            }
+        }
+        checkCount++;
+        if (checkCount > 1000) {
+            clearInterval(handle);
+            testFailed("The img elements leaked.");
+            finishJSTest();
+        }
+    }, 10);
+}
+
+onload = () => {
+    for (let i = 0; i < imgCount; i++) {
+        img = document.getElementById("lazy-loaded-img-"+i);
+        imgElementId = internals.elementIdentifier(img);
+        shouldBeTrue("internals.isElementAlive(imgElementId)");
+        imgElementIds.push(imgElementId);
+        document.getElementById('below-viewport-img-container').removeChild(img);
+        img = null;
+    }
+    imgs = [];
+    setTimeout(runTest, 0);
+};
+
+for (let i = 0; i < imgCount; i++) {
+    img = new Image();
+    img.id = "lazy-loaded-img-"+i;
+    img.loading = 'lazy';
+    img.src = image_path;
+    document.getElementById('below-viewport-img-container').append(img);
+    imgs.push(img);
+    img = null;
+}
+
+</script>
+</body>
+</html>
diff --git a/Source/WebCore/loader/ImageLoader.cpp b/Source/WebCore/loader/ImageLoader.cpp
@@ -545,6 +545,14 @@ void ImageLoader::timerFired()
     m_protectedElement = nullptr;
 }
 
+bool ImageLoader::hasPendingActivity() const
+{
+    // Because of lazy image loading, an image's load may be deferred indefinitely. To avoid leaking the element, we only
+    // protect it once the load has actually started.
+    bool imageWillBeLoadedLater = m_image && !m_image->isLoading() && m_image->stillNeedsLoad();
+    return (m_hasPendingLoadEvent && !imageWillBeLoadedLater) || m_hasPendingErrorEvent;
+}
+
 void ImageLoader::dispatchPendingEvent(ImageEventSender* eventSender, const AtomString& eventType)
 {
     ASSERT_UNUSED(eventSender, eventSender == &loadEventSender());

diff --git a/Source/WebCore/loader/ImageLoader.h b/Source/WebCore/loader/ImageLoader.h
@@ -74,7 +74,7 @@ class ImageLoader : public CachedImageClient {
 
     // FIXME: Delete this code. beforeload event no longer exists.
     bool hasPendingBeforeLoadEvent() const { return m_hasPendingBeforeLoadEvent; }
-    bool hasPendingActivity() const { return m_hasPendingLoadEvent || m_hasPendingErrorEvent; }
+    bool hasPendingActivity() const;
 
     void dispatchPendingEvent(ImageEventSender*, const AtomString& eventType);