Changeset 249594 in webkit

Timestamp:

Sep 6, 2019 3:15:51 PM (5 years ago)

Author:

commit-queue@webkit.org

Message:

REGRESSION (r249367): m_decodingPromises grows indefinitely until ImageLoader destruction
​https://bugs.webkit.org/show_bug.cgi?id=201402

Patch by Said Abou-Hallawa <​sabouhallawa@apple.com> on 2019-09-06
Reviewed by Youenn Fablet and Daniel Bates.

Source/WebCore:

Add the static functions resolvePromises() and rejectPromises(). These
functions take an lvalue reference to a Vector of promises. Inside them,
the lvalue reference argument are exchanged with an empty Vector of
promises then the promises are processed. This clears m_decodingPromises
and fixes the leak.

Add an internal API which returns the count of the pending promises of
an HTMLImageElement. This internal API will be used in the attached test.

Test: fast/images/decode-resolve-reject-no-leak.html

• html/HTMLImageElement.h:

(WebCore::HTMLImageElement::pendingDecodePromisesCountForTesting const):

• loader/ImageLoader.cpp:

(WebCore::resolvePromises):
ImageLoader::decode() calls BitmapImage::decode() and moves m_decodingPromises
in capture. When decoding finishes, this function is called to resolve the
promises. But ImageLoader might get deleted before the image decoding
finishes. So this function has to be static.

(WebCore::rejectPromises):
(WebCore::ImageLoader::resolveDecodePromises):
(WebCore::ImageLoader::rejectDecodePromises):
(WebCore::ImageLoader::notifyFinished):
(WebCore::ImageLoader::decode):
(WebCore::resolveDecodePromises): Deleted.
(WebCore::rejectDecodePromises): Deleted.

• loader/ImageLoader.h:

(WebCore::ImageLoader::pendingDecodePromisesCountForTesting const):

• testing/Internals.cpp:

(WebCore::Internals::imagePendingDecodePromisesCountForTesting):

• testing/Internals.h:
• testing/Internals.idl:

LayoutTests:

• fast/images/decode-resolve-reject-no-leak-expected.txt: Added.
• fast/images/decode-resolve-reject-no-leak.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/images/decode-resolve-reject-no-leak-expected.txt (added)
• LayoutTests/fast/images/decode-resolve-reject-no-leak.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/html/HTMLImageElement.h (modified) (1 diff)
• Source/WebCore/loader/ImageLoader.cpp (modified) (8 diffs)
• Source/WebCore/loader/ImageLoader.h (modified) (2 diffs)
• Source/WebCore/testing/Internals.cpp (modified) (1 diff)
• Source/WebCore/testing/Internals.h (modified) (1 diff)
• Source/WebCore/testing/Internals.idl (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2019-09-06  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        REGRESSION (r249367): m_decodingPromises grows indefinitely until ImageLoader destruction
+        https://bugs.webkit.org/show_bug.cgi?id=201402
+
+        Reviewed by Youenn Fablet and Daniel Bates.
+
+        * fast/images/decode-resolve-reject-no-leak-expected.txt: Added.
+        * fast/images/decode-resolve-reject-no-leak.html: Added.
+
 2019-09-06  Said Abou-Hallawa  <sabouhallawa@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-09-06  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        REGRESSION (r249367): m_decodingPromises grows indefinitely until ImageLoader destruction
+        https://bugs.webkit.org/show_bug.cgi?id=201402
+
+        Reviewed by Youenn Fablet and Daniel Bates.
+
+        Add the static functions resolvePromises() and rejectPromises(). These 
+        functions take an lvalue reference to a Vector of promises. Inside them,
+        the lvalue reference argument are exchanged with an empty Vector of
+        promises then the promises are processed. This clears m_decodingPromises
+        and fixes the leak.
+
+        Add an internal API which returns the count of the pending promises of
+        an HTMLImageElement. This internal API will be used in the attached test.
+
+        Test: fast/images/decode-resolve-reject-no-leak.html
+
+        * html/HTMLImageElement.h:
+        (WebCore::HTMLImageElement::pendingDecodePromisesCountForTesting const):
+        * loader/ImageLoader.cpp:
+        (WebCore::resolvePromises):
+        ImageLoader::decode() calls BitmapImage::decode() and moves m_decodingPromises
+        in capture. When decoding finishes, this function is called to resolve the
+        promises. But ImageLoader might get deleted before the image decoding
+        finishes. So this function has to be static.
+
+        (WebCore::rejectPromises):
+        (WebCore::ImageLoader::resolveDecodePromises):
+        (WebCore::ImageLoader::rejectDecodePromises):
+        (WebCore::ImageLoader::notifyFinished):
+        (WebCore::ImageLoader::decode):
+        (WebCore::resolveDecodePromises): Deleted.
+        (WebCore::rejectDecodePromises): Deleted.
+        * loader/ImageLoader.h:
+        (WebCore::ImageLoader::pendingDecodePromisesCountForTesting const):
+        * testing/Internals.cpp:
+        (WebCore::Internals::imagePendingDecodePromisesCountForTesting):
+        * testing/Internals.h:
+        * testing/Internals.idl:
+
 2019-09-06  Said Abou-Hallawa  <sabouhallawa@apple.com>
 

trunk/Source/WebCore/html/HTMLImageElement.h

@@ -105 +105 @@
 
     bool hasPendingActivity() const { return m_imageLoader.hasPendingActivity(); }
+    size_t pendingDecodePromisesCountForTesting() const { return m_imageLoader.pendingDecodePromisesCountForTesting(); }
 
     bool canContainRangeEndPoint() const override { return false; }

trunk/Source/WebCore/loader/ImageLoader.cpp

@@ -276 +276 @@
 }
 
-static inline void resolveDecodePromises(Vector<RefPtr<DeferredPromise>>&& promises)
+static inline void resolvePromises(Vector<RefPtr<DeferredPromise>>& promises)
 {
     ASSERT(!promises.isEmpty());
-    for (auto& promise : promises)
+    auto promisesToBeResolved = std::exchange(promises, { });
+    for (auto& promise : promisesToBeResolved)
         promise->resolve();
 }
 
-static inline void rejectDecodePromises(Vector<RefPtr<DeferredPromise>>&& promises, const char* message)
+static inline void rejectPromises(Vector<RefPtr<DeferredPromise>>& promises, const char* message)
 {
     ASSERT(!promises.isEmpty());
-    for (auto& promise : promises)
+    auto promisesToBeRejected = std::exchange(promises, { });
+    for (auto& promise : promisesToBeRejected)
         promise->reject(Exception { EncodingError, message });
+}
+
+inline void ImageLoader::resolveDecodePromises()
+{
+    resolvePromises(m_decodingPromises);
+}
+
+inline void ImageLoader::rejectDecodePromises(const char* message)
+{
+    rejectPromises(m_decodingPromises, message);
 }
 
@@ -314 +326 @@
 
         if (hasPendingDecodePromises())
-            rejectDecodePromises(WTFMove(m_decodingPromises), "Access control error.");
+            rejectDecodePromises("Access control error.");
         
         ASSERT(!m_hasPendingLoadEvent);
@@ -326 +338 @@
     if (m_image->wasCanceled()) {
         if (hasPendingDecodePromises())
-            rejectDecodePromises(WTFMove(m_decodingPromises), "Loading was canceled.");
+            rejectDecodePromises("Loading was canceled.");
         m_hasPendingLoadEvent = false;
         // Only consider updating the protection ref-count of the Element immediately before returning
@@ -403 +415 @@
 
     if (!element().document().domWindow()) {
-        rejectDecodePromises(WTFMove(m_decodingPromises), "Inactive document.");
+        rejectDecodePromises("Inactive document.");
         return;
     }
@@ -409 +421 @@
     AtomString attr = element().imageSourceURL();
     if (stripLeadingAndTrailingHTMLSpaces(attr).isEmpty()) {
-        rejectDecodePromises(WTFMove(m_decodingPromises), "Missing source URL.");
+        rejectDecodePromises("Missing source URL.");
         return;
     }
@@ -422 +434 @@
     
     if (!element().document().domWindow()) {
-        rejectDecodePromises(WTFMove(m_decodingPromises), "Inactive document.");
+        rejectDecodePromises("Inactive document.");
         return;
     }
 
     if (!m_image || !m_image->image() || m_image->errorOccurred()) {
-        rejectDecodePromises(WTFMove(m_decodingPromises), "Loading error.");
+        rejectDecodePromises("Loading error.");
         return;
     }
@@ -433 +445 @@
     Image* image = m_image->image();
     if (!is<BitmapImage>(image)) {
-        resolveDecodePromises(WTFMove(m_decodingPromises));
+        resolveDecodePromises();
         return;
     }
@@ -439 +451 @@
     auto& bitmapImage = downcast<BitmapImage>(*image);
     bitmapImage.decode([promises = WTFMove(m_decodingPromises)]() mutable {
-        resolveDecodePromises(WTFMove(promises));
+        resolvePromises(promises);
     });
 }

trunk/Source/WebCore/loader/ImageLoader.h

@@ -62 +62 @@
     void clearImage(); // Cancels pending beforeload and load events, and doesn't dispatch new ones.
     
+    size_t pendingDecodePromisesCountForTesting() const { return m_decodingPromises.size(); }
     void decode(Ref<DeferredPromise>&&);
 
@@ -96 +97 @@
 
     bool hasPendingDecodePromises() const { return !m_decodingPromises.isEmpty(); }
+    void resolveDecodePromises();
+    void rejectDecodePromises(const char* message);
     void decode();
     

trunk/Source/WebCore/testing/Internals.cpp

@@ -871 +871 @@
 }
 
+unsigned Internals::imagePendingDecodePromisesCountForTesting(HTMLImageElement& element)
+{
+    return element.pendingDecodePromisesCountForTesting();
+}
+
 void Internals::setClearDecoderAfterAsyncFrameRequestForTesting(HTMLImageElement& element, bool enabled)
 {

trunk/Source/WebCore/testing/Internals.h

@@ -155 +155 @@
     void resetImageAnimation(HTMLImageElement&);
     bool isImageAnimating(HTMLImageElement&);
+    unsigned imagePendingDecodePromisesCountForTesting(HTMLImageElement&);
     void setClearDecoderAfterAsyncFrameRequestForTesting(HTMLImageElement&, bool enabled);
     unsigned imageDecodeCount(HTMLImageElement&);

trunk/Source/WebCore/testing/Internals.idl

@@ -370 +370 @@
     void resetImageAnimation(HTMLImageElement element);
     boolean isImageAnimating(HTMLImageElement element);
+    unsigned long imagePendingDecodePromisesCountForTesting(HTMLImageElement element);
     void setClearDecoderAfterAsyncFrameRequestForTesting(HTMLImageElement element, boolean enabled);
     unsigned long imageDecodeCount(HTMLImageElement element);