Skip to content

Commit e766ee8

Browse files
pgorszkowski-igaliapgorszkowski-igalia
committed
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
1 parent 51171f8 commit e766ee8

4 files changed

Lines changed: 93 additions & 1 deletion

File tree

‎LayoutTests/fast/dom/dynamic-image-with-lazy-loading-leak-expected.txt‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
Tests that lazy image loading doesn't cause leaks when 'img' element is created and removed dynamically.
2+
3+
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
4+
5+
6+
PASS internals.isElementAlive(imgElementId) is true
7+
PASS internals.isElementAlive(imgElementId) is true
8+
PASS internals.isElementAlive(imgElementId) is true
9+
PASS internals.isElementAlive(imgElementId) is true
10+
PASS internals.isElementAlive(imgElementId) is true
11+
PASS internals.isElementAlive(imgElementId) is true
12+
PASS internals.isElementAlive(imgElementId) is true
13+
PASS internals.isElementAlive(imgElementId) is true
14+
PASS internals.isElementAlive(imgElementId) is true
15+
PASS internals.isElementAlive(imgElementId) is true
16+
PASS The img element didn't leak.
17+
PASS successfullyParsed is true
18+
19+
TEST COMPLETE
20+

‎LayoutTests/fast/dom/dynamic-image-with-lazy-loading-leak.html‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<body>
4+
<script src="../../resources/js-test.js"></script>
5+
<div style="height: 2000px"></div>
6+
<div id="below-viewport-img-container" style="height: 1000px"></div>
7+
<script>
8+
description("Tests that lazy image loading doesn't cause leaks when 'img' element is created and removed dynamically.");
9+
jsTestIsAsync = true;
10+
11+
checkCount = 0;
12+
13+
const imgCount = 10;
14+
imgs = [];
15+
const image_path = 'image.png';
16+
imgElementIds = [];
17+
18+
function runTest()
19+
{
20+
handle = setInterval(() => {
21+
gc();
22+
for (let imgElementId of imgElementIds) {
23+
if (!internals.isElementAlive(imgElementId)) {
24+
clearInterval(handle);
25+
testPassed("The img element didn't leak.");
26+
finishJSTest();
27+
return;
28+
}
29+
}
30+
checkCount++;
31+
if (checkCount > 1000) {
32+
clearInterval(handle);
33+
testFailed("The img elements leaked.");
34+
finishJSTest();
35+
}
36+
}, 10);
37+
}
38+
39+
onload = () => {
40+
for (let i = 0; i < imgCount; i++) {
41+
img = document.getElementById("lazy-loaded-img-"+i);
42+
imgElementId = internals.elementIdentifier(img);
43+
shouldBeTrue("internals.isElementAlive(imgElementId)");
44+
imgElementIds.push(imgElementId);
45+
document.getElementById('below-viewport-img-container').removeChild(img);
46+
img = null;
47+
}
48+
imgs = [];
49+
setTimeout(runTest, 0);
50+
};
51+
52+
for (let i = 0; i < imgCount; i++) {
53+
img = new Image();
54+
img.id = "lazy-loaded-img-"+i;
55+
img.loading = 'lazy';
56+
img.src = image_path;
57+
document.getElementById('below-viewport-img-container').append(img);
58+
imgs.push(img);
59+
img = null;
60+
}
61+
62+
</script>
63+
</body>
64+
</html>

‎Source/WebCore/loader/ImageLoader.cpp‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -519,6 +519,14 @@ void ImageLoader::timerFired()
519519
m_protectedElement = nullptr;
520520
}
521521

522+
bool ImageLoader::hasPendingActivity() const
523+
{
524+
// Because of lazy image loading, an image's load may be deferred indefinitely. To avoid leaking the element, we only
525+
// protect it once the load has actually started.
526+
bool imageWillBeLoadedLater = m_image && !m_image->isLoading() && m_image->stillNeedsLoad();
527+
return (m_hasPendingLoadEvent && !imageWillBeLoadedLater) || m_hasPendingErrorEvent;
528+
}
529+
522530
void ImageLoader::dispatchPendingEvent(ImageEventSender* eventSender)
523531
{
524532
ASSERT(eventSender == &beforeLoadEventSender() || eventSender == &loadEventSender() || eventSender == &errorEventSender());

‎Source/WebCore/loader/ImageLoader.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ class ImageLoader : public CachedImageClient {
7272

7373
// FIXME: Delete this code. beforeload event no longer exists.
7474
bool hasPendingBeforeLoadEvent() const { return m_hasPendingBeforeLoadEvent; }
75-
bool hasPendingActivity() const { return m_hasPendingLoadEvent || m_hasPendingErrorEvent; }
75+
bool hasPendingActivity() const;
7676

7777
void dispatchPendingEvent(ImageEventSender*);
7878

0 commit comments

Comments
 (0)