WebPage::close needs to remove all message receivers associated with that WebPage...
authorachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 12 Feb 2019 19:29:21 +0000 (19:29 +0000)
committerachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 12 Feb 2019 19:29:21 +0000 (19:29 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194522
<rdar://problem/47789393>

Reviewed by Chris Dumez.

Source/WebKit:

The InjectedBundle SPI can retain the WebPage or wrapping objects (WKWebProcessPlugInBrowserContextController/WKBundlePageRef).
This can make it so WebPage::close is called before WebPage::~WebPage, and if the SuspendedPageProxy is reused for a subsequent
navigation to the same domain, the WebProcess is reused with a different WebPage instance with the same PageID, which causes problems
when another WebPage registers message handlers and then the previous WebPage is destroyed, which removes both message handlers.

* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::~WebPage):
(WebKit::WebPage::close):
(WebKit::WebPage::mainFrameDidLayout):
* WebProcess/WebPage/WebPage.h:
* WebProcess/WebProcess.h:
(WebKit::WebProcess::eventDispatcher):

Tools:

* TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* TestWebKitAPI/Tests/WebKitCocoa/BundleRetainPagePlugIn.mm: Added.
(-[BundleRetainPagePlugIn webProcessPlugIn:didCreateBrowserContextController:]):
* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

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

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

index fca2de3..429da3b 100644 (file)
@@ -1,3 +1,24 @@
+2019-02-12  Alex Christensen  <achristensen@webkit.org>
+
+        WebPage::close needs to remove all message receivers associated with that WebPage, not WebPage::~WebPage
+        https://bugs.webkit.org/show_bug.cgi?id=194522
+        <rdar://problem/47789393>
+
+        Reviewed by Chris Dumez.
+
+        The InjectedBundle SPI can retain the WebPage or wrapping objects (WKWebProcessPlugInBrowserContextController/WKBundlePageRef).
+        This can make it so WebPage::close is called before WebPage::~WebPage, and if the SuspendedPageProxy is reused for a subsequent
+        navigation to the same domain, the WebProcess is reused with a different WebPage instance with the same PageID, which causes problems
+        when another WebPage registers message handlers and then the previous WebPage is destroyed, which removes both message handlers.
+
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::~WebPage):
+        (WebKit::WebPage::close):
+        (WebKit::WebPage::mainFrameDidLayout):
+        * WebProcess/WebPage/WebPage.h:
+        * WebProcess/WebProcess.h:
+        (WebKit::WebProcess::eventDispatcher):
+
 2019-02-12  Michael Catanzaro  <mcatanzaro@igalia.com>
 
         [WPE][GTK] Unsafe g_unsetenv() use in WebProcessPool::platformInitialize
index 20a5528..a611b26 100644 (file)
@@ -364,7 +364,7 @@ WebPage::WebPage(uint64_t pageID, WebPageCreationParameters&& parameters)
 #endif
     , m_layerHostingMode(parameters.layerHostingMode)
 #if PLATFORM(COCOA) || PLATFORM(GTK)
-    , m_viewGestureGeometryCollector(makeUniqueRef<ViewGestureGeometryCollector>(*this))
+    , m_viewGestureGeometryCollector(std::make_unique<ViewGestureGeometryCollector>(*this))
 #elif HAVE(ACCESSIBILITY) && PLATFORM(GTK)
     , m_accessibilityObject(nullptr)
 #endif
@@ -750,12 +750,6 @@ WebPage::~WebPage()
 {
     ASSERT(!m_page);
 
-    auto& webProcess = WebProcess::singleton();
-#if ENABLE(ASYNC_SCROLLING)
-    if (m_useAsyncScrolling)
-        webProcess.eventDispatcher().removeScrollingTreeForPage(this);
-#endif
-
     platformDetach();
     
     m_sandboxExtensionTracker.invalidate();
@@ -770,16 +764,6 @@ WebPage::~WebPage()
         m_footerBanner->detachFromPage();
 #endif // !PLATFORM(IOS_FAMILY)
 
-    webProcess.removeMessageReceiver(Messages::WebPage::messageReceiverName(), m_pageID);
-
-    // FIXME: This should be done in the object destructors, and the objects themselves should be message receivers.
-    webProcess.removeMessageReceiver(Messages::WebInspector::messageReceiverName(), m_pageID);
-    webProcess.removeMessageReceiver(Messages::WebInspectorUI::messageReceiverName(), m_pageID);
-    webProcess.removeMessageReceiver(Messages::RemoteWebInspectorUI::messageReceiverName(), m_pageID);
-#if ENABLE(FULLSCREEN_API)
-    webProcess.removeMessageReceiver(Messages::WebFullScreenManager::messageReceiverName(), m_pageID);
-#endif
-
 #ifndef NDEBUG
     webPageCounter.decrement();
 #endif
@@ -1318,6 +1302,23 @@ void WebPage::close()
     bool isRunningModal = m_isRunningModal;
     m_isRunningModal = false;
 
+    auto& webProcess = WebProcess::singleton();
+#if ENABLE(ASYNC_SCROLLING)
+    if (m_useAsyncScrolling)
+        webProcess.eventDispatcher().removeScrollingTreeForPage(this);
+#endif
+    webProcess.removeMessageReceiver(Messages::WebPage::messageReceiverName(), m_pageID);
+    // FIXME: This should be done in the object destructors, and the objects themselves should be message receivers.
+    webProcess.removeMessageReceiver(Messages::WebInspector::messageReceiverName(), m_pageID);
+    webProcess.removeMessageReceiver(Messages::WebInspectorUI::messageReceiverName(), m_pageID);
+    webProcess.removeMessageReceiver(Messages::RemoteWebInspectorUI::messageReceiverName(), m_pageID);
+#if ENABLE(FULLSCREEN_API)
+    webProcess.removeMessageReceiver(Messages::WebFullScreenManager::messageReceiverName(), m_pageID);
+#endif
+#if PLATFORM(COCOA) || PLATFORM(GTK)
+    m_viewGestureGeometryCollector = nullptr;
+#endif
+
     // The WebPage can be destroyed by this call.
     WebProcess::singleton().removeWebPage(m_pageID);
 
@@ -4216,7 +4217,8 @@ void WebPage::mainFrameDidLayout()
     }
 
 #if PLATFORM(COCOA) || PLATFORM(GTK)
-    m_viewGestureGeometryCollector->mainFrameDidLayout();
+    if (m_viewGestureGeometryCollector)
+        m_viewGestureGeometryCollector->mainFrameDidLayout();
 #endif
 #if PLATFORM(IOS_FAMILY)
     if (FrameView* frameView = mainFrameView()) {
index a22ed87..03b4d3e 100644 (file)
@@ -1585,7 +1585,7 @@ private:
 #endif
 
 #if PLATFORM(COCOA) || PLATFORM(GTK)
-    UniqueRef<ViewGestureGeometryCollector> m_viewGestureGeometryCollector;
+    std::unique_ptr<ViewGestureGeometryCollector> m_viewGestureGeometryCollector;
 #endif
 
 #if PLATFORM(COCOA)
index 235db77..b9ca2ee 100644 (file)
@@ -165,7 +165,7 @@ public:
     PluginProcessConnectionManager& pluginProcessConnectionManager();
 #endif
 
-    EventDispatcher& eventDispatcher() { return *m_eventDispatcher; }
+    EventDispatcher& eventDispatcher() { return m_eventDispatcher.get(); }
 
     NetworkProcessConnection& ensureNetworkProcessConnection();
     void networkProcessConnectionClosed(NetworkProcessConnection*);
@@ -402,7 +402,7 @@ private:
     HashMap<uint64_t, RefPtr<WebPageGroupProxy>> m_pageGroupMap;
     RefPtr<InjectedBundle> m_injectedBundle;
 
-    RefPtr<EventDispatcher> m_eventDispatcher;
+    Ref<EventDispatcher> m_eventDispatcher;
 #if PLATFORM(IOS_FAMILY)
     RefPtr<ViewUpdateDispatcher> m_viewUpdateDispatcher;
 #endif
index 9d83ea3..d9df508 100644 (file)
@@ -1,3 +1,16 @@
+2019-02-12  Alex Christensen  <achristensen@webkit.org>
+
+        WebPage::close needs to remove all message receivers associated with that WebPage, not WebPage::~WebPage
+        https://bugs.webkit.org/show_bug.cgi?id=194522
+        <rdar://problem/47789393>
+
+        Reviewed by Chris Dumez.
+
+        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
+        * TestWebKitAPI/Tests/WebKitCocoa/BundleRetainPagePlugIn.mm: Added.
+        (-[BundleRetainPagePlugIn webProcessPlugIn:didCreateBrowserContextController:]):
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
 2019-02-12  Andy Estes  <aestes@apple.com>
 
         [iOSMac] Enable Parental Controls Content Filtering
index e6c27f6..d3bd644 100644 (file)
                5C4A84951F7EEFFC00ACFC54 /* Configuration.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5C4A84941F7EEFD400ACFC54 /* Configuration.mm */; };
                5C69BDD51F82A7EF000F4F4B /* JavaScriptDuringNavigation.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5C69BDD41F82A7EB000F4F4B /* JavaScriptDuringNavigation.mm */; };
                5C7148952123A40A00FDE3C5 /* WKWebsiteDatastore.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5C7148942123A40700FDE3C5 /* WKWebsiteDatastore.mm */; };
+               5C75716122124C5200B9E5AC /* BundleRetainPagePlugIn.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5C75715F221249BD00B9E5AC /* BundleRetainPagePlugIn.mm */; };
                5C7964101EB0278D0075D74C /* EventModifiers.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 5C79640F1EB0269B0075D74C /* EventModifiers.cpp */; };
                5C7C74CB1FB529BA002F9ABE /* WebViewScheduleInRunLoop.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5C7C74CA1FB528D4002F9ABE /* WebViewScheduleInRunLoop.mm */; };
                5C838F7F1DB04F900082858F /* LoadInvalidURLRequest.mm in Sources */ = {isa = PBXBuildFile; fileRef = 57901FAE1CAF137100ED64F9 /* LoadInvalidURLRequest.mm */; };
                5C5E633D1D0B67940085A025 /* UniqueRef.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = UniqueRef.cpp; sourceTree = "<group>"; };
                5C69BDD41F82A7EB000F4F4B /* JavaScriptDuringNavigation.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = JavaScriptDuringNavigation.mm; sourceTree = "<group>"; };
                5C7148942123A40700FDE3C5 /* WKWebsiteDatastore.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = WKWebsiteDatastore.mm; sourceTree = "<group>"; };
+               5C75715F221249BD00B9E5AC /* BundleRetainPagePlugIn.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = BundleRetainPagePlugIn.mm; sourceTree = "<group>"; };
                5C79640F1EB0269B0075D74C /* EventModifiers.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = EventModifiers.cpp; sourceTree = "<group>"; };
                5C7C74CA1FB528D4002F9ABE /* WebViewScheduleInRunLoop.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = WebViewScheduleInRunLoop.mm; sourceTree = "<group>"; };
                5C8BC798218CF3E900813886 /* NetworkProcess.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = NetworkProcess.mm; sourceTree = "<group>"; };
                                37A709AD1E3EA8B000CA5969 /* BundleRangeHandle.mm */,
                                37A709AA1E3EA79000CA5969 /* BundleRangeHandlePlugIn.mm */,
                                37A709AC1E3EA7E800CA5969 /* BundleRangeHandleProtocol.h */,
+                               5C75715F221249BD00B9E5AC /* BundleRetainPagePlugIn.mm */,
                                1C2B817E1C891E4200A5529F /* CancelFontSubresource.mm */,
                                1C2B81811C891EFA00A5529F /* CancelFontSubresourcePlugIn.mm */,
                                5CB18BA71F5645B200EE23C4 /* ClickAutoFillButton.mm */,
                                374B7A611DF371CF00ACCB6C /* BundleEditingDelegatePlugIn.mm in Sources */,
                                A13EBBB01B87436F00097110 /* BundleParametersPlugIn.mm in Sources */,
                                37A709AF1E3EA97E00CA5969 /* BundleRangeHandlePlugIn.mm in Sources */,
+                               5C75716122124C5200B9E5AC /* BundleRetainPagePlugIn.mm in Sources */,
                                1C2B81831C891F0900A5529F /* CancelFontSubresourcePlugIn.mm in Sources */,
                                5CB18BA81F5645E300EE23C4 /* ClickAutoFillButton.mm in Sources */,
                                A14FC58B1B89927100D107EB /* ContentFilteringPlugIn.mm in Sources */,
diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/BundleRetainPagePlugIn.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/BundleRetainPagePlugIn.mm
new file mode 100644 (file)
index 0000000..b8f2498
--- /dev/null
@@ -0,0 +1,45 @@
+/*
+ * Copyright (C) 2019 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"
+
+#if WK_API_ENABLED
+
+#import <WebKit/WKWebProcessPlugIn.h>
+#import <wtf/RetainPtr.h>
+
+@interface BundleRetainPagePlugIn : NSObject <WKWebProcessPlugIn>
+@end
+
+@implementation BundleRetainPagePlugIn
+
+- (void)webProcessPlugIn:(WKWebProcessPlugInController *)plugInController didCreateBrowserContextController:(WKWebProcessPlugInBrowserContextController *)browserContextController
+{
+    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 0.5 * NSEC_PER_SEC), dispatch_get_main_queue(), [retainedPage = retainPtr(browserContextController)] { });
+}
+
+@end
+
+#endif // WK_API_ENABLED
index 71cff60..a513b42 100644 (file)
@@ -2404,11 +2404,18 @@ setInterval(() => {
 </body>
 )PSONRESOURCE";
 
-TEST(ProcessSwap, ReuseSuspendedProcessForRegularNavigation)
+enum class RetainPageInBundle { No, Yes };
+
+void testReuseSuspendedProcessForRegularNavigation(RetainPageInBundle retainPageInBundle)
 {
     auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
     [processPoolConfiguration setProcessSwapsOnNavigation:YES];
+    if (retainPageInBundle == RetainPageInBundle::Yes)
+        [processPoolConfiguration setInjectedBundleURL:[[NSBundle mainBundle] URLForResource:@"TestWebKitAPI" withExtension:@"wkbundle"]];
+
     auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+    if (retainPageInBundle == RetainPageInBundle::Yes)
+        [processPool _setObject:@"BundleRetainPagePlugIn" forBundleParameter:TestWebKitAPI::Util::TestPlugInClassNameParameter];
 
     auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
     [webViewConfiguration setProcessPool:processPool.get()];
@@ -2446,6 +2453,16 @@ TEST(ProcessSwap, ReuseSuspendedProcessForRegularNavigation)
     EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
 }
 
+TEST(ProcessSwap, ReuseSuspendedProcessForRegularNavigationRetainBundlePage)
+{
+    testReuseSuspendedProcessForRegularNavigation(RetainPageInBundle::Yes);
+}
+
+TEST(ProcessSwap, ReuseSuspendedProcessForRegularNavigation)
+{
+    testReuseSuspendedProcessForRegularNavigation(RetainPageInBundle::No);
+}
+
 static const char* mainFramesOnlyMainFrame = R"PSONRESOURCE(
 <script>
 function loaded() {