- Revision
- 291141
- Author
- cdu...@apple.com
- Date
- 2022-03-10 18:04:19 -0800 (Thu, 10 Mar 2022)
Log Message
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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (291140 => 291141)
--- trunk/LayoutTests/ChangeLog 2022-03-11 01:47:40 UTC (rev 291140)
+++ trunk/LayoutTests/ChangeLog 2022-03-11 02:04:19 UTC (rev 291141)
@@ -1,3 +1,17 @@
+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-10 Matteo Flores <matteo_flo...@apple.com>
[ iOS Mac ] imported/w3c/web-platform-tests/service-workers/service-worker/partitioned-service-worker-matchAll.tentative.https.html is a flaky text failure on all queues
Added: trunk/LayoutTests/fast/dom/lazy-image-loading-document-leak-expected.txt (0 => 291141)
--- trunk/LayoutTests/fast/dom/lazy-image-loading-document-leak-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/dom/lazy-image-loading-document-leak-expected.txt 2022-03-11 02:04:19 UTC (rev 291141)
@@ -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: trunk/LayoutTests/fast/dom/lazy-image-loading-document-leak.html (0 => 291141)
--- trunk/LayoutTests/fast/dom/lazy-image-loading-document-leak.html (rev 0)
+++ trunk/LayoutTests/fast/dom/lazy-image-loading-document-leak.html 2022-03-11 02:04:19 UTC (rev 291141)
@@ -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: trunk/LayoutTests/fast/dom/resources/lazy-image-loading-document-leak-popup.html (0 => 291141)
--- trunk/LayoutTests/fast/dom/resources/lazy-image-loading-document-leak-popup.html (rev 0)
+++ trunk/LayoutTests/fast/dom/resources/lazy-image-loading-document-leak-popup.html 2022-03-11 02:04:19 UTC (rev 291141)
@@ -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: trunk/Source/WebCore/ChangeLog (291140 => 291141)
--- trunk/Source/WebCore/ChangeLog 2022-03-11 01:47:40 UTC (rev 291140)
+++ trunk/Source/WebCore/ChangeLog 2022-03-11 02:04:19 UTC (rev 291141)
@@ -1,3 +1,46 @@
+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-10 Michael Saboff <msab...@apple.com>
Catalyst _javascript_Core, WebCore, WebKitLegacy, and WebKit shouldn't be copied to the Secondary Path
Modified: trunk/Source/WebCore/loader/ImageLoader.cpp (291140 => 291141)
--- trunk/Source/WebCore/loader/ImageLoader.cpp 2022-03-11 01:47:40 UTC (rev 291140)
+++ trunk/Source/WebCore/loader/ImageLoader.cpp 2022-03-11 02:04:19 UTC (rev 291141)
@@ -47,6 +47,7 @@
#include "RenderSVGImage.h"
#include "Settings.h"
#include <wtf/NeverDestroyed.h>
+#include <wtf/Scope.h>
#if ENABLE(VIDEO)
#include "RenderVideo.h"
@@ -106,6 +107,7 @@
, m_imageComplete(true)
, m_loadManually(false)
, m_elementIsProtected(false)
+ , m_inUpdateFromElement(false)
{
}
@@ -177,6 +179,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;
@@ -238,8 +245,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;
@@ -258,9 +273,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
@@ -577,6 +599,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: trunk/Source/WebCore/loader/ImageLoader.h (291140 => 291141)
--- trunk/Source/WebCore/loader/ImageLoader.h 2022-03-11 01:47:40 UTC (rev 291140)
+++ trunk/Source/WebCore/loader/ImageLoader.h 2022-03-11 02:04:19 UTC (rev 291141)
@@ -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;
@@ -89,6 +89,7 @@
protected:
explicit ImageLoader(Element&);
void notifyFinished(CachedResource&, const NetworkLoadMetrics&) override;
+ void didStartLoading() override;
private:
void resetLazyImageLoading(Document&);
@@ -98,6 +99,7 @@
virtual String sourceURI(const AtomString&) const = 0;
void updatedHasPendingEvent();
+ void didUpdateCachedImage(RelevantMutation, CachedResourceHandle<CachedImage>&&);
void dispatchPendingBeforeLoadEvent();
void dispatchPendingLoadEvent();
@@ -130,6 +132,7 @@
bool m_imageComplete : 1;
bool m_loadManually : 1;
bool m_elementIsProtected : 1;
+ bool m_inUpdateFromElement : 1;
LazyImageLoadState m_lazyImageLoadState { LazyImageLoadState::None };
};
Modified: trunk/Source/WebCore/loader/cache/CachedImage.cpp (291140 => 291141)
--- trunk/Source/WebCore/loader/cache/CachedImage.cpp 2022-03-11 01:47:40 UTC (rev 291140)
+++ trunk/Source/WebCore/loader/cache/CachedImage.cpp 2022-03-11 02:04:19 UTC (rev 291141)
@@ -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: trunk/Source/WebCore/loader/cache/CachedImageClient.h (291140 => 291141)
--- trunk/Source/WebCore/loader/cache/CachedImageClient.h 2022-03-11 01:47:40 UTC (rev 291140)
+++ trunk/Source/WebCore/loader/cache/CachedImageClient.h 2022-03-11 02:04:19 UTC (rev 291141)
@@ -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&) { }
