- Revision
- 291662
- Author
- alanc...@apple.com
- Date
- 2022-03-22 10:56:28 -0700 (Tue, 22 Mar 2022)
Log Message
Cherry-pick r291141. rdar://problem/90035071
Main document is leaking on haaretz.co.il due to lazy image loading
https://bugs.webkit.org/show_bug.cgi?id=237660
<rdar://problem/90035071>
Reviewed by Geoffrey Garen.
Source/WebCore:
When an HTML image uses `loading=lazy`, ImageLoader::updateFromElement() may get
called twice. If the image is outside the viewport, the first time ImageLoader::updateFromElement()
is called, we'll request a CachedImage but we'll set m_lazyImageLoadState to LazyImageLoadState::Deferred
and not ask the CachedImage to load. Then, later on, if the HTML image approaches the viewport,
ImageLoader::loadDeferredImage() will get called. It will set m_lazyImageLoadState to LazyImageLoadState::LoadImmediately
and call updateFromElement() again. This time however, updateFromElement() will actually ask the CachedImage
to load.
The issue was that the first time ImageLoader::updateFromElement(), we would set m_lazyImageLoadState to Deferred
and not ask the CachedImage to load but still set m_hasPendingLoadEvent to true. This is problematic if the CachedImage
is not already loaded since no was was started and thus no load event is coming (and no load event may ever come if the
image never approaches the viewport). When updatedHasPendingEvent() is called, it will protect the HTMLImageElement if
m_hasPendingLoadEvent is true, to make sure the image element stays alive long enough for us to dispatch the load event.
With lazy loading, this meant that we would protect the HTMLImageElement right away and undefinitely since no load event
may ever come. This meant that when navigating away from a page with images that are lazily loaded (and not loaded yet),
we would leak the HTMLImageElements (and ImageLoaders), which in turn would keep the Document alive too.
To address the issue, we now make sure that m_hasPendingLoadEvent is no longer set to true when updateFromElement()
is called but the CachedImage is not already loaded and not loading (lazy loading case). When updateFromElement() gets
called the second time (when the lazily loaded image approaches the viewport), we make sure that the m_hasPendingLoadEvent
flag gets set to true then.
Test: fast/dom/lazy-image-loading-document-leak.html
* loader/ImageLoader.cpp:
(WebCore::ImageLoader::ImageLoader):
(WebCore::ImageLoader::updateFromElement):
(WebCore::ImageLoader::didUpdateCachedImage):
(WebCore::ImageLoader::didStartLoading):
* loader/ImageLoader.h:
* loader/cache/CachedImage.cpp:
(WebCore::CachedImage::load):
* loader/cache/CachedImageClient.h:
(WebCore::CachedImageClient::didStartLoading):
LayoutTests:
Add layout test coverage.
* fast/dom/lazy-image-loading-document-leak-expected.txt: Added.
* fast/dom/lazy-image-loading-document-leak.html: Added.
* fast/dom/resources/lazy-image-loading-document-leak-popup.html: Added.
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@291141 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Modified Paths
Added Paths
Diff
Modified: branches/safari-613-branch/LayoutTests/ChangeLog (291661 => 291662)
--- branches/safari-613-branch/LayoutTests/ChangeLog 2022-03-22 17:56:23 UTC (rev 291661)
+++ branches/safari-613-branch/LayoutTests/ChangeLog 2022-03-22 17:56:28 UTC (rev 291662)
@@ -1,5 +1,77 @@
2022-03-21 Alan Coon <alanc...@apple.com>
+ Cherry-pick r291141. rdar://problem/90035071
+
+ Main document is leaking on haaretz.co.il due to lazy image loading
+ https://bugs.webkit.org/show_bug.cgi?id=237660
+ <rdar://problem/90035071>
+
+ Reviewed by Geoffrey Garen.
+
+ Source/WebCore:
+
+ When an HTML image uses `loading=lazy`, ImageLoader::updateFromElement() may get
+ called twice. If the image is outside the viewport, the first time ImageLoader::updateFromElement()
+ is called, we'll request a CachedImage but we'll set m_lazyImageLoadState to LazyImageLoadState::Deferred
+ and not ask the CachedImage to load. Then, later on, if the HTML image approaches the viewport,
+ ImageLoader::loadDeferredImage() will get called. It will set m_lazyImageLoadState to LazyImageLoadState::LoadImmediately
+ and call updateFromElement() again. This time however, updateFromElement() will actually ask the CachedImage
+ to load.
+
+ The issue was that the first time ImageLoader::updateFromElement(), we would set m_lazyImageLoadState to Deferred
+ and not ask the CachedImage to load but still set m_hasPendingLoadEvent to true. This is problematic if the CachedImage
+ is not already loaded since no was was started and thus no load event is coming (and no load event may ever come if the
+ image never approaches the viewport). When updatedHasPendingEvent() is called, it will protect the HTMLImageElement if
+ m_hasPendingLoadEvent is true, to make sure the image element stays alive long enough for us to dispatch the load event.
+ With lazy loading, this meant that we would protect the HTMLImageElement right away and undefinitely since no load event
+ may ever come. This meant that when navigating away from a page with images that are lazily loaded (and not loaded yet),
+ we would leak the HTMLImageElements (and ImageLoaders), which in turn would keep the Document alive too.
+
+ To address the issue, we now make sure that m_hasPendingLoadEvent is no longer set to true when updateFromElement()
+ is called but the CachedImage is not already loaded and not loading (lazy loading case). When updateFromElement() gets
+ called the second time (when the lazily loaded image approaches the viewport), we make sure that the m_hasPendingLoadEvent
+ flag gets set to true then.
+
+ Test: fast/dom/lazy-image-loading-document-leak.html
+
+ * loader/ImageLoader.cpp:
+ (WebCore::ImageLoader::ImageLoader):
+ (WebCore::ImageLoader::updateFromElement):
+ (WebCore::ImageLoader::didUpdateCachedImage):
+ (WebCore::ImageLoader::didStartLoading):
+ * loader/ImageLoader.h:
+ * loader/cache/CachedImage.cpp:
+ (WebCore::CachedImage::load):
+ * loader/cache/CachedImageClient.h:
+ (WebCore::CachedImageClient::didStartLoading):
+
+ LayoutTests:
+
+ Add layout test coverage.
+
+ * fast/dom/lazy-image-loading-document-leak-expected.txt: Added.
+ * fast/dom/lazy-image-loading-document-leak.html: Added.
+ * fast/dom/resources/lazy-image-loading-document-leak-popup.html: Added.
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@291141 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2022-03-10 Chris Dumez <cdu...@apple.com>
+
+ Main document is leaking on haaretz.co.il due to lazy image loading
+ https://bugs.webkit.org/show_bug.cgi?id=237660
+ <rdar://problem/90035071>
+
+ Reviewed by Geoffrey Garen.
+
+ Add layout test coverage.
+
+ * fast/dom/lazy-image-loading-document-leak-expected.txt: Added.
+ * fast/dom/lazy-image-loading-document-leak.html: Added.
+ * fast/dom/resources/lazy-image-loading-document-leak-popup.html: Added.
+
+2022-03-21 Alan Coon <alanc...@apple.com>
+
Cherry-pick r291030. rdar://problem/89989815
IntersectionObserver is causing massive document leaks on haaretz.co.il
Added: branches/safari-613-branch/LayoutTests/fast/dom/lazy-image-loading-document-leak-expected.txt (0 => 291662)
--- branches/safari-613-branch/LayoutTests/fast/dom/lazy-image-loading-document-leak-expected.txt (rev 0)
+++ branches/safari-613-branch/LayoutTests/fast/dom/lazy-image-loading-document-leak-expected.txt 2022-03-22 17:56:28 UTC (rev 291662)
@@ -0,0 +1,11 @@
+Tests that lazy image loading doesn't cause Document leaks.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS internals.isDocumentAlive(frameDocumentID) is true
+PASS The popup document didn't leak.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: branches/safari-613-branch/LayoutTests/fast/dom/lazy-image-loading-document-leak.html (0 => 291662)
--- branches/safari-613-branch/LayoutTests/fast/dom/lazy-image-loading-document-leak.html (rev 0)
+++ branches/safari-613-branch/LayoutTests/fast/dom/lazy-image-loading-document-leak.html 2022-03-22 17:56:28 UTC (rev 291662)
@@ -0,0 +1,45 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src=""
+<script>
+description("Tests that lazy image loading doesn't cause Document leaks.");
+jsTestIsAsync = true;
+
+frameDocumentID = 0;
+checkCount = 0;
+popup = null;
+
+function runTest()
+{
+ popup.close();
+ popup = null;
+
+ handle = setInterval(() => {
+ gc();
+ if (!internals.isDocumentAlive(frameDocumentID)) {
+ clearInterval(handle);
+ testPassed("The popup document didn't leak.");
+ finishJSTest();
+ }
+ checkCount++;
+ if (checkCount > 500) {
+ clearInterval(handle);
+ testFailed("The popup document leaked.");
+ finishJSTest();
+ }
+ }, 10);
+}
+
+_onload_ = () => {
+ popup = window.open("resources/lazy-image-loading-document-leak-popup.html");
+ popup._onload_ = () => {
+ popup._onload_ = null;
+ frameDocumentID = internals.documentIdentifier(popup.document);
+ shouldBeTrue("internals.isDocumentAlive(frameDocumentID)");
+ setTimeout(runTest, 0);
+ }
+};
+</script>
+</body>
+</html>
Added: branches/safari-613-branch/LayoutTests/fast/dom/resources/lazy-image-loading-document-leak-popup.html (0 => 291662)
--- branches/safari-613-branch/LayoutTests/fast/dom/resources/lazy-image-loading-document-leak-popup.html (rev 0)
+++ branches/safari-613-branch/LayoutTests/fast/dom/resources/lazy-image-loading-document-leak-popup.html 2022-03-22 17:56:28 UTC (rev 291662)
@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+<html>
+<body>
+<div style="height: 3000px">
+<div style="height: 2000px">Test lazy image loading</div>
+<div style="height: 1000px">
+<img loading="lazy" src=""
+<img loading="lazy" src=""
+</div>
+</div>
+</body>
+</html>
Modified: branches/safari-613-branch/Source/WebCore/ChangeLog (291661 => 291662)
--- branches/safari-613-branch/Source/WebCore/ChangeLog 2022-03-22 17:56:23 UTC (rev 291661)
+++ branches/safari-613-branch/Source/WebCore/ChangeLog 2022-03-22 17:56:28 UTC (rev 291662)
@@ -1,5 +1,106 @@
2022-03-21 Alan Coon <alanc...@apple.com>
+ Cherry-pick r291141. rdar://problem/90035071
+
+ Main document is leaking on haaretz.co.il due to lazy image loading
+ https://bugs.webkit.org/show_bug.cgi?id=237660
+ <rdar://problem/90035071>
+
+ Reviewed by Geoffrey Garen.
+
+ Source/WebCore:
+
+ When an HTML image uses `loading=lazy`, ImageLoader::updateFromElement() may get
+ called twice. If the image is outside the viewport, the first time ImageLoader::updateFromElement()
+ is called, we'll request a CachedImage but we'll set m_lazyImageLoadState to LazyImageLoadState::Deferred
+ and not ask the CachedImage to load. Then, later on, if the HTML image approaches the viewport,
+ ImageLoader::loadDeferredImage() will get called. It will set m_lazyImageLoadState to LazyImageLoadState::LoadImmediately
+ and call updateFromElement() again. This time however, updateFromElement() will actually ask the CachedImage
+ to load.
+
+ The issue was that the first time ImageLoader::updateFromElement(), we would set m_lazyImageLoadState to Deferred
+ and not ask the CachedImage to load but still set m_hasPendingLoadEvent to true. This is problematic if the CachedImage
+ is not already loaded since no was was started and thus no load event is coming (and no load event may ever come if the
+ image never approaches the viewport). When updatedHasPendingEvent() is called, it will protect the HTMLImageElement if
+ m_hasPendingLoadEvent is true, to make sure the image element stays alive long enough for us to dispatch the load event.
+ With lazy loading, this meant that we would protect the HTMLImageElement right away and undefinitely since no load event
+ may ever come. This meant that when navigating away from a page with images that are lazily loaded (and not loaded yet),
+ we would leak the HTMLImageElements (and ImageLoaders), which in turn would keep the Document alive too.
+
+ To address the issue, we now make sure that m_hasPendingLoadEvent is no longer set to true when updateFromElement()
+ is called but the CachedImage is not already loaded and not loading (lazy loading case). When updateFromElement() gets
+ called the second time (when the lazily loaded image approaches the viewport), we make sure that the m_hasPendingLoadEvent
+ flag gets set to true then.
+
+ Test: fast/dom/lazy-image-loading-document-leak.html
+
+ * loader/ImageLoader.cpp:
+ (WebCore::ImageLoader::ImageLoader):
+ (WebCore::ImageLoader::updateFromElement):
+ (WebCore::ImageLoader::didUpdateCachedImage):
+ (WebCore::ImageLoader::didStartLoading):
+ * loader/ImageLoader.h:
+ * loader/cache/CachedImage.cpp:
+ (WebCore::CachedImage::load):
+ * loader/cache/CachedImageClient.h:
+ (WebCore::CachedImageClient::didStartLoading):
+
+ LayoutTests:
+
+ Add layout test coverage.
+
+ * fast/dom/lazy-image-loading-document-leak-expected.txt: Added.
+ * fast/dom/lazy-image-loading-document-leak.html: Added.
+ * fast/dom/resources/lazy-image-loading-document-leak-popup.html: Added.
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@291141 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2022-03-10 Chris Dumez <cdu...@apple.com>
+
+ Main document is leaking on haaretz.co.il due to lazy image loading
+ https://bugs.webkit.org/show_bug.cgi?id=237660
+ <rdar://problem/90035071>
+
+ Reviewed by Geoffrey Garen.
+
+ When an HTML image uses `loading=lazy`, ImageLoader::updateFromElement() may get
+ called twice. If the image is outside the viewport, the first time ImageLoader::updateFromElement()
+ is called, we'll request a CachedImage but we'll set m_lazyImageLoadState to LazyImageLoadState::Deferred
+ and not ask the CachedImage to load. Then, later on, if the HTML image approaches the viewport,
+ ImageLoader::loadDeferredImage() will get called. It will set m_lazyImageLoadState to LazyImageLoadState::LoadImmediately
+ and call updateFromElement() again. This time however, updateFromElement() will actually ask the CachedImage
+ to load.
+
+ The issue was that the first time ImageLoader::updateFromElement(), we would set m_lazyImageLoadState to Deferred
+ and not ask the CachedImage to load but still set m_hasPendingLoadEvent to true. This is problematic if the CachedImage
+ is not already loaded since no was was started and thus no load event is coming (and no load event may ever come if the
+ image never approaches the viewport). When updatedHasPendingEvent() is called, it will protect the HTMLImageElement if
+ m_hasPendingLoadEvent is true, to make sure the image element stays alive long enough for us to dispatch the load event.
+ With lazy loading, this meant that we would protect the HTMLImageElement right away and undefinitely since no load event
+ may ever come. This meant that when navigating away from a page with images that are lazily loaded (and not loaded yet),
+ we would leak the HTMLImageElements (and ImageLoaders), which in turn would keep the Document alive too.
+
+ To address the issue, we now make sure that m_hasPendingLoadEvent is no longer set to true when updateFromElement()
+ is called but the CachedImage is not already loaded and not loading (lazy loading case). When updateFromElement() gets
+ called the second time (when the lazily loaded image approaches the viewport), we make sure that the m_hasPendingLoadEvent
+ flag gets set to true then.
+
+ Test: fast/dom/lazy-image-loading-document-leak.html
+
+ * loader/ImageLoader.cpp:
+ (WebCore::ImageLoader::ImageLoader):
+ (WebCore::ImageLoader::updateFromElement):
+ (WebCore::ImageLoader::didUpdateCachedImage):
+ (WebCore::ImageLoader::didStartLoading):
+ * loader/ImageLoader.h:
+ * loader/cache/CachedImage.cpp:
+ (WebCore::CachedImage::load):
+ * loader/cache/CachedImageClient.h:
+ (WebCore::CachedImageClient::didStartLoading):
+
+2022-03-21 Alan Coon <alanc...@apple.com>
+
Cherry-pick r291127. rdar://problem/90050632
Document is leaking on haaretz.co.il due to an async script
Modified: branches/safari-613-branch/Source/WebCore/loader/ImageLoader.cpp (291661 => 291662)
--- branches/safari-613-branch/Source/WebCore/loader/ImageLoader.cpp 2022-03-22 17:56:23 UTC (rev 291661)
+++ branches/safari-613-branch/Source/WebCore/loader/ImageLoader.cpp 2022-03-22 17:56:28 UTC (rev 291662)
@@ -46,6 +46,7 @@
#include "RenderSVGImage.h"
#include "Settings.h"
#include <wtf/NeverDestroyed.h>
+#include <wtf/Scope.h>
#if ENABLE(VIDEO)
#include "RenderVideo.h"
@@ -104,6 +105,7 @@
, m_imageComplete(true)
, m_loadManually(false)
, m_elementIsProtected(false)
+ , m_inUpdateFromElement(false)
{
}
@@ -175,6 +177,11 @@
if (!m_failedLoadURL.isEmpty() && attr == m_failedLoadURL)
return;
+ m_inUpdateFromElement = true;
+ auto inUpdateFromElementScope = makeScopeExit([this] {
+ m_inUpdateFromElement = false;
+ });
+
// Do not load any image if the 'src' attribute is missing or if it is
// an empty string.
CachedResourceHandle<CachedImage> newImage = nullptr;
@@ -235,8 +242,16 @@
errorEventSender().dispatchEventSoon(*this);
}
+ didUpdateCachedImage(relevantMutation, WTFMove(newImage));
+}
+
+void ImageLoader::didUpdateCachedImage(RelevantMutation relevantMutation, CachedResourceHandle<CachedImage>&& newImage)
+{
+ auto& document = element().document();
+
CachedImage* oldImage = m_image.get();
- if (newImage != oldImage || relevantMutation == RelevantMutation::Yes) {
+ bool loadHasNowLazilyStarted = oldImage == newImage && !m_hasPendingLoadEvent && newImage && newImage->isLoading();
+ if (newImage != oldImage || relevantMutation == RelevantMutation::Yes || loadHasNowLazilyStarted) {
if (m_hasPendingBeforeLoadEvent) {
beforeLoadEventSender().cancelEvent(*this);
m_hasPendingBeforeLoadEvent = false;
@@ -255,9 +270,12 @@
m_hasPendingErrorEvent = false;
}
+ // Due to lazy image loading and the loadsImagesAutomatically setting, it is possible that we did not actually start the image load.
+ bool imageWillBeLoadedLater = newImage && !newImage->isLoading() && newImage->stillNeedsLoad();
+
m_image = newImage;
- m_hasPendingBeforeLoadEvent = !document.isImageDocument() && newImage;
- m_hasPendingLoadEvent = newImage;
+ m_hasPendingBeforeLoadEvent = !document.isImageDocument() && newImage && !imageWillBeLoadedLater;
+ m_hasPendingLoadEvent = newImage && !imageWillBeLoadedLater;
m_imageComplete = !newImage;
if (newImage) {
@@ -269,8 +287,12 @@
} else
updateRenderer();
- if (m_lazyImageLoadState == LazyImageLoadState::Deferred)
- LazyLoadImageObserver::observe(element());
+ if (m_lazyImageLoadState == LazyImageLoadState::Deferred) {
+ if (imageWillBeLoadedLater)
+ LazyLoadImageObserver::observe(element());
+ else // No need to set up a LazyLoadImageObserver if the image is already loaded (e.g. was already cached).
+ m_lazyImageLoadState = LazyImageLoadState::None;
+ }
// If newImage is cached, addClient() will result in the load event
// being queued to fire. Ensure this happens after beforeload is
@@ -597,6 +619,13 @@
updateFromElement(RelevantMutation::No);
}
+void ImageLoader::didStartLoading()
+{
+ if (m_inUpdateFromElement)
+ return;
+
+ didUpdateCachedImage(RelevantMutation::No, CachedResourceHandle { m_image });
+}
void ImageLoader::resetLazyImageLoading(Document& document)
{
if (isDeferred())
Modified: branches/safari-613-branch/Source/WebCore/loader/ImageLoader.h (291661 => 291662)
--- branches/safari-613-branch/Source/WebCore/loader/ImageLoader.h 2022-03-22 17:56:23 UTC (rev 291661)
+++ branches/safari-613-branch/Source/WebCore/loader/ImageLoader.h 2022-03-22 17:56:28 UTC (rev 291662)
@@ -40,7 +40,7 @@
template<typename T> class EventSender;
using ImageEventSender = EventSender<ImageLoader>;
-enum class RelevantMutation : bool { Yes, No };
+enum class RelevantMutation : bool { No, Yes };
class ImageLoader : public CachedImageClient, public CanMakeWeakPtr<ImageLoader> {
WTF_MAKE_FAST_ALLOCATED;
@@ -88,6 +88,7 @@
protected:
explicit ImageLoader(Element&);
void notifyFinished(CachedResource&, const NetworkLoadMetrics&) override;
+ void didStartLoading() override;
private:
void resetLazyImageLoading(Document&);
@@ -97,6 +98,7 @@
virtual String sourceURI(const AtomString&) const = 0;
void updatedHasPendingEvent();
+ void didUpdateCachedImage(RelevantMutation, CachedResourceHandle<CachedImage>&&);
void dispatchPendingBeforeLoadEvent();
void dispatchPendingLoadEvent();
@@ -129,6 +131,7 @@
bool m_imageComplete : 1;
bool m_loadManually : 1;
bool m_elementIsProtected : 1;
+ bool m_inUpdateFromElement : 1;
LazyImageLoadState m_lazyImageLoadState { LazyImageLoadState::None };
};
Modified: branches/safari-613-branch/Source/WebCore/loader/cache/CachedImage.cpp (291661 => 291662)
--- branches/safari-613-branch/Source/WebCore/loader/cache/CachedImage.cpp 2022-03-22 17:56:23 UTC (rev 291661)
+++ branches/safari-613-branch/Source/WebCore/loader/cache/CachedImage.cpp 2022-03-22 17:56:28 UTC (rev 291662)
@@ -99,9 +99,12 @@
void CachedImage::load(CachedResourceLoader& loader)
{
m_skippingRevalidationDocument = loader.document();
- if (loader.shouldPerformImageLoad(url()))
+ if (loader.shouldPerformImageLoad(url())) {
CachedResource::load(loader);
- else
+ CachedResourceClientWalker<CachedImageClient> walker(m_clients);
+ while (auto* client = walker.next())
+ client->didStartLoading();
+ } else
setLoading(false);
}
Modified: branches/safari-613-branch/Source/WebCore/loader/cache/CachedImageClient.h (291661 => 291662)
--- branches/safari-613-branch/Source/WebCore/loader/cache/CachedImageClient.h 2022-03-22 17:56:23 UTC (rev 291661)
+++ branches/safari-613-branch/Source/WebCore/loader/cache/CachedImageClient.h 2022-03-22 17:56:28 UTC (rev 291662)
@@ -49,6 +49,7 @@
virtual VisibleInViewportState imageFrameAvailable(CachedImage& image, ImageAnimatingState, const IntRect* changeRect) { imageChanged(&image, changeRect); return VisibleInViewportState::No; }
virtual VisibleInViewportState imageVisibleInViewport(const Document&) const { return VisibleInViewportState::No; }
+ virtual void didStartLoading() { }
virtual void didRemoveCachedImageClient(CachedImage&) { }
virtual void scheduleRenderingUpdateForImage(CachedImage&) { }
