Changeset 249350 in webkit

Timestamp:

Aug 30, 2019 3:59:10 PM (5 years ago)

Author:

Chris Dumez

Message:

[PSON] We no longer need to throw away suspended pages in a process before using it for a navigation
​https://bugs.webkit.org/show_bug.cgi?id=201344

Reviewed by Antti Koivisto.

We no longer need to throw away suspended pages in a process before using it for a navigation, now that
Bug 201225 has been fixed. WebPage objects (suspended or live) in the process now have distinct
identifiers and can coexist.

• Shared/API/Cocoa/RemoteObjectRegistry.h:
• Shared/API/Cocoa/RemoteObjectRegistry.mm:

(WebKit::RemoteObjectRegistry::RemoteObjectRegistry):

• UIProcess/Cocoa/UIRemoteObjectRegistry.cpp:

(WebKit::UIRemoteObjectRegistry::UIRemoteObjectRegistry):

• WebProcess/WebPage/Cocoa/WebRemoteObjectRegistry.cpp:

(WebKit::WebRemoteObjectRegistry::WebRemoteObjectRegistry):
(WebKit::WebRemoteObjectRegistry::close):

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• Shared/API/Cocoa/RemoteObjectRegistry.h (modified) (2 diffs)
• Shared/API/Cocoa/RemoteObjectRegistry.mm (modified) (2 diffs)
• UIProcess/Cocoa/UIRemoteObjectRegistry.cpp (modified) (2 diffs)
• UIProcess/Cocoa/UIRemoteObjectRegistry.h (modified) (1 diff)
• UIProcess/WebPageProxy.cpp (modified) (1 diff)
• UIProcess/WebProcessPool.cpp (modified) (3 diffs)
• WebProcess/WebPage/Cocoa/WebRemoteObjectRegistry.cpp (modified) (3 diffs)
• WebProcess/WebPage/Cocoa/WebRemoteObjectRegistry.h (modified) (1 diff)
• WebProcess/WebStorage/StorageNamespaceImpl.cpp (modified) (2 diffs)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-08-30  Chris Dumez  <cdumez@apple.com>
+
+        [PSON] We no longer need to throw away suspended pages in a process before using it for a navigation
+        https://bugs.webkit.org/show_bug.cgi?id=201344
+
+        Reviewed by Antti Koivisto.
+
+        We no longer need to throw away suspended pages in a process before using it for a navigation, now that
+        Bug 201225 has been fixed. WebPage objects (suspended or live) in the process now have distinct
+        identifiers and can coexist.
+
+        * Shared/API/Cocoa/RemoteObjectRegistry.h:
+        * Shared/API/Cocoa/RemoteObjectRegistry.mm:
+        (WebKit::RemoteObjectRegistry::RemoteObjectRegistry):
+        * UIProcess/Cocoa/UIRemoteObjectRegistry.cpp:
+        (WebKit::UIRemoteObjectRegistry::UIRemoteObjectRegistry):
+        * WebProcess/WebPage/Cocoa/WebRemoteObjectRegistry.cpp:
+        (WebKit::WebRemoteObjectRegistry::WebRemoteObjectRegistry):
+        (WebKit::WebRemoteObjectRegistry::close):
+
 2019-08-30  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebKit/Shared/API/Cocoa/RemoteObjectRegistry.h

@@ -54 +54 @@
 
 protected:
-    RemoteObjectRegistry(_WKRemoteObjectRegistry *, WebPageProxyIdentifier messageDestinationID);
+    explicit RemoteObjectRegistry(_WKRemoteObjectRegistry *);
     
 private:
     virtual ProcessThrottler::BackgroundActivityToken takeBackgroundActivityToken() { return nullptr; }
     virtual IPC::MessageSender& messageSender() = 0;
+    virtual uint64_t messageDestinationID() = 0;
 
     // IPC::MessageReceiver
@@ -70 +71 @@
     WeakObjCPtr<_WKRemoteObjectRegistry> m_remoteObjectRegistry;
     HashMap<uint64_t, ProcessThrottler::BackgroundActivityToken> m_pendingReplies;
-    WebPageProxyIdentifier m_messageDestinationID;
 };
 

trunk/Source/WebKit/Shared/API/Cocoa/RemoteObjectRegistry.mm

@@ -35 +35 @@
 namespace WebKit {
 
-RemoteObjectRegistry::RemoteObjectRegistry(_WKRemoteObjectRegistry *remoteObjectRegistry, WebPageProxyIdentifier messageDestinationID)
+RemoteObjectRegistry::RemoteObjectRegistry(_WKRemoteObjectRegistry *remoteObjectRegistry)
     : m_remoteObjectRegistry(remoteObjectRegistry)
-    , m_messageDestinationID(messageDestinationID)
 {
 }
@@ -53 +52 @@
     }
 
-    messageSender().send(Messages::RemoteObjectRegistry::InvokeMethod(invocation), m_messageDestinationID);
+    messageSender().send(Messages::RemoteObjectRegistry::InvokeMethod(invocation), messageDestinationID());
 }
 
 void RemoteObjectRegistry::sendReplyBlock(uint64_t replyID, const UserData& blockInvocation)
 {
-    messageSender().send(Messages::RemoteObjectRegistry::CallReplyBlock(replyID, blockInvocation), m_messageDestinationID);
+    messageSender().send(Messages::RemoteObjectRegistry::CallReplyBlock(replyID, blockInvocation), messageDestinationID());
 }
 
 void RemoteObjectRegistry::sendUnusedReply(uint64_t replyID)
 {
-    messageSender().send(Messages::RemoteObjectRegistry::ReleaseUnusedReplyBlock(replyID), m_messageDestinationID);
+    messageSender().send(Messages::RemoteObjectRegistry::ReleaseUnusedReplyBlock(replyID), messageDestinationID());
 }
 

trunk/Source/WebKit/UIProcess/Cocoa/UIRemoteObjectRegistry.cpp

@@ -38 +38 @@
 
 UIRemoteObjectRegistry::UIRemoteObjectRegistry(_WKRemoteObjectRegistry *remoteObjectRegistry, WebPageProxy& page)
-    : RemoteObjectRegistry(remoteObjectRegistry, page.identifier())
+    : RemoteObjectRegistry(remoteObjectRegistry)
     , m_page(page)
 {
@@ -56 +56 @@
 }
 
+uint64_t UIRemoteObjectRegistry::messageDestinationID()
+{
+    return m_page.webPageID().toUInt64();
+}
+
 } // namespace WebKit

trunk/Source/WebKit/UIProcess/Cocoa/UIRemoteObjectRegistry.h

@@ -40 +40 @@
 private:
     IPC::MessageSender& messageSender() final;
+    uint64_t messageDestinationID() final;
     ProcessThrottler::BackgroundActivityToken takeBackgroundActivityToken() final;
 

trunk/Source/WebKit/UIProcess/WebPageProxy.cpp

@@ -2892 +2892 @@
 
         if (shouldProcessSwap) {
-            // FIXME: Architecturally we do not currently support multiple WebPage's with the same ID in a given WebProcess.
-            // In the case where the destination WebProcess has a SuspendedPageProxy for this WebPage, we should have thrown
-            // it away to support WebProcess re-use.
-            ASSERT(destinationSuspendedPage || !process().processPool().hasSuspendedPageFor(processForNavigation, *this));
-
             auto suspendedPage = destinationSuspendedPage ? process().processPool().takeSuspendedPage(*destinationSuspendedPage) : nullptr;
             if (suspendedPage && suspendedPage->pageIsClosedOrClosing())

trunk/Source/WebKit/UIProcess/WebProcessPool.cpp

@@ -2332 +2332 @@
         if (RefPtr<WebProcessProxy> process = WebProcessProxy::processForIdentifier(targetItem->lastProcessIdentifier())) {
             if (process->state() != WebProcessProxy::State::Terminated) {
-                // FIXME: Architecturally we do not currently support multiple WebPage's with the same ID in a given WebProcess.
-                // In the case where this WebProcess has a SuspendedPageProxy for this WebPage, we can throw it away to support
-                // WebProcess re-use.
-                removeAllSuspendedPagesForPage(page, process.get());
-
                 // Make sure we remove the process from the cache if it is in there since we're about to use it.
                 if (process->isInProcessCache()) {
@@ -2374 +2369 @@
                 LOG(ProcessSwapping, "(ProcessSwapping) Reusing a previously cached process with pid %i to continue navigation to URL %s", process->processIdentifier(), targetURL.string().utf8().data());
 
-                // FIXME: Architecturally we do not currently support multiple WebPage's with the same ID in a given WebProcess.
-                // In the case where this WebProcess has a SuspendedPageProxy for this WebPage, we can throw it away to support
-                // WebProcess re-use.
-                // In the future it would be great to refactor-out this limitation (https://bugs.webkit.org/show_bug.cgi?id=191166).
-                removeAllSuspendedPagesForPage(page, process);
-
                 return completionHandler(makeRef(*process), nullptr, reason);
             }
@@ -2393 +2382 @@
         return suspendedPage->process().registrableDomain() == registrableDomain && &suspendedPage->process().websiteDataStore() == &dataStore;
     });
-    if (it == m_suspendedPages.end())
-        return nullptr;
-
-    Ref<WebProcessProxy> process = (*it)->process();
-
-    // FIXME: If the SuspendedPage is for this page, then we need to destroy the suspended page as we do not support having
-    // multiple WebPages with the same ID in a given WebProcess currently (https://bugs.webkit.org/show_bug.cgi?id=191166).
-    if (&(*it)->page() == &page)
-        m_suspendedPages.remove(it);
-
-
-    return process;
+    return it == m_suspendedPages.end() ? nullptr : &(*it)->process();
 }
 

trunk/Source/WebKit/WebProcess/WebPage/Cocoa/WebRemoteObjectRegistry.cpp

@@ -33 +33 @@
 
 WebRemoteObjectRegistry::WebRemoteObjectRegistry(_WKRemoteObjectRegistry *remoteObjectRegistry, WebPage& page)
-    : RemoteObjectRegistry(remoteObjectRegistry, page.webPageProxyIdentifier())
+    : RemoteObjectRegistry(remoteObjectRegistry)
     , m_page(page)
 {
-    WebProcess::singleton().addMessageReceiver(Messages::RemoteObjectRegistry::messageReceiverName(), m_page.webPageProxyIdentifier(), *this);
+    WebProcess::singleton().addMessageReceiver(Messages::RemoteObjectRegistry::messageReceiverName(), m_page.pageID(), *this);
     page.setRemoteObjectRegistry(this);
 }
@@ -48 +48 @@
 {
     if (m_page.remoteObjectRegistry() == this) {
-        WebProcess::singleton().removeMessageReceiver(Messages::RemoteObjectRegistry::messageReceiverName(), m_page.webPageProxyIdentifier());
+        WebProcess::singleton().removeMessageReceiver(Messages::RemoteObjectRegistry::messageReceiverName(), m_page.pageID());
         m_page.setRemoteObjectRegistry(nullptr);
     }
@@ -58 +58 @@
 }
 
+uint64_t WebRemoteObjectRegistry::messageDestinationID()
+{
+    return m_page.webPageProxyIdentifier().toUInt64();
+}
+
 } // namespace WebKit

trunk/Source/WebKit/WebProcess/WebPage/Cocoa/WebRemoteObjectRegistry.h

@@ -41 +41 @@
 private:
     IPC::MessageSender& messageSender() final;
+    uint64_t messageDestinationID() final;
 
     WebPage& m_page;

trunk/Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.cpp

@@ -43 +43 @@
 using namespace WebCore;
 
+static HashMap<StorageNamespaceImpl::Identifier, StorageNamespaceImpl*>& sessionStorageNamespaces()
+{
+    static NeverDestroyed<HashMap<StorageNamespaceImpl::Identifier, StorageNamespaceImpl*>> map;
+    return map;
+}
+
 Ref<StorageNamespaceImpl> StorageNamespaceImpl::createSessionStorageNamespace(Identifier identifier, PageIdentifier pageID, unsigned quotaInBytes, PAL::SessionID sessionID)
 {
+    // The identifier of a session storage namespace is the WebPageProxyIdentifier. It is possible we have several WebPage objects in a single process for the same
+    // WebPageProxyIdentifier and these need to share the same namespace instance so we know where to route the IPC to.
+    if (auto* existingNamespace = sessionStorageNamespaces().get(identifier))
+        return *existingNamespace;
     return adoptRef(*new StorageNamespaceImpl(StorageType::Session, identifier, pageID, nullptr, quotaInBytes, sessionID));
 }
@@ -67 +77 @@
 {
     ASSERT(storageType == StorageType::Session || !m_sessionPageID);
+    
+    if (m_storageType == StorageType::Session) {
+        ASSERT(!sessionStorageNamespaces().contains(m_storageNamespaceID));
+        sessionStorageNamespaces().add(m_storageNamespaceID, this);
+    }
 }
 
 StorageNamespaceImpl::~StorageNamespaceImpl()
 {
+    if (m_storageType == StorageType::Session) {
+        bool wasRemoved = sessionStorageNamespaces().remove(m_storageNamespaceID);
+        ASSERT_UNUSED(wasRemoved, wasRemoved);
+    }
 }