Skip to content
Permalink
Browse files
Flaky API Test: TestWebKitAPI.ProcessSwap.SessionStorage
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:


Canonical link: https://commits.webkit.org/209475@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@242182 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
cdumez committed Feb 28, 2019
1 parent a1acd1d commit 6c1d418e9382f7519d21bef3273a96b82c137ab0
Showing 15 changed files with 203 additions and 87 deletions.
@@ -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
@@ -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());
@@ -80,6 +80,7 @@ class ProvisionalPageProxy : public IPC::MessageReceiver, public CanMakeWeakPtr<

void processDidFinishLaunching();
void processDidTerminate();
void connectionWillOpen(IPC::Connection&);

private:
// IPC::MessageReceiver
@@ -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);
@@ -377,6 +377,8 @@ class WebPageProxy : public API::ObjectImpl<API::Object::Type::Page>

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 @@ class WebPageProxy : public API::ObjectImpl<API::Object::Type::Page>
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&);
@@ -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));
@@ -43,8 +43,8 @@ class WebProcessLifetimeObserver {
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;

@@ -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;
}

}
@@ -36,6 +36,7 @@ namespace WebKit {

class WebPageProxy;
class WebProcessLifetimeObserver;
class WebProcessProxy;

class WebProcessLifetimeTracker {
public:
@@ -44,13 +45,13 @@ class WebProcessLifetimeTracker {

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;

@@ -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)

0 comments on commit 6c1d418

Please sign in to comment.