Attachment 349860 Details for Bug 189437 – Patch

[patch] Patch

bug-189437-20180915113909.patch (text/plain), 8.48 KB, created by Simon Fraser (smfr) on 2018-09-15 11:39:10 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Simon Fraser (smfr)

Created: 2018-09-15 11:39:10 PDT

Size: 8.48 KB

patch

obsolete

>Subversion Revision: 235830
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 95f1f42128108ea0ca8c5162e8bedfdfad3d5534..6a7042c483c29c4b0cb9465fc833e1c18ad6e357 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,15 @@
>+2018-09-15  Simon Fraser  <simon.fraser@apple.com>
>+
>+        Many modern media control tests leak documents in testing
>+        https://bugs.webkit.org/show_bug.cgi?id=189437
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        No new tests (OOPS!).
>+
>+        * page/MemoryRelease.cpp:
>+        (WebCore::releaseCriticalMemory):
>+
> 2018-09-08  Simon Fraser  <simon.fraser@apple.com>
> 
>         svg/W3C-SVG-1.1/render-groups-03-t.svg and some other SVG tests leak documents
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 093631b162b39bc6ebb2fb2e628694428578817d..a728f3b58251819813d01b577dcdf19ec5bd45e4 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,26 @@
>+2018-09-15  Simon Fraser  <simon.fraser@apple.com>
>+
>+        Many modern media control tests leak documents in testing
>+        https://bugs.webkit.org/show_bug.cgi?id=189437
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        In order to accurately detect leaks in media controls tests which use lots of
>+        SVGImages, we have to:
>+        - Fire a zero-delay timer after the postTask, in order for ImagesLoader's m_derefElementTimer
>+          to clear references to elements.
>+        - Have releaseCriticalMemory() call CachedResourceLoader's garbageCollectDocumentResources()
>+          to drop the last handle to the CachedResource for an SVGImage.
>+        - Call WKBundleReleaseMemory() after the GC and timer, since we need garbageCollectDocumentResources()
>+          to run again after that timer has fired.
>+        
>+        This should fix all the spurious leak reports involving SVGImage documents.
>+
>+        * WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:
>+        (WKBundlePageCallAfterTasksAndTimers):
>+        (WKBundlePagePostTask): Deleted.
>+        * WebProcess/InjectedBundle/API/c/WKBundlePage.h:
>+
> 2018-09-08  Simon Fraser  <simon.fraser@apple.com>
> 
>         Before tracking Document leaks, clear all caches
>diff --git a/Source/WebCore/page/MemoryRelease.cpp b/Source/WebCore/page/MemoryRelease.cpp
>index 57778bff3a32d7c0a80b152cf480cf2aa315f488..4a67fe04bc0994fe6189319b504ec677c30b0fd7 100644
>--- a/Source/WebCore/page/MemoryRelease.cpp
>+++ b/Source/WebCore/page/MemoryRelease.cpp
>@@ -28,6 +28,7 @@
> 
> #include "CSSFontSelector.h"
> #include "CSSValuePool.h"
>+#include "CachedResourceLoader.h"
> #include "Chrome.h"
> #include "ChromeClient.h"
> #include "CommonVM.h"
>@@ -88,6 +89,7 @@ static void releaseCriticalMemory(Synchronous synchronous)
>     for (auto& document : copyToVectorOf<RefPtr<Document>>(Document::allDocuments())) {
>         document->styleScope().releaseMemory();
>         document->fontSelector().emptyCaches();
>+        document->cachedResourceLoader().garbageCollectDocumentResources();
>     }
> 
>     GCController::singleton().deleteAllCode(JSC::DeleteAllCodeIfNotCollecting);
>diff --git a/Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp b/Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp
>index 24d4f0d449d4071988661dd0c66be00f1073819f..fcd6e1b166d2f4ba836caf824d1efc07fb3f5dd0 100644
>--- a/Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp
>+++ b/Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp
>@@ -620,7 +620,7 @@ void WKBundlePageRegisterScrollOperationCompletionCallback(WKBundlePageRef pageR
>     });
> }
> 
>-void WKBundlePagePostTask(WKBundlePageRef pageRef, WKBundlePageTestNotificationCallback callback, void* context)
>+void WKBundlePageCallAfterTasksAndTimers(WKBundlePageRef pageRef, WKBundlePageTestNotificationCallback callback, void* context)
> {
>     if (!callback)
>         return;
>@@ -634,8 +634,29 @@ void WKBundlePagePostTask(WKBundlePageRef pageRef, WKBundlePageTestNotificationC
>     if (!document)
>         return;
> 
>+    class TimerOwner {
>+    public:
>+        TimerOwner(WTF::Function<void (void*)>&& callback, void* context)
>+            : m_timer(*this, &TimerOwner::timerFired)
>+            , m_callback(WTFMove(callback))
>+            , m_context(context)
>+        {
>+            m_timer.startOneShot(0_s);
>+        }
>+        
>+        void timerFired()
>+        {
>+            m_callback(m_context);
>+            delete this;
>+        }
>+        
>+        WebCore::Timer m_timer;
>+        WTF::Function<void (void*)> m_callback;
>+        void* m_context;
>+    };
>+    
>     document->postTask([=] (WebCore::ScriptExecutionContext&) {
>-        callback(context);
>+        new TimerOwner(callback, context); // deletes itself when done.
>     });
> }
> 
>diff --git a/Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.h b/Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.h
>index 276589125ee8e2bf5e7fe259fd2cc77693e3b120..cd013dfef1b4991d4ada9308289931d2a913e867 100644
>--- a/Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.h
>+++ b/Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.h
>@@ -117,8 +117,8 @@ WK_EXPORT WKStringRef WKBundlePageCopyGroupIdentifier(WKBundlePageRef page);
> typedef void (*WKBundlePageTestNotificationCallback)(void* context);
> WK_EXPORT void WKBundlePageRegisterScrollOperationCompletionCallback(WKBundlePageRef, WKBundlePageTestNotificationCallback, void* context);
> 
>-// Posts a task in the ScriptExecutionContext of the main frame. Used to do work after other tasks have completed.
>-WK_EXPORT void WKBundlePagePostTask(WKBundlePageRef, WKBundlePageTestNotificationCallback, void* context);
>+// Call the given callback after both a postTask() on the page's document's ScriptExecutionContext, and a zero-delay timer.
>+WK_EXPORT void WKBundlePageCallAfterTasksAndTimers(WKBundlePageRef, WKBundlePageTestNotificationCallback, void* context);
> 
> WK_EXPORT void WKBundlePagePostMessage(WKBundlePageRef page, WKStringRef messageName, WKTypeRef messageBody);
> 
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 598254356957f5feb6d72ae14840e744f3598084..b4eb0ab1fc3e1889cb4368b28d5acf0d1f452757 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,25 @@
>+2018-09-15  Simon Fraser  <simon.fraser@apple.com>
>+
>+        Many modern media control tests leak documents in testing
>+        https://bugs.webkit.org/show_bug.cgi?id=189437
>+
>+        Reviewed by NOBODY (OOPS!).
>+        
>+        In order to accurately detect leaks in media controls tests which use lots of
>+        SVGImages, we have to:
>+        - Fire a zero-delay timer after the postTask, in order for ImagesLoader's m_derefElementTimer
>+          to clear references to elements.
>+        - Have releaseCriticalMemory() call CachedResourceLoader's garbageCollectDocumentResources()
>+          to drop the last handle to the CachedResource for an SVGImage.
>+        - Call WKBundleReleaseMemory() after the GC and timer, since we need garbageCollectDocumentResources()
>+          to run again after that timer has fired.
>+        
>+        This should fix all the spurious leak reports involving SVGImage documents.
>+
>+        * WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:
>+        (WTR::InjectedBundle::reportLiveDocuments):
>+        (WTR::InjectedBundle::didReceiveMessageToPage):
>+
> 2018-09-08  Simon Fraser  <simon.fraser@apple.com>
> 
>         Before tracking Document leaks, clear all caches
>diff --git a/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp b/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp
>index 692fb85a512fc2d5c1e45234bff06c1d0846e36f..3b2876fd0102ff1bf54fa6d0e5b018d8c932fc98 100644
>--- a/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp
>+++ b/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp
>@@ -189,6 +189,9 @@ static void postGCTask(void* context)
> 
> void InjectedBundle::reportLiveDocuments(WKBundlePageRef page)
> {
>+    // Release memory again, after the GC and timer fire. This is necessary to clear entries from CachedResourceLoader's m_documentResources in some scenarios.
>+    WKBundleReleaseMemory(m_bundle);
>+
>     const bool excludeDocumentsInPageGroup = true;
>     auto documentURLs = adoptWK(WKBundleGetLiveDocumentURLs(m_bundle, m_pageGroup, excludeDocumentsInPageGroup));
>     
>@@ -287,7 +290,7 @@ void InjectedBundle::didReceiveMessageToPage(WKBundlePageRef page, WKStringRef m
>         WKBundleReleaseMemory(m_bundle);
> 
>         WKRetain(page); // Balanced by the release in postGCTask.
>-        WKBundlePagePostTask(page, postGCTask, (void*)page);
>+        WKBundlePageCallAfterTasksAndTimers(page, postGCTask, (void*)page);
>         return;
>     }
>

Flags:

darin: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 189437: 349851 | 349860