RemoteObjectRegistry message receiver should be removed when WebPage::close is called...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Apr 2019 18:35:32 +0000 (18:35 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Apr 2019 18:35:32 +0000 (18:35 +0000)
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:]):

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

Source/WebKit/ChangeLog
Source/WebKit/Shared/API/Cocoa/RemoteObjectRegistry.h
Source/WebKit/Shared/API/Cocoa/RemoteObjectRegistry.mm
Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm
Source/WebKit/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm
Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm
Source/WebKit/WebProcess/WebPage/WebPage.cpp
Source/WebKit/WebProcess/WebPage/WebPage.h
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/BundleRetainPagePlugIn.mm

index df40d4e..f47b87d 100644 (file)
@@ -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.
index 037caac..86f8041 100644 (file)
  * 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 @@ public:
     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 @@ private:
     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
index 4f78861..1cb9e6c 100644 (file)
@@ -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 @@ RemoteObjectRegistry::RemoteObjectRegistry(_WKRemoteObjectRegistry *remoteObject
     : 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(_WKRemoteObjectRegistry *remoteObject
 {
 }
 
-
 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)
index 2ef2ba5..f35d930 100644 (file)
@@ -36,6 +36,7 @@
 #import "WKRemoteObject.h"
 #import "WKRemoteObjectCoder.h"
 #import "WKSharedAPICast.h"
+#import "WebPage.h"
 #import "_WKRemoteObjectInterface.h"
 #import <objc/runtime.h>
 
index 5a99deb..80caa2d 100644 (file)
@@ -350,10 +350,8 @@ static void setUpResourceLoadClient(WKWebProcessPlugInBrowserContextController *
 
 - (void)dealloc
 {
-    if (_remoteObjectRegistry) {
-        WebKit::WebProcess::singleton().removeMessageReceiver(Messages::RemoteObjectRegistry::messageReceiverName(), _page->pageID());
+    if (_remoteObjectRegistry)
         [_remoteObjectRegistry _invalidate];
-    }
 
     _page->~WebPage();
 
@@ -416,10 +414,8 @@ static void setUpResourceLoadClient(WKWebProcessPlugInBrowserContextController *
 
 - (_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();
 }
index 16f3fa8..ac04246 100644 (file)
 #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 @@ void WebPage::getContentsAsAttributedString(CompletionHandler<void(const Attribu
     completionHandler({ result });
 }
 
+void WebPage::setRemoteObjectRegistry(RemoteObjectRegistry& registry)
+{
+    m_remoteObjectRegistry = makeWeakPtr(registry);
+}
+    
 } // namespace WebKit
 
 #endif // PLATFORM(COCOA)
index abe627b..4f8e13e 100644 (file)
 #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);
index df4c75d..645badb 100644 (file)
@@ -207,6 +207,7 @@ class NotificationPermissionRequestManager;
 class PDFPlugin;
 class PageBanner;
 class PluginView;
+class RemoteObjectRegistry;
 class RemoteWebInspectorUI;
 class TextCheckingControllerProxy;
 class UserMediaPermissionRequestManager;
@@ -1181,6 +1182,8 @@ public:
     TextCheckingControllerProxy& textCheckingController() { return m_textCheckingControllerProxy.get(); }
 #endif
 
+    void setRemoteObjectRegistry(RemoteObjectRegistry&);
+
 private:
     WebPage(uint64_t pageID, WebPageCreationParameters&&);
 
@@ -1888,6 +1891,9 @@ private:
     OptionSet<LayerTreeFreezeReason> m_LayerTreeFreezeReasons;
     bool m_isSuspended { false };
     bool m_needsFontAttributes { false };
+#if PLATFORM(COCOA)
+    WeakPtr<RemoteObjectRegistry> m_remoteObjectRegistry;
+#endif
 };
 
 } // namespace WebKit
index f4614d7..ddab7a4 100644 (file)
@@ -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
index e23c58d..5ef3c83 100644 (file)
@@ -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 @@
 - (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