ASSERTION FAILURE (r220931): !m_function in ~CompletionHandler() after switch tabs
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 1 Jun 2020 16:30:53 +0000 (16:30 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 1 Jun 2020 16:30:53 +0000 (16:30 +0000)
https://bugs.webkit.org/show_bug.cgi?id=212537
<rdar://problem/63766838>

Reviewed by Alex Christensen.

Source/WebKit:

When WebPage::markAllLayersVolatile(), it would destroy m_pageMarkingLayersAsVolatileCounter,
which may not have called its completion handler yet. As a result, we would hit an assertion
in the CompletionHandler destructor.

* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::callVolatilityCompletionHandlers):
(WebKit::WebPage::markLayersVolatile):
(WebKit::WebPage::cancelMarkLayersVolatile):
* WebProcess/WebPage/WebPage.h:
(WebKit::WebPage::markLayersVolatile):
* WebProcess/WebProcess.cpp:
(WebKit::WebProcess::prepareToSuspend):
(WebKit::WebProcess::markAllLayersVolatile):
(WebKit::WebProcess::cancelMarkAllLayersVolatile):
* WebProcess/WebProcess.h:

Tools:

Add API test coverage.

* TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* TestWebKitAPI/Tests/WebKitCocoa/ProcessSuspension.mm: Added.
(TEST):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@262380 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/WebProcess/WebPage/WebPage.cpp
Source/WebKit/WebProcess/WebPage/WebPage.h
Source/WebKit/WebProcess/WebProcess.cpp
Source/WebKit/WebProcess/WebProcess.h
Tools/ChangeLog
Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSuspension.mm [new file with mode: 0644]

index d239550..c84e347 100644 (file)
@@ -1,3 +1,27 @@
+2020-06-01  Chris Dumez  <cdumez@apple.com>
+
+        ASSERTION FAILURE (r220931): !m_function in ~CompletionHandler() after switch tabs
+        https://bugs.webkit.org/show_bug.cgi?id=212537
+        <rdar://problem/63766838>
+
+        Reviewed by Alex Christensen.
+
+        When WebPage::markAllLayersVolatile(), it would destroy m_pageMarkingLayersAsVolatileCounter,
+        which may not have called its completion handler yet. As a result, we would hit an assertion
+        in the CompletionHandler destructor.
+
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::callVolatilityCompletionHandlers):
+        (WebKit::WebPage::markLayersVolatile):
+        (WebKit::WebPage::cancelMarkLayersVolatile):
+        * WebProcess/WebPage/WebPage.h:
+        (WebKit::WebPage::markLayersVolatile):
+        * WebProcess/WebProcess.cpp:
+        (WebKit::WebProcess::prepareToSuspend):
+        (WebKit::WebProcess::markAllLayersVolatile):
+        (WebKit::WebProcess::cancelMarkAllLayersVolatile):
+        * WebProcess/WebProcess.h:
+
 2020-06-01  Sam Weinig  <weinig@apple.com>
 
         Extended Color: Replace Color constructors taking numeric values with type specific factory functions
index 0af6311..0fc2f07 100644 (file)
@@ -2576,7 +2576,7 @@ void WebPage::updateDrawingAreaLayerTreeFreezeState()
 
 void WebPage::callVolatilityCompletionHandlers(bool succeeded)
 {
-    auto completionHandlers = WTFMove(m_markLayersAsVolatileCompletionHandlers);
+    auto completionHandlers = std::exchange(m_markLayersAsVolatileCompletionHandlers, { });
     for (auto& completionHandler : completionHandlers)
         completionHandler(succeeded);
 }
@@ -2604,7 +2604,7 @@ bool WebPage::markLayersVolatileImmediatelyIfPossible()
     return !drawingArea() || drawingArea()->markLayersVolatileImmediatelyIfPossible();
 }
 
-void WebPage::markLayersVolatile(WTF::Function<void (bool)>&& completionHandler)
+void WebPage::markLayersVolatile(CompletionHandler<void(bool)>&& completionHandler)
 {
     RELEASE_LOG_IF_ALLOWED(Layers, "markLayersVolatile:");
 
@@ -2634,7 +2634,7 @@ void WebPage::cancelMarkLayersVolatile()
 {
     RELEASE_LOG_IF_ALLOWED(Layers, "cancelMarkLayersVolatile:");
     m_layerVolatilityTimer.stop();
-    m_markLayersAsVolatileCompletionHandlers.clear();
+    callVolatilityCompletionHandlers(false);
 }
 
 class CurrentEvent {
index 3c984ce..b217bc7 100644 (file)
@@ -773,7 +773,7 @@ public:
     void freezeLayerTree(LayerTreeFreezeReason);
     void unfreezeLayerTree(LayerTreeFreezeReason);
 
-    void markLayersVolatile(Function<void(bool)>&& completionHandler = { });
+    void markLayersVolatile(CompletionHandler<void(bool)>&& completionHandler = { });
     void cancelMarkLayersVolatile();
 
     void freezeLayerTreeDueToSwipeAnimation();
@@ -2031,7 +2031,7 @@ private:
 #endif
 
     WebCore::Timer m_layerVolatilityTimer;
-    Vector<Function<void(bool)>> m_markLayersAsVolatileCompletionHandlers;
+    Vector<CompletionHandler<void(bool)>> m_markLayersAsVolatileCompletionHandlers;
     bool m_isSuspendedUnderLock { false };
 
     HashSet<String, ASCIICaseInsensitiveHash> m_mimeTypesWithCustomContentProviders;
index 7ed17b8..5cfb2dc 100644 (file)
 #include <WebCore/Settings.h>
 #include <WebCore/UserGestureIndicator.h>
 #include <wtf/Algorithms.h>
+#include <wtf/CallbackAggregator.h>
 #include <wtf/Language.h>
 #include <wtf/ProcessPrivilege.h>
 #include <wtf/RunLoop.h>
@@ -1430,44 +1431,29 @@ void WebProcess::prepareToSuspend(bool isSuspensionImminent, CompletionHandler<v
     updateFreezerStatus();
 #endif
 
-    markAllLayersVolatile([this, completionHandler = WTFMove(completionHandler)](bool success) mutable {
-        if (success)
-            RELEASE_LOG_IF_ALLOWED(ProcessSuspension, "markAllLayersVolatile: Successfuly marked all layers as volatile");
-        else
-            RELEASE_LOG_IF_ALLOWED(ProcessSuspension, "markAllLayersVolatile: Failed to mark all layers as volatile");
-
+    markAllLayersVolatile([this, completionHandler = WTFMove(completionHandler)]() mutable {
         RELEASE_LOG_IF_ALLOWED(ProcessSuspension, "prepareToSuspend: Process is ready to suspend");
         completionHandler();
     });
 }
 
-void WebProcess::markAllLayersVolatile(WTF::Function<void(bool)>&& completionHandler)
+void WebProcess::markAllLayersVolatile(CompletionHandler<void()>&& completionHandler)
 {
     RELEASE_LOG_IF_ALLOWED(ProcessSuspension, "markAllLayersVolatile:");
-    ASSERT(!m_pageMarkingLayersAsVolatileCounter);
-    m_countOfPagesFailingToMarkVolatile = 0;
-
-    m_pageMarkingLayersAsVolatileCounter = makeUnique<PageMarkingLayersAsVolatileCounter>([this, completionHandler = WTFMove(completionHandler)] (RefCounterEvent) {
-        if (m_pageMarkingLayersAsVolatileCounter->value())
-            return;
-
-        completionHandler(m_countOfPagesFailingToMarkVolatile == 0);
-        m_pageMarkingLayersAsVolatileCounter = nullptr;
-    });
-    auto token = m_pageMarkingLayersAsVolatileCounter->count();
-    for (auto& page : m_pageMap.values())
-        page->markLayersVolatile([token, this] (bool succeeded) {
-            if (!succeeded)
-                ++m_countOfPagesFailingToMarkVolatile;
+    auto callbackAggregator = CallbackAggregator::create(WTFMove(completionHandler));
+    for (auto& page : m_pageMap.values()) {
+        page->markLayersVolatile([this, callbackAggregator = callbackAggregator.copyRef(), pageID = page->identifier()] (bool succeeded) {
+            if (succeeded)
+                RELEASE_LOG_IF_ALLOWED(ProcessSuspension, "markAllLayersVolatile: Successfuly marked layers as volatile for webPageID=%" PRIu64, pageID.toUInt64());
+            else
+                RELEASE_LOG_ERROR_IF_ALLOWED(ProcessSuspension, "markAllLayersVolatile: Failed to mark layers as volatile for webPageID=%" PRIu64, pageID.toUInt64());
         });
+    }
 }
 
 void WebProcess::cancelMarkAllLayersVolatile()
 {
-    if (!m_pageMarkingLayersAsVolatileCounter)
-        return;
-
-    m_pageMarkingLayersAsVolatileCounter = nullptr;
+    RELEASE_LOG_IF_ALLOWED(ProcessSuspension, "cancelMarkAllLayersVolatile:");
     for (auto& page : m_pageMap.values())
         page->cancelMarkLayersVolatile();
 }
index b8c9e7c..a85fef3 100644 (file)
@@ -342,7 +342,7 @@ private:
     void registerWithStateDumper();
 #endif
 
-    void markAllLayersVolatile(WTF::Function<void(bool)>&& completionHandler);
+    void markAllLayersVolatile(CompletionHandler<void()>&&);
     void cancelMarkAllLayersVolatile();
 
     void freezeAllLayerTrees();
@@ -581,11 +581,6 @@ private:
     std::unique_ptr<ProcessAssertion> m_uiProcessDependencyProcessAssertion;
 #endif
 
-    enum PageMarkingLayersAsVolatileCounterType { };
-    using PageMarkingLayersAsVolatileCounter = RefCounter<PageMarkingLayersAsVolatileCounterType>;
-    std::unique_ptr<PageMarkingLayersAsVolatileCounter> m_pageMarkingLayersAsVolatileCounter;
-    unsigned m_countOfPagesFailingToMarkVolatile { 0 };
-
     bool m_suppressMemoryPressureHandler { false };
 #if PLATFORM(MAC)
     std::unique_ptr<WebCore::CPUMonitor> m_cpuMonitor;
index b6ac19a..ab1b3db 100644 (file)
@@ -1,3 +1,17 @@
+2020-06-01  Chris Dumez  <cdumez@apple.com>
+
+        ASSERTION FAILURE (r220931): !m_function in ~CompletionHandler() after switch tabs
+        https://bugs.webkit.org/show_bug.cgi?id=212537
+        <rdar://problem/63766838>
+
+        Reviewed by Alex Christensen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSuspension.mm: Added.
+        (TEST):
+
 2020-06-01  Carlos Alberto Lopez Perez  <clopez@igalia.com>
 
         [EWS] Add a special case for running the layout test step without aborting in case of many failures for WPT tests
index b7d67b2..5800565 100644 (file)
                83779C381F82FECE007CDA8A /* VisitedLinkStore.mm in Sources */ = {isa = PBXBuildFile; fileRef = 83779C371F82FEB0007CDA8A /* VisitedLinkStore.mm */; };
                837A35F11D9A1E7D00663C57 /* DownloadRequestBlobURL.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 837A35F01D9A1E6400663C57 /* DownloadRequestBlobURL.html */; };
                839AA35F21A26B0300980DD6 /* client-side-redirect.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 839AA35E21A26ACD00980DD6 /* client-side-redirect.html */; };
+               83A2CFDE2481B3520018B2D3 /* ProcessSuspension.mm in Sources */ = {isa = PBXBuildFile; fileRef = 83A2CFDC2481B32F0018B2D3 /* ProcessSuspension.mm */; };
                83B6DE6F1EE75221001E792F /* RestoreSessionState.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 83B6DE6E1EE7520F001E792F /* RestoreSessionState.cpp */; };
                83BAEE8D1EF4625500DDE894 /* PluginLoadClientPolicies.mm in Sources */ = {isa = PBXBuildFile; fileRef = 83BAEE8C1EF4625500DDE894 /* PluginLoadClientPolicies.mm */; };
                83BC5AC020E6C0DF00F5879F /* StartLoadInDidFailProvisionalLoad.mm in Sources */ = {isa = PBXBuildFile; fileRef = 83BC5ABF20E6C0D300F5879F /* StartLoadInDidFailProvisionalLoad.mm */; };
                83779C371F82FEB0007CDA8A /* VisitedLinkStore.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = VisitedLinkStore.mm; sourceTree = "<group>"; };
                837A35F01D9A1E6400663C57 /* DownloadRequestBlobURL.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = DownloadRequestBlobURL.html; sourceTree = "<group>"; };
                839AA35E21A26ACD00980DD6 /* client-side-redirect.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "client-side-redirect.html"; sourceTree = "<group>"; };
+               83A2CFDC2481B32F0018B2D3 /* ProcessSuspension.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = ProcessSuspension.mm; sourceTree = "<group>"; };
                83B6DE6E1EE7520F001E792F /* RestoreSessionState.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = RestoreSessionState.cpp; sourceTree = "<group>"; };
                83B88A331C80056D00BB2418 /* HTMLParserIdioms.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = HTMLParserIdioms.cpp; sourceTree = "<group>"; };
                83BAEE8C1EF4625500DDE894 /* PluginLoadClientPolicies.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = PluginLoadClientPolicies.mm; sourceTree = "<group>"; };
                                7C1AF7931E8DCBAB002645B9 /* PrepareForMoveToWindow.mm */,
                                41882F0221010A70002FF288 /* ProcessPreWarming.mm */,
                                CDA4438D21F7A47700379489 /* ProcessSuspendMediaBuffering.mm */,
+                               83A2CFDC2481B32F0018B2D3 /* ProcessSuspension.mm */,
                                518C1152205B04F9001FF4AE /* ProcessSwapOnNavigation.mm */,
                                5798E2AF1CAF5C2800C5CBA0 /* ProvisionalURLNotChange.mm */,
                                5CFACF64226FD1FB0056C7D0 /* Proxy.mm */,
                                4647B1261EBA3B850041D7EF /* ProcessDidTerminate.cpp in Sources */,
                                41882F0321010C0D002FF288 /* ProcessPreWarming.mm in Sources */,
                                CDA4438E21F7A47700379489 /* ProcessSuspendMediaBuffering.mm in Sources */,
+                               83A2CFDE2481B3520018B2D3 /* ProcessSuspension.mm in Sources */,
                                518C1153205B0504001FF4AE /* ProcessSwapOnNavigation.mm in Sources */,
                                7C83E0C11D0A652F00FEBCF3 /* ProvisionalURLNotChange.mm in Sources */,
                                5CFACF65226FD2DC0056C7D0 /* Proxy.mm in Sources */,
diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSuspension.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSuspension.mm
new file mode 100644 (file)
index 0000000..73d5b0c
--- /dev/null
@@ -0,0 +1,53 @@
+/*
+ * Copyright (C) 2020 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#import "config.h"
+
+#import "PlatformUtilities.h"
+#import "TestWKWebView.h"
+#import <WebKit/WKWebViewConfiguration.h>
+#import <WebKit/WKWebViewPrivate.h>
+#import <WebKit/WKWebViewPrivateForTesting.h>
+
+TEST(ProcessSuspension, CancelWebProcessSuspension)
+{
+    auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 800, 600) configuration:configuration.get() addToWindow:YES]);
+    [webView synchronouslyLoadTestPageNamed:@"large-red-square-image"];
+
+    auto pid1 = [webView _webProcessIdentifier];
+    EXPECT_NE(pid1, 0);
+
+    [webView _processWillSuspendImminentlyForTesting];
+    [webView _processDidResumeForTesting];
+
+    bool done = false;
+    [webView evaluateJavaScript:@"window.location" completionHandler: [&] (id result, NSError *error) {
+        auto pid2 = [webView _webProcessIdentifier];
+        EXPECT_EQ(pid1, pid2);
+        done = true;
+    }];
+    TestWebKitAPI::Util::run(&done);
+}