RemoteObjectRegistry message receiver should be removed when WebPage:…

…:close is called instead of waiting until dealloc https://bugs.webkit.org/show_bug.cgi?id=196744 <rdar://49415309> Patch by Alex Christensen <achristensen@webkit.org> on 2019-04-10 Reviewed by Chris Dumez. Source/WebKit: This is a similar problem to the one I fixed in r241306 so I piggy-backed on the same test. When you do a cross site navigation but the previous page is in a suspended process then you navigate back, you can get two WebPage objects in the same process with the same IDs. WebPage::close has been called on the old one which is supposed to make it so all the message receivers associated with it have been removed so we don't have any loss of communication, but we missed the RemoteObjectRegistry messages, which are owned by the ObjC bundle object wrapping the WebPage (which can keep it alive if a strong reference to it is held). To fix the assertion that happens in this case and the resulting communication breakage, teach the WebPage about these messages so it can tear down the message receiver with the others it removes at close time. * Shared/API/Cocoa/RemoteObjectRegistry.h: * WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm: (-[WKWebProcessPlugInBrowserContextController dealloc]): (-[WKWebProcessPlugInBrowserContextController _remoteObjectRegistry]): * WebProcess/WebPage/Cocoa/WebPageCocoa.mm: (WebKit::WebPage::addRemoteObjectRegistry): * WebProcess/WebPage/WebPage.cpp: (WebKit::WebPage::close): * WebProcess/WebPage/WebPage.h: Tools: * TestWebKitAPI/Tests/WebKitCocoa/BundleRetainPagePlugIn.mm: (-[BundleRetainPagePlugIn webProcessPlugIn:didCreateBrowserContextController:]): Canonical link: https://commits.webkit.org/211057@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@244139 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Apr 10, 2019 · 32f88d3b49f75c2165743454bc9b02b202504f09 · 32f88d3
1 parent 0e6d307
commit 32f88d3b49f75c2165743454bc9b02b202504f09
Showing 10 changed files with 86 additions and 16 deletions.
diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
@@ -1,3 +1,30 @@
+2019-04-10  Alex Christensen  <achristensen@webkit.org>
+
+        RemoteObjectRegistry message receiver should be removed when WebPage::close is called instead of waiting until dealloc
+        https://bugs.webkit.org/show_bug.cgi?id=196744
+        <rdar://49415309>
+
+        Reviewed by Chris Dumez.
+
+        This is a similar problem to the one I fixed in r241306 so I piggy-backed on the same test.
+        When you do a cross site navigation but the previous page is in a suspended process then you navigate back,
+        you can get two WebPage objects in the same process with the same IDs.  WebPage::close has been called
+        on the old one which is supposed to make it so all the message receivers associated with it have been removed
+        so we don't have any loss of communication, but we missed the RemoteObjectRegistry messages, which are owned
+        by the ObjC bundle object wrapping the WebPage (which can keep it alive if a strong reference to it is held).
+        To fix the assertion that happens in this case and the resulting communication breakage, teach the WebPage about
+        these messages so it can tear down the message receiver with the others it removes at close time.
+
+        * Shared/API/Cocoa/RemoteObjectRegistry.h:
+        * WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:
+        (-[WKWebProcessPlugInBrowserContextController dealloc]):
+        (-[WKWebProcessPlugInBrowserContextController _remoteObjectRegistry]):
+        * WebProcess/WebPage/Cocoa/WebPageCocoa.mm:
+        (WebKit::WebPage::addRemoteObjectRegistry):
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::close):
+        * WebProcess/WebPage/WebPage.h:
+
 2019-04-10  Chris Dumez  <cdumez@apple.com>
 
         Unreviewed, drop SuspendedPageProxy data member that is unused after r244075.

diff --git a/Source/WebKit/Shared/API/Cocoa/RemoteObjectRegistry.h b/Source/WebKit/Shared/API/Cocoa/RemoteObjectRegistry.h
@@ -23,12 +23,13 @@
  * THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#ifndef RemoteObjectRegistry_h
-#define RemoteObjectRegistry_h
+#pragma once
 
 #include "MessageReceiver.h"
 #include "ProcessThrottler.h"
 #include <wtf/Function.h>
+#include <wtf/WeakObjCPtr.h>
+#include <wtf/WeakPtr.h>
 
 OBJC_CLASS _WKRemoteObjectRegistry;
 
@@ -43,7 +44,7 @@ class UserData;
 class WebPage;
 class WebPageProxy;
 
-class RemoteObjectRegistry final : public IPC::MessageReceiver {
+class RemoteObjectRegistry final : public CanMakeWeakPtr<RemoteObjectRegistry>, public IPC::MessageReceiver {
     WTF_MAKE_FAST_ALLOCATED;
 public:
     RemoteObjectRegistry(_WKRemoteObjectRegistry *, WebPage&);
@@ -55,6 +56,8 @@ class RemoteObjectRegistry final : public IPC::MessageReceiver {
     void sendReplyBlock(uint64_t replyID, const UserData& blockInvocation);
     void sendUnusedReply(uint64_t replyID);
 
+    void close();
+
 private:
     // IPC::MessageReceiver
     void didReceiveMessage(IPC::Connection&, IPC::Decoder&) override;
@@ -64,13 +67,13 @@ class RemoteObjectRegistry final : public IPC::MessageReceiver {
     void callReplyBlock(uint64_t replyID, const UserData& blockInvocation);
     void releaseUnusedReplyBlock(uint64_t replyID);
 
-    _WKRemoteObjectRegistry *m_remoteObjectRegistry;
+    WeakObjCPtr<_WKRemoteObjectRegistry> m_remoteObjectRegistry;
     IPC::MessageSender& m_messageSender;
-    WTF::Function<ProcessThrottler::BackgroundActivityToken()> m_takeBackgroundActivityToken;
-    WTF::Function<void()> m_launchInitialProcessIfNecessary;
+    Function<ProcessThrottler::BackgroundActivityToken()> m_takeBackgroundActivityToken;
+    Function<void()> m_launchInitialProcessIfNecessary;
     HashMap<uint64_t, ProcessThrottler::BackgroundActivityToken> m_pendingReplies;
+    bool m_isRegisteredAsMessageReceiver { false };
+    uint64_t m_messageReceiverID { 0 };
 };
 
 } // namespace WebKit
-
-#endif // RemoteObjectRegistry_h
diff --git a/Source/WebKit/Shared/API/Cocoa/RemoteObjectRegistry.mm b/Source/WebKit/Shared/API/Cocoa/RemoteObjectRegistry.mm
@@ -33,6 +33,7 @@
 #import "UserData.h"
 #import "WebPage.h"
 #import "WebPageProxy.h"
+#import "WebProcess.h"
 #import "WebProcessProxy.h"
 #import "_WKRemoteObjectRegistryInternal.h"
 
@@ -42,7 +43,11 @@
     : m_remoteObjectRegistry(remoteObjectRegistry)
     , m_messageSender(page)
     , m_takeBackgroundActivityToken([] { return ProcessThrottler::BackgroundActivityToken(); })
+    , m_isRegisteredAsMessageReceiver(true)
+    , m_messageReceiverID(page.pageID())
 {
+    WebProcess::singleton().addMessageReceiver(Messages::RemoteObjectRegistry::messageReceiverName(), m_messageReceiverID, *this);
+    page.setRemoteObjectRegistry(*this);
 }
 
 RemoteObjectRegistry::RemoteObjectRegistry(_WKRemoteObjectRegistry *remoteObjectRegistry, WebPageProxy& page)
@@ -53,9 +58,17 @@
 {
 }
 
-
 RemoteObjectRegistry::~RemoteObjectRegistry()
 {
+    close();
+}
+
+void RemoteObjectRegistry::close()
+{
+    if (m_isRegisteredAsMessageReceiver) {
+        WebProcess::singleton().removeMessageReceiver(Messages::RemoteObjectRegistry::messageReceiverName(), m_messageReceiverID);
+        m_isRegisteredAsMessageReceiver = false;
+    }
 }
 
 void RemoteObjectRegistry::sendInvocation(const RemoteObjectInvocation& invocation)

diff --git a/Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm b/Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm
@@ -36,6 +36,7 @@
 #import "WKRemoteObject.h"
 #import "WKRemoteObjectCoder.h"
 #import "WKSharedAPICast.h"
+#import "WebPage.h"
 #import "_WKRemoteObjectInterface.h"
 #import <objc/runtime.h>
 

diff --git a/...ce/WebKit/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm b/...ce/WebKit/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm
@@ -350,10 +350,8 @@ - (void)setLoadDelegate:(id <WKWebProcessPlugInLoadDelegate>)loadDelegate
 
 - (void)dealloc
 {
-    if (_remoteObjectRegistry) {
-        WebKit::WebProcess::singleton().removeMessageReceiver(Messages::RemoteObjectRegistry::messageReceiverName(), _page->pageID());
+    if (_remoteObjectRegistry)
         [_remoteObjectRegistry _invalidate];
-    }
 
     _page->~WebPage();
 
@@ -416,10 +414,8 @@ + (instancetype)lookUpBrowsingContextFromHandle:(WKBrowsingContextHandle *)handl
 
 - (_WKRemoteObjectRegistry *)_remoteObjectRegistry
 {
-    if (!_remoteObjectRegistry) {
+    if (!_remoteObjectRegistry)
         _remoteObjectRegistry = adoptNS([[_WKRemoteObjectRegistry alloc] _initWithWebPage:*_page]);
-        WebKit::WebProcess::singleton().addMessageReceiver(Messages::RemoteObjectRegistry::messageReceiverName(), _page->pageID(), [_remoteObjectRegistry remoteObjectRegistry]);
-    }
 
     return _remoteObjectRegistry.get();
 }

diff --git a/Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm b/Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm
@@ -26,10 +26,10 @@
 #import "config.h"
 #import "WebPage.h"
 
-
 #import "AttributedString.h"
 #import "LoadParameters.h"
 #import "PluginView.h"
+#import "RemoteObjectRegistry.h"
 #import "WebPageProxyMessages.h"
 #import "WebPaymentCoordinator.h"
 #import <WebCore/DictionaryLookup.h>
@@ -222,6 +222,11 @@
     completionHandler({ result });
 }
 
+void WebPage::setRemoteObjectRegistry(RemoteObjectRegistry& registry)
+{
+    m_remoteObjectRegistry = makeWeakPtr(registry);
+}
+
 } // namespace WebKit
 
 #endif // PLATFORM(COCOA)
diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.cpp b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
@@ -254,6 +254,8 @@
 #include "PDFPlugin.h"
 #include "PlaybackSessionManager.h"
 #include "RemoteLayerTreeTransaction.h"
+#include "RemoteObjectRegistry.h"
+#include "RemoteObjectRegistryMessages.h"
 #include "TextCheckingControllerProxy.h"
 #include "TouchBarMenuData.h"
 #include "TouchBarMenuItemData.h"
@@ -1358,6 +1360,10 @@ void WebPage::close()
     m_isRunningModal = false;
 
     auto& webProcess = WebProcess::singleton();
+#if PLATFORM(COCOA)
+    if (m_remoteObjectRegistry)
+        m_remoteObjectRegistry->close();
+#endif
 #if ENABLE(ASYNC_SCROLLING)
     if (m_useAsyncScrolling)
         webProcess.eventDispatcher().removeScrollingTreeForPage(this);

diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.h b/Source/WebKit/WebProcess/WebPage/WebPage.h
@@ -207,6 +207,7 @@ class NotificationPermissionRequestManager;
 class PDFPlugin;
 class PageBanner;
 class PluginView;
+class RemoteObjectRegistry;
 class RemoteWebInspectorUI;
 class TextCheckingControllerProxy;
 class UserMediaPermissionRequestManager;
@@ -1181,6 +1182,8 @@ class WebPage : public API::ObjectImpl<API::Object::Type::BundlePage>, public IP
     TextCheckingControllerProxy& textCheckingController() { return m_textCheckingControllerProxy.get(); }
 #endif
 
+    void setRemoteObjectRegistry(RemoteObjectRegistry&);
+
 private:
     WebPage(uint64_t pageID, WebPageCreationParameters&&);
 
@@ -1888,6 +1891,9 @@ class WebPage : public API::ObjectImpl<API::Object::Type::BundlePage>, public IP
     OptionSet<LayerTreeFreezeReason> m_LayerTreeFreezeReasons;
     bool m_isSuspended { false };
     bool m_needsFontAttributes { false };
+#if PLATFORM(COCOA)
+    WeakPtr<RemoteObjectRegistry> m_remoteObjectRegistry;
+#endif
 };
 
 } // namespace WebKit

diff --git a/Tools/ChangeLog b/Tools/ChangeLog
@@ -1,3 +1,14 @@
+2019-04-10  Alex Christensen  <achristensen@webkit.org>
+
+        RemoteObjectRegistry message receiver should be removed when WebPage::close is called instead of waiting until dealloc
+        https://bugs.webkit.org/show_bug.cgi?id=196744
+        <rdar://49415309>
+
+        Reviewed by Chris Dumez.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/BundleRetainPagePlugIn.mm:
+        (-[BundleRetainPagePlugIn webProcessPlugIn:didCreateBrowserContextController:]):
+
 2019-04-10  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         Add a way to opt into modern compatibility mode in layout tests

diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/BundleRetainPagePlugIn.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/BundleRetainPagePlugIn.mm
@@ -26,6 +26,7 @@
 #import "config.h"
 
 #import <WebKit/WKWebProcessPlugIn.h>
+#import <WebKit/WKWebProcessPlugInBrowserContextControllerPrivate.h>
 #import <wtf/RetainPtr.h>
 
 @interface BundleRetainPagePlugIn : NSObject <WKWebProcessPlugIn>
@@ -36,6 +37,7 @@ @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)] { });
+    [browserContextController _remoteObjectRegistry];
 }
 
 @end