Flaky API Test: TestWebKitAPI.ProcessSwap.SessionStorage
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 28 Feb 2019 00:54:19 +0000 (00:54 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 28 Feb 2019 00:54:19 +0000 (00:54 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194480

Reviewed by Brady Eidson.

Source/WebKit:

The StorageManager would only start listening for IPC on a given connection when
StorageManager::processWillOpenConnection() is called. This gets called from
WebsiteDataStore::webProcessWillOpenConnection() which gets called from
WebProcessLifetimeTracker::webPageEnteringWebProcess().

webPageEnteringWebProcess() was called in WebPageProxy::finishAttachingToWebProcess()
after process-swapping. This means that IPC comming from the *provisional* process
would not get processed and we would only start processing IPC on the provisional
process's connection when it would get committed.

To address the issue, this patch teaches WebProcessLifetimeTracker that a page can
now have several processes. We also make sure that webPageEnteringWebProcess() gets
called for the provisional process as soon as we create the provisional page, instead
of waiting for it to get committed.

* UIProcess/ProvisionalPageProxy.cpp:
(WebKit::ProvisionalPageProxy::ProvisionalPageProxy):
(WebKit::ProvisionalPageProxy::~ProvisionalPageProxy):
(WebKit::ProvisionalPageProxy::connectionWillOpen):
* UIProcess/ProvisionalPageProxy.h:
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::finishAttachingToWebProcess):
(WebKit::WebPageProxy::connectionWillOpen):
(WebKit::WebPageProxy::webProcessWillShutDown):
(WebKit::WebPageProxy::processDidTerminate):
* UIProcess/WebPageProxy.h:
(WebKit::WebPageProxy::webProcessLifetimeTracker):
* UIProcess/WebProcessLifetimeObserver.cpp:
(WebKit::WebProcessLifetimeObserver::addWebPage):
(WebKit::WebProcessLifetimeObserver::removeWebPage):
* UIProcess/WebProcessLifetimeObserver.h:
* UIProcess/WebProcessLifetimeTracker.cpp:
(WebKit::WebProcessLifetimeTracker::addObserver):
(WebKit::WebProcessLifetimeTracker::webPageEnteringWebProcess):
(WebKit::WebProcessLifetimeTracker::webPageLeavingWebProcess):
(WebKit::WebProcessLifetimeTracker::pageWasInvalidated):
(WebKit::WebProcessLifetimeTracker::processIsRunning):
* UIProcess/WebProcessLifetimeTracker.h:
* UIProcess/WebProcessProxy.cpp:
(WebKit::WebProcessProxy::connectionWillOpen):
* UIProcess/WebStorage/StorageManager.cpp:
(WebKit::StorageManager::SessionStorageNamespace::allowedConnections const):
(WebKit::StorageManager::SessionStorageNamespace::addAllowedConnection):
(WebKit::StorageManager::SessionStorageNamespace::removeAllowedConnection):
(WebKit::StorageManager::addAllowedSessionStorageNamespaceConnection):
(WebKit::StorageManager::removeAllowedSessionStorageNamespaceConnection):
(WebKit::StorageManager::processWillOpenConnection):
(WebKit::StorageManager::processDidCloseConnection):
(WebKit::StorageManager::createSessionStorageMap):
(WebKit::StorageManager::SessionStorageNamespace::allowedConnection const): Deleted.
(WebKit::StorageManager::SessionStorageNamespace::setAllowedConnection): Deleted.
(WebKit::StorageManager::setAllowedSessionStorageNamespaceConnection): Deleted.
* UIProcess/WebStorage/StorageManager.h:
* UIProcess/WebsiteData/WebsiteDataStore.cpp:
(WebKit::WebsiteDataStore::webPageWillOpenConnection):
(WebKit::WebsiteDataStore::webPageDidCloseConnection):

Tools:

Update existing API test to make it more likely to reproduce the issue.

* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

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

15 files changed:
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/ProvisionalPageProxy.cpp
Source/WebKit/UIProcess/ProvisionalPageProxy.h
Source/WebKit/UIProcess/WebPageProxy.cpp
Source/WebKit/UIProcess/WebPageProxy.h
Source/WebKit/UIProcess/WebProcessLifetimeObserver.cpp
Source/WebKit/UIProcess/WebProcessLifetimeObserver.h
Source/WebKit/UIProcess/WebProcessLifetimeTracker.cpp
Source/WebKit/UIProcess/WebProcessLifetimeTracker.h
Source/WebKit/UIProcess/WebProcessProxy.cpp
Source/WebKit/UIProcess/WebStorage/StorageManager.cpp
Source/WebKit/UIProcess/WebStorage/StorageManager.h
Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

index 6e55199..7f96bf5 100644 (file)
@@ -1,3 +1,67 @@
+2019-02-27  Chris Dumez  <cdumez@apple.com>
+
+        Flaky API Test: TestWebKitAPI.ProcessSwap.SessionStorage
+        https://bugs.webkit.org/show_bug.cgi?id=194480
+
+        Reviewed by Brady Eidson.
+
+        The StorageManager would only start listening for IPC on a given connection when
+        StorageManager::processWillOpenConnection() is called. This gets called from
+        WebsiteDataStore::webProcessWillOpenConnection() which gets called from
+        WebProcessLifetimeTracker::webPageEnteringWebProcess().
+
+        webPageEnteringWebProcess() was called in WebPageProxy::finishAttachingToWebProcess()
+        after process-swapping. This means that IPC comming from the *provisional* process
+        would not get processed and we would only start processing IPC on the provisional
+        process's connection when it would get committed.
+
+        To address the issue, this patch teaches WebProcessLifetimeTracker that a page can
+        now have several processes. We also make sure that webPageEnteringWebProcess() gets
+        called for the provisional process as soon as we create the provisional page, instead
+        of waiting for it to get committed.
+
+        * UIProcess/ProvisionalPageProxy.cpp:
+        (WebKit::ProvisionalPageProxy::ProvisionalPageProxy):
+        (WebKit::ProvisionalPageProxy::~ProvisionalPageProxy):
+        (WebKit::ProvisionalPageProxy::connectionWillOpen):
+        * UIProcess/ProvisionalPageProxy.h:
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::finishAttachingToWebProcess):
+        (WebKit::WebPageProxy::connectionWillOpen):
+        (WebKit::WebPageProxy::webProcessWillShutDown):
+        (WebKit::WebPageProxy::processDidTerminate):
+        * UIProcess/WebPageProxy.h:
+        (WebKit::WebPageProxy::webProcessLifetimeTracker):
+        * UIProcess/WebProcessLifetimeObserver.cpp:
+        (WebKit::WebProcessLifetimeObserver::addWebPage):
+        (WebKit::WebProcessLifetimeObserver::removeWebPage):
+        * UIProcess/WebProcessLifetimeObserver.h:
+        * UIProcess/WebProcessLifetimeTracker.cpp:
+        (WebKit::WebProcessLifetimeTracker::addObserver):
+        (WebKit::WebProcessLifetimeTracker::webPageEnteringWebProcess):
+        (WebKit::WebProcessLifetimeTracker::webPageLeavingWebProcess):
+        (WebKit::WebProcessLifetimeTracker::pageWasInvalidated):
+        (WebKit::WebProcessLifetimeTracker::processIsRunning):
+        * UIProcess/WebProcessLifetimeTracker.h:
+        * UIProcess/WebProcessProxy.cpp:
+        (WebKit::WebProcessProxy::connectionWillOpen):
+        * UIProcess/WebStorage/StorageManager.cpp:
+        (WebKit::StorageManager::SessionStorageNamespace::allowedConnections const):
+        (WebKit::StorageManager::SessionStorageNamespace::addAllowedConnection):
+        (WebKit::StorageManager::SessionStorageNamespace::removeAllowedConnection):
+        (WebKit::StorageManager::addAllowedSessionStorageNamespaceConnection):
+        (WebKit::StorageManager::removeAllowedSessionStorageNamespaceConnection):
+        (WebKit::StorageManager::processWillOpenConnection):
+        (WebKit::StorageManager::processDidCloseConnection):
+        (WebKit::StorageManager::createSessionStorageMap):
+        (WebKit::StorageManager::SessionStorageNamespace::allowedConnection const): Deleted.
+        (WebKit::StorageManager::SessionStorageNamespace::setAllowedConnection): Deleted.
+        (WebKit::StorageManager::setAllowedSessionStorageNamespaceConnection): Deleted.
+        * UIProcess/WebStorage/StorageManager.h:
+        * UIProcess/WebsiteData/WebsiteDataStore.cpp:
+        (WebKit::WebsiteDataStore::webPageWillOpenConnection):
+        (WebKit::WebsiteDataStore::webPageDidCloseConnection):
+
 2019-02-27  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         [iOS] Web pages shouldn't be able to present a keyboard after the web view resigns first responder
index 9b1b046..1299ef8 100644 (file)
@@ -63,6 +63,9 @@ ProvisionalPageProxy::ProvisionalPageProxy(WebPageProxy& page, Ref<WebProcessPro
     m_process->addMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID(), *this);
     m_process->addProvisionalPageProxy(*this);
 
+    if (m_process->state() == AuxiliaryProcessProxy::State::Running)
+        m_page.webProcessLifetimeTracker().webPageEnteringWebProcess(m_process);
+
     // If we are reattaching to a SuspendedPage, then the WebProcess' WebPage already exists and
     // WebPageProxy::didCreateMainFrame() will not be called to initialize m_mainFrame. In such
     // case, we need to initialize m_mainFrame to reflect the fact the the WebProcess' WebPage
@@ -84,6 +87,9 @@ ProvisionalPageProxy::~ProvisionalPageProxy()
     if (m_wasCommitted)
         return;
 
+    if (m_process->state() == AuxiliaryProcessProxy::State::Running)
+        m_page.webProcessLifetimeTracker().webPageLeavingWebProcess(m_process);
+
     m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID());
     m_process->send(Messages::WebPage::Close(), m_page.pageID());
 
@@ -92,6 +98,13 @@ ProvisionalPageProxy::~ProvisionalPageProxy()
     });
 }
 
+void ProvisionalPageProxy::connectionWillOpen(IPC::Connection& connection)
+{
+    ASSERT_UNUSED(connection, &connection == m_process->connection());
+
+    m_page.webProcessLifetimeTracker().webPageEnteringWebProcess(m_process);
+}
+
 void ProvisionalPageProxy::processDidTerminate()
 {
     RELEASE_LOG_ERROR_IF_ALLOWED(ProcessSwapping, "processDidTerminate: pageID = %" PRIu64, m_page.pageID());
index 5db9886..8883f73 100644 (file)
@@ -80,6 +80,7 @@ public:
 
     void processDidFinishLaunching();
     void processDidTerminate();
+    void connectionWillOpen(IPC::Connection&);
 
 private:
     // IPC::MessageReceiver
index 8511061..870c17e 100644 (file)
@@ -754,7 +754,7 @@ void WebPageProxy::reattachToWebProcess()
     m_process->addExistingWebPage(*this, m_pageID, WebProcessProxy::BeginsUsingDataStore::Yes);
     m_process->addMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_pageID, *this);
 
-    finishAttachingToWebProcess();
+    finishAttachingToWebProcess(IsProcessSwap::No);
 }
 
 bool WebPageProxy::suspendCurrentPageIfPossible(API::Navigation& navigation, Optional<uint64_t> mainFrameID, ProcessSwapRequestedByClient processSwapRequestedByClient)
@@ -816,16 +816,18 @@ void WebPageProxy::swapToWebProcess(Ref<WebProcessProxy>&& process, std::unique_
     m_process->addExistingWebPage(*this, m_pageID, WebProcessProxy::BeginsUsingDataStore::No);
     m_process->addMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_pageID, *this);
 
-    // No need to initialize the WebPage since it was created when we created the ProvisionalPageProxy.
-    finishAttachingToWebProcess(ShouldInitializeWebPage::No);
+    finishAttachingToWebProcess(IsProcessSwap::Yes);
 }
 
-void WebPageProxy::finishAttachingToWebProcess(ShouldInitializeWebPage shouldInitializePage)
+void WebPageProxy::finishAttachingToWebProcess(IsProcessSwap isProcessSwap)
 {
     ASSERT(m_process->state() != AuxiliaryProcessProxy::State::Terminated);
 
     if (m_process->state() == AuxiliaryProcessProxy::State::Running) {
-        m_webProcessLifetimeTracker.webPageEnteringWebProcess();
+        // In the process-swap case, the ProvisionalPageProxy constructor already took care of calling webPageEnteringWebProcess()
+        // when the process was provisional.
+        if (isProcessSwap != IsProcessSwap::Yes)
+            m_webProcessLifetimeTracker.webPageEnteringWebProcess(m_process);
         processDidFinishLaunching();
     }
 
@@ -856,7 +858,8 @@ void WebPageProxy::finishAttachingToWebProcess(ShouldInitializeWebPage shouldIni
     m_editableImageController = std::make_unique<EditableImageController>(*this);
 #endif
 
-    if (shouldInitializePage == ShouldInitializeWebPage::Yes)
+    // In the process-swap case, the ProvisionalPageProxy already took care of initializing the WebPage in the WebProcess.
+    if (isProcessSwap != IsProcessSwap::Yes)
         initializeWebPage();
 
     m_inspector->updateForNewPageProcess(this);
@@ -4996,12 +4999,12 @@ void WebPageProxy::connectionWillOpen(IPC::Connection& connection)
 {
     ASSERT_UNUSED(connection, &connection == m_process->connection());
 
-    m_webProcessLifetimeTracker.webPageEnteringWebProcess();
+    m_webProcessLifetimeTracker.webPageEnteringWebProcess(m_process);
 }
 
 void WebPageProxy::webProcessWillShutDown()
 {
-    m_webProcessLifetimeTracker.webPageLeavingWebProcess();
+    m_webProcessLifetimeTracker.webPageLeavingWebProcess(m_process);
 }
 
 void WebPageProxy::processDidFinishLaunching()
@@ -6560,7 +6563,7 @@ void WebPageProxy::processDidTerminate(ProcessTerminationReason reason)
     // If it does *during* process swapping, and the client triggers a reload, that causes bizarre WebKit re-entry.
     // FIXME: This might have to change
     if (reason == ProcessTerminationReason::NavigationSwap)
-        m_webProcessLifetimeTracker.webPageLeavingWebProcess();
+        m_webProcessLifetimeTracker.webPageLeavingWebProcess(m_process);
     else {
         navigationState().clearAllNavigations();
         dispatchProcessDidTerminate(reason);
index 68c3587..8be426c 100644 (file)
@@ -377,6 +377,8 @@ public:
 
     void addPreviouslyVisitedPath(const String&);
 
+    WebProcessLifetimeTracker& webProcessLifetimeTracker() { return m_webProcessLifetimeTracker; }
+
 #if ENABLE(DATA_DETECTION)
     NSArray *dataDetectionResults() { return m_dataDetectionResults.get(); }
     void detectDataInAllFrames(WebCore::DataDetectorTypes, CompletionHandler<void(const DataDetectionResult&)>&&);
@@ -1634,8 +1636,8 @@ private:
     void didFailToSuspendAfterProcessSwap();
     void didSuspendAfterProcessSwap();
 
-    enum class ShouldInitializeWebPage { No, Yes };
-    void finishAttachingToWebProcess(ShouldInitializeWebPage = ShouldInitializeWebPage::Yes);
+    enum class IsProcessSwap { No, Yes };
+    void finishAttachingToWebProcess(IsProcessSwap);
 
     RefPtr<API::Navigation> reattachToWebProcessForReload();
     RefPtr<API::Navigation> reattachToWebProcessWithItem(WebBackForwardListItem&);
index c36474f..3d75abd 100644 (file)
@@ -39,10 +39,8 @@ WebProcessLifetimeObserver::~WebProcessLifetimeObserver()
 {
 }
 
-void WebProcessLifetimeObserver::addWebPage(WebPageProxy& webPageProxy)
+void WebProcessLifetimeObserver::addWebPage(WebPageProxy& webPageProxy, WebProcessProxy& process)
 {
-    auto& process = webPageProxy.process();
-
     ASSERT(process.state() == WebProcessProxy::State::Running);
 
     if (m_processes.add(&process).isNewEntry)
@@ -51,10 +49,8 @@ void WebProcessLifetimeObserver::addWebPage(WebPageProxy& webPageProxy)
     webPageWillOpenConnection(webPageProxy, *process.connection());
 }
 
-void WebProcessLifetimeObserver::removeWebPage(WebPageProxy& webPageProxy)
+void WebProcessLifetimeObserver::removeWebPage(WebPageProxy& webPageProxy, WebProcessProxy& process)
 {
-    auto& process = webPageProxy.process();
-
     // FIXME: This should assert that the page is either closed or that the process is no longer running,
     // but we have to make sure that removeWebPage is called after the connection has been removed in that case.
     ASSERT(m_processes.contains(&process));
index 48f7a0e..61ce22b 100644 (file)
@@ -43,8 +43,8 @@ public:
     WebProcessLifetimeObserver();
     virtual ~WebProcessLifetimeObserver();
 
-    void addWebPage(WebPageProxy&);
-    void removeWebPage(WebPageProxy&);
+    void addWebPage(WebPageProxy&, WebProcessProxy&);
+    void removeWebPage(WebPageProxy&, WebProcessProxy&);
 
     WTF::IteratorRange<HashCountedSet<WebProcessProxy*>::const_iterator::Keys> processes() const;
 
index d1d7016..71a0a5b 100644 (file)
@@ -49,41 +49,41 @@ void WebProcessLifetimeTracker::addObserver(WebProcessLifetimeObserver& observer
 
     observer.webPageWasAdded(m_webPageProxy);
 
-    if (processIsRunning())
-        observer.addWebPage(m_webPageProxy);
+    if (processIsRunning(m_webPageProxy.process()))
+        observer.addWebPage(m_webPageProxy, m_webPageProxy.process());
 }
 
-void WebProcessLifetimeTracker::webPageEnteringWebProcess()
+void WebProcessLifetimeTracker::webPageEnteringWebProcess(WebProcessProxy& process)
 {
-    ASSERT(processIsRunning());
+    ASSERT(processIsRunning(process));
 
     for (auto& observer : m_observers)
-        observer->addWebPage(m_webPageProxy);
+        observer->addWebPage(m_webPageProxy, process);
 }
 
-void WebProcessLifetimeTracker::webPageLeavingWebProcess()
+void WebProcessLifetimeTracker::webPageLeavingWebProcess(WebProcessProxy& process)
 {
-    ASSERT(processIsRunning());
+    ASSERT(processIsRunning(process));
 
     for (auto& observer : m_observers)
-        observer->removeWebPage(m_webPageProxy);
+        observer->removeWebPage(m_webPageProxy, process);
 }
 
 void WebProcessLifetimeTracker::pageWasInvalidated()
 {
-    if (!processIsRunning())
+    if (!processIsRunning(m_webPageProxy.process()))
         return;
 
     for (auto& observer : m_observers) {
-        observer->removeWebPage(m_webPageProxy);
+        observer->removeWebPage(m_webPageProxy, m_webPageProxy.process());
 
         observer->webPageWasInvalidated(m_webPageProxy);
     }
 }
 
-bool WebProcessLifetimeTracker::processIsRunning()
+bool WebProcessLifetimeTracker::processIsRunning(WebProcessProxy& process)
 {
-    return m_webPageProxy.process().state() == WebProcessProxy::State::Running;
+    return process.state() == WebProcessProxy::State::Running;
 }
 
 }
index fdc4def..12875ae 100644 (file)
@@ -36,6 +36,7 @@ namespace WebKit {
 
 class WebPageProxy;
 class WebProcessLifetimeObserver;
+class WebProcessProxy;
 
 class WebProcessLifetimeTracker {
 public:
@@ -44,13 +45,13 @@ public:
 
     void addObserver(WebProcessLifetimeObserver&);
 
-    void webPageEnteringWebProcess();
-    void webPageLeavingWebProcess();
+    void webPageEnteringWebProcess(WebProcessProxy&);
+    void webPageLeavingWebProcess(WebProcessProxy&);
 
     void pageWasInvalidated();
 
 private:
-    bool processIsRunning();
+    static bool processIsRunning(WebProcessProxy&);
 
     WebPageProxy& m_webPageProxy;
 
index d3b627b..e4329c8 100644 (file)
@@ -261,6 +261,9 @@ void WebProcessProxy::connectionWillOpen(IPC::Connection& connection)
 
     for (auto& page : m_pageMap.values())
         page->connectionWillOpen(connection);
+
+    for (auto* provisionalPage : m_provisionalPages)
+        provisionalPage->connectionWillOpen(connection);
 }
 
 void WebProcessProxy::processWillShutDown(IPC::Connection& connection)
index bed23b0..8cccdc1 100644 (file)
@@ -383,8 +383,9 @@ public:
 
     bool isEmpty() const { return m_storageAreaMap.isEmpty(); }
 
-    IPC::Connection::UniqueID allowedConnection() const { return m_allowedConnection; }
-    void setAllowedConnection(IPC::Connection::UniqueID);
+    Vector<IPC::Connection::UniqueID> allowedConnections() const { return m_allowedConnections; }
+    void addAllowedConnection(IPC::Connection::UniqueID);
+    void removeAllowedConnection(IPC::Connection::UniqueID);
 
     Ref<StorageArea> getOrCreateStorageArea(SecurityOriginData&&);
 
@@ -419,7 +420,7 @@ public:
 private:
     explicit SessionStorageNamespace(unsigned quotaInBytes);
 
-    IPC::Connection::UniqueID m_allowedConnection;
+    Vector<IPC::Connection::UniqueID> m_allowedConnections;
     unsigned m_quotaInBytes;
 
     HashMap<SecurityOriginData, RefPtr<StorageArea>> m_storageAreaMap;
@@ -439,11 +440,18 @@ StorageManager::SessionStorageNamespace::~SessionStorageNamespace()
 {
 }
 
-void StorageManager::SessionStorageNamespace::setAllowedConnection(IPC::Connection::UniqueID allowedConnection)
+void StorageManager::SessionStorageNamespace::addAllowedConnection(IPC::Connection::UniqueID allowedConnection)
 {
-    m_allowedConnection = allowedConnection;
+    ASSERT(!m_allowedConnections.contains(allowedConnection));
+    m_allowedConnections.append(allowedConnection);
 }
 
+
+void StorageManager::SessionStorageNamespace::removeAllowedConnection(IPC::Connection::UniqueID allowedConnection)
+{
+    ASSERT(m_allowedConnections.contains(allowedConnection));
+    m_allowedConnections.removeAll(allowedConnection);
+}
 auto StorageManager::SessionStorageNamespace::getOrCreateStorageArea(SecurityOriginData&& securityOrigin) -> Ref<StorageArea>
 {
     return *m_storageAreaMap.ensure(securityOrigin, [this, securityOrigin]() mutable {
@@ -494,13 +502,23 @@ void StorageManager::destroySessionStorageNamespace(uint64_t storageNamespaceID)
     });
 }
 
-void StorageManager::setAllowedSessionStorageNamespaceConnection(uint64_t storageNamespaceID, IPC::Connection* allowedConnection)
+void StorageManager::addAllowedSessionStorageNamespaceConnection(uint64_t storageNamespaceID, IPC::Connection& allowedConnection)
+{
+    auto allowedConnectionID = allowedConnection.uniqueID();
+    m_queue->dispatch([this, protectedThis = makeRef(*this), allowedConnectionID, storageNamespaceID]() mutable {
+        ASSERT(m_sessionStorageNamespaces.contains(storageNamespaceID));
+
+        m_sessionStorageNamespaces.get(storageNamespaceID)->addAllowedConnection(allowedConnectionID);
+    });
+}
+
+void StorageManager::removeAllowedSessionStorageNamespaceConnection(uint64_t storageNamespaceID, IPC::Connection& allowedConnection)
 {
-    auto allowedConnectionID = allowedConnection ? allowedConnection->uniqueID() : IPC::Connection::UniqueID { };
+    auto allowedConnectionID = allowedConnection.uniqueID();
     m_queue->dispatch([this, protectedThis = makeRef(*this), allowedConnectionID, storageNamespaceID]() mutable {
         ASSERT(m_sessionStorageNamespaces.contains(storageNamespaceID));
 
-        m_sessionStorageNamespaces.get(storageNamespaceID)->setAllowedConnection(allowedConnectionID);
+        m_sessionStorageNamespaces.get(storageNamespaceID)->removeAllowedConnection(allowedConnectionID);
     });
 }
 
@@ -522,12 +540,12 @@ void StorageManager::cloneSessionStorageNamespace(uint64_t storageNamespaceID, u
     });
 }
 
-void StorageManager::processWillOpenConnection(WebProcessProxy&, IPC::Connection& connection)
+void StorageManager::processWillOpenConnection(WebProcessProxy& process, IPC::Connection& connection)
 {
     connection.addWorkQueueMessageReceiver(Messages::StorageManager::messageReceiverName(), m_queue.get(), this);
 }
 
-void StorageManager::processDidCloseConnection(WebProcessProxy&, IPC::Connection& connection)
+void StorageManager::processDidCloseConnection(WebProcessProxy& process, IPC::Connection& connection)
 {
     connection.removeWorkQueueMessageReceiver(Messages::StorageManager::messageReceiverName());
 
@@ -764,7 +782,7 @@ void StorageManager::createSessionStorageMap(IPC::Connection& connection, uint64
     ASSERT(!slot);
 
     // FIXME: This should be a message check.
-    ASSERT(connection.uniqueID() == sessionStorageNamespace->allowedConnection());
+    ASSERT(sessionStorageNamespace->allowedConnections().contains(connection.uniqueID()));
 
     auto storageArea = sessionStorageNamespace->getOrCreateStorageArea(WTFMove(securityOriginData));
     storageArea->addListener(connection.uniqueID(), storageMapID);
index 3866ff9..d6f62a0 100644 (file)
@@ -51,7 +51,8 @@ public:
 
     void createSessionStorageNamespace(uint64_t storageNamespaceID, unsigned quotaInBytes);
     void destroySessionStorageNamespace(uint64_t storageNamespaceID);
-    void setAllowedSessionStorageNamespaceConnection(uint64_t storageNamespaceID, IPC::Connection* allowedConnection);
+    void addAllowedSessionStorageNamespaceConnection(uint64_t storageNamespaceID, IPC::Connection&);
+    void removeAllowedSessionStorageNamespaceConnection(uint64_t storageNamespaceID, IPC::Connection&);
     void cloneSessionStorageNamespace(uint64_t storageNamespaceID, uint64_t newStorageNamespaceID);
 
     void processWillOpenConnection(WebProcessProxy&, IPC::Connection&);
index f53308c..acea90c 100644 (file)
@@ -1803,13 +1803,13 @@ void WebsiteDataStore::webProcessWillOpenConnection(WebProcessProxy& webProcessP
 void WebsiteDataStore::webPageWillOpenConnection(WebPageProxy& webPageProxy, IPC::Connection& connection)
 {
     if (m_storageManager)
-        m_storageManager->setAllowedSessionStorageNamespaceConnection(webPageProxy.pageID(), &connection);
+        m_storageManager->addAllowedSessionStorageNamespaceConnection(webPageProxy.pageID(), connection);
 }
 
-void WebsiteDataStore::webPageDidCloseConnection(WebPageProxy& webPageProxy, IPC::Connection&)
+void WebsiteDataStore::webPageDidCloseConnection(WebPageProxy& webPageProxy, IPC::Connection& connection)
 {
     if (m_storageManager)
-        m_storageManager->setAllowedSessionStorageNamespaceConnection(webPageProxy.pageID(), nullptr);
+        m_storageManager->removeAllowedSessionStorageNamespaceConnection(webPageProxy.pageID(), connection);
 }
 
 void WebsiteDataStore::webProcessDidCloseConnection(WebProcessProxy& webProcessProxy, IPC::Connection& connection)
index 5a91204..b337822 100644 (file)
@@ -1,3 +1,14 @@
+2019-02-27  Chris Dumez  <cdumez@apple.com>
+
+        Flaky API Test: TestWebKitAPI.ProcessSwap.SessionStorage
+        https://bugs.webkit.org/show_bug.cgi?id=194480
+
+        Reviewed by Brady Eidson.
+
+        Update existing API test to make it more likely to reproduce the issue.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
 2019-02-27  Brady Eidson  <beidson@apple.com>
 
         Universal links from Google search results pages don't open the app.
index 70d4d95..47e9f5b 100644 (file)
@@ -2288,58 +2288,61 @@ window.onload = function(evt) {
 
 TEST(ProcessSwap, SessionStorage)
 {
-    auto processPoolConfiguration = psonProcessPoolConfiguration();
-    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+    for (int i = 0; i < 5; ++i) {
+        [receivedMessages removeAllObjects];
+        auto processPoolConfiguration = psonProcessPoolConfiguration();
+        auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
 
-    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
-    [webViewConfiguration setProcessPool:processPool.get()];
-    auto handler = adoptNS([[PSONScheme alloc] init]);
-    [handler addMappingFromURLString:@"pson://www.webkit.org/main.html" toData:sessionStorageTestBytes];
-    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
+        auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
+        [webViewConfiguration setProcessPool:processPool.get()];
+        auto handler = adoptNS([[PSONScheme alloc] init]);
+        [handler addMappingFromURLString:@"pson://www.webkit.org/main.html" toData:sessionStorageTestBytes];
+        [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
 
-    auto messageHandler = adoptNS([[PSONMessageHandler alloc] init]);
-    [[webViewConfiguration userContentController] addScriptMessageHandler:messageHandler.get() name:@"pson"];
+        auto messageHandler = adoptNS([[PSONMessageHandler alloc] init]);
+        [[webViewConfiguration userContentController] addScriptMessageHandler:messageHandler.get() name:@"pson"];
 
-    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
-    auto delegate = adoptNS([[PSONNavigationDelegate alloc] init]);
-    [webView setNavigationDelegate:delegate.get()];
+        auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+        auto delegate = adoptNS([[PSONNavigationDelegate alloc] init]);
+        [webView setNavigationDelegate:delegate.get()];
 
-    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
-    [webView loadRequest:request];
+        NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
+        [webView loadRequest:request];
 
-    TestWebKitAPI::Util::run(&receivedMessage);
-    receivedMessage = false;
-    TestWebKitAPI::Util::run(&done);
-    done = false;
+        TestWebKitAPI::Util::run(&receivedMessage);
+        receivedMessage = false;
+        TestWebKitAPI::Util::run(&done);
+        done = false;
 
-    auto webkitPID = [webView _webProcessIdentifier];
+        auto webkitPID = [webView _webProcessIdentifier];
 
-    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main.html"]];
-    [webView loadRequest:request];
+        request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main.html"]];
+        [webView loadRequest:request];
 
-    TestWebKitAPI::Util::run(&done);
-    done = false;
+        TestWebKitAPI::Util::run(&done);
+        done = false;
 
-    auto applePID = [webView _webProcessIdentifier];
+        auto applePID = [webView _webProcessIdentifier];
 
-    // Verify the web pages are in different processes
-    EXPECT_NE(webkitPID, applePID);
+        // Verify the web pages are in different processes
+        EXPECT_NE(webkitPID, applePID);
 
-    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
-    [webView loadRequest:request];
+        request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
+        [webView loadRequest:request];
 
-    TestWebKitAPI::Util::run(&receivedMessage);
-    receivedMessage = false;
-    TestWebKitAPI::Util::run(&done);
-    done = false;
+        TestWebKitAPI::Util::run(&receivedMessage);
+        receivedMessage = false;
+        TestWebKitAPI::Util::run(&done);
+        done = false;
 
-    // We should have gone back to the webkit.org process for this load since we reuse SuspendedPages' process when possible.
-    EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
+        // We should have gone back to the webkit.org process for this load since we reuse SuspendedPages' process when possible.
+        EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
 
-    // Verify the sessionStorage values were as expected
-    EXPECT_EQ([receivedMessages count], 2u);
-    EXPECT_TRUE([receivedMessages.get()[0] isEqualToString:@""]);
-    EXPECT_TRUE([receivedMessages.get()[1] isEqualToString:@"I exist!"]);
+        // Verify the sessionStorage values were as expected
+        EXPECT_EQ([receivedMessages count], 2u);
+        EXPECT_TRUE([receivedMessages.get()[0] isEqualToString:@""]);
+        EXPECT_TRUE([receivedMessages.get()[1] isEqualToString:@"I exist!"]);
+    }
 }
 
 TEST(ProcessSwap, ReuseSuspendedProcess)