Improve page to process relationship tracking
authorap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Aug 2014 23:56:57 +0000 (23:56 +0000)
committerap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Aug 2014 23:56:57 +0000 (23:56 +0000)
https://bugs.webkit.org/show_bug.cgi?id=135996
<rdar://problem/16991213>

Reviewed by Sam Weinig.

* UIProcess/VisitedLinkProvider.cpp:
(WebKit::VisitedLinkProvider::removeAll):
(WebKit::VisitedLinkProvider::pendingVisitedLinksTimerFired):
(WebKit::VisitedLinkProvider::sendTable):
Added assertions for m_processes only having valid entries.

* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::reattachToWebProcess): When attaching to a new process,
tell the old process that the page is not longer associated with it, avoiding
a potential stale pointer.
If re-attached to an existing process, make sure that we perform all the same
registrations as after having launched a new process. This substantially improves
the behavior when the number of open tabs is over process limit.
(WebKit::WebPageProxy::reattachToWebProcessWithItem): Added ASSERT(!isValid())
to avoid confusion. All other calls to reattachToWebProcess() have this as a
runtime check, but reattachToWebProcessWithItem() is only called for valid pages.
(WebKit::WebPageProxy::terminateProcess): Added an assertion with a FIXME for
something that will need to be fixed another day.

* UIProcess/WebPageProxy.h: Removed an unimplemented function.

* UIProcess/WebProcessProxy.cpp:
(WebKit::WebProcessProxy::addExistingWebPage): Added assertions for page map sanity.
(WebKit::WebProcessProxy::removeWebPage): Added a check for page state being Terminated
already. This avoids an assertion failure that happened under the new call to
removeWebPage() in reattachToWebProcess(), as we are now calling it for terminated
processes that are not in WebContext::m_processes any more.
(WebKit::WebProcessProxy::didFinishLaunching): Added an assertion that page agrees
about using this process.

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

Source/WebKit2/ChangeLog
Source/WebKit2/UIProcess/VisitedLinkProvider.cpp
Source/WebKit2/UIProcess/WebPageProxy.cpp
Source/WebKit2/UIProcess/WebPageProxy.h
Source/WebKit2/UIProcess/WebProcessProxy.cpp

index 38c22f2..0a4c818 100644 (file)
@@ -1,3 +1,41 @@
+2014-08-15  Alexey Proskuryakov  <ap@apple.com>
+
+        Improve page to process relationship tracking
+        https://bugs.webkit.org/show_bug.cgi?id=135996
+        <rdar://problem/16991213>
+
+        Reviewed by Sam Weinig.
+
+        * UIProcess/VisitedLinkProvider.cpp:
+        (WebKit::VisitedLinkProvider::removeAll):
+        (WebKit::VisitedLinkProvider::pendingVisitedLinksTimerFired):
+        (WebKit::VisitedLinkProvider::sendTable):
+        Added assertions for m_processes only having valid entries.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::reattachToWebProcess): When attaching to a new process,
+        tell the old process that the page is not longer associated with it, avoiding
+        a potential stale pointer.
+        If re-attached to an existing process, make sure that we perform all the same
+        registrations as after having launched a new process. This substantially improves
+        the behavior when the number of open tabs is over process limit.
+        (WebKit::WebPageProxy::reattachToWebProcessWithItem): Added ASSERT(!isValid())
+        to avoid confusion. All other calls to reattachToWebProcess() have this as a
+        runtime check, but reattachToWebProcessWithItem() is only called for valid pages.
+        (WebKit::WebPageProxy::terminateProcess): Added an assertion with a FIXME for
+        something that will need to be fixed another day.
+
+        * UIProcess/WebPageProxy.h: Removed an unimplemented function.
+
+        * UIProcess/WebProcessProxy.cpp:
+        (WebKit::WebProcessProxy::addExistingWebPage): Added assertions for page map sanity.
+        (WebKit::WebProcessProxy::removeWebPage): Added a check for page state being Terminated
+        already. This avoids an assertion failure that happened under the new call to
+        removeWebPage() in reattachToWebProcess(), as we are now calling it for terminated
+        processes that are not in WebContext::m_processes any more.
+        (WebKit::WebProcessProxy::didFinishLaunching): Added an assertion that page agrees
+        about using this process.
+
 2014-08-15  Gavin Barraclough  <barraclough@apple.com>
 
         Fix plugin visibility check.
index 7cb6399..5e4a1ce 100644 (file)
@@ -97,8 +97,11 @@ void VisitedLinkProvider::removeAll()
     m_tableSize = 0;
     m_table.clear();
 
-    for (auto& processAndCount : m_processes)
-        processAndCount.key->connection()->send(Messages::VisitedLinkTableController::RemoveAllVisitedLinks(), m_identifier);
+    for (auto& processAndCount : m_processes) {
+        WebProcessProxy* process = processAndCount.key;
+        ASSERT(process->context().processes().contains(process));
+        process->connection()->send(Messages::VisitedLinkTableController::RemoveAllVisitedLinks(), m_identifier);
+    }
 }
 
 void VisitedLinkProvider::addVisitedLinkHash(LinkHash linkHash)
@@ -175,10 +178,13 @@ void VisitedLinkProvider::pendingVisitedLinksTimerFired()
         return;
 
     for (auto& processAndCount : m_processes) {
+        WebProcessProxy* process = processAndCount.key;
+        ASSERT(process->context().processes().contains(process));
+
         if (addedVisitedLinks.size() > 20)
-            processAndCount.key->connection()->send(Messages::VisitedLinkTableController::AllVisitedLinkStateChanged(), m_identifier);
+            process->connection()->send(Messages::VisitedLinkTableController::AllVisitedLinkStateChanged(), m_identifier);
         else
-            processAndCount.key->connection()->send(Messages::VisitedLinkTableController::VisitedLinkStateChanged(addedVisitedLinks), m_identifier);
+            process->connection()->send(Messages::VisitedLinkTableController::VisitedLinkStateChanged(addedVisitedLinks), m_identifier);
     }
 }
 
@@ -229,6 +235,8 @@ void VisitedLinkProvider::resizeTable(unsigned newTableSize)
 
 void VisitedLinkProvider::sendTable(WebProcessProxy& process)
 {
+    ASSERT(process.context().processes().contains(&process));
+
     SharedMemory::Handle handle;
     if (!m_table.sharedMemory()->createHandle(handle, SharedMemory::ReadOnly))
         return;
index f3ba104..a9bfad8 100644 (file)
@@ -545,11 +545,16 @@ void WebPageProxy::reattachToWebProcess()
     ASSERT(m_process->state() == WebProcessProxy::State::Terminated);
 
     m_isValid = true;
+    m_process->removeWebPage(m_pageID);
 
     if (m_process->context().processModel() == ProcessModelSharedSecondaryProcess)
         m_process = m_process->context().ensureSharedWebProcess();
     else
         m_process = m_process->context().createNewWebProcessRespectingProcessCountLimit();
+
+    ASSERT(m_process->state() != ChildProcessProxy::State::Terminated);
+    if (m_process->state() == ChildProcessProxy::State::Running)
+        processDidFinishLaunching();
     m_process->addExistingWebPage(this, m_pageID);
     m_process->addMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_pageID, *this);
 
@@ -579,7 +584,8 @@ uint64_t WebPageProxy::reattachToWebProcessWithItem(WebBackForwardListItem* item
 
     if (item && item != m_backForwardList->currentItem())
         m_backForwardList->goToItem(item);
-    
+
+    ASSERT(!isValid());
     reattachToWebProcess();
 
     if (!item)
@@ -1865,6 +1871,10 @@ void WebPageProxy::setCustomTextEncodingName(const String& encodingName)
 
 void WebPageProxy::terminateProcess()
 {
+    // requestTermination() is a no-op for launching processes, so we get into an inconsistent state by calling resetStateAfterProcessExited().
+    // FIXME: A client can terminate the page at any time, so we should do something more meaningful than assert and fall apart in release builds.
+    ASSERT(m_process->state() != WebProcessProxy::State::Launching);
+
     // NOTE: This uses a check of m_isValid rather than calling isValid() since
     // we want this to run even for pages being closed or that already closed.
     if (!m_isValid)
index cf37bfc..367c69f 100644 (file)
@@ -740,8 +740,6 @@ public:
 
     bool isValid() const;
 
-    PassRefPtr<API::Array> relatedPages() const;
-
     const String& urlAtProcessExit() const { return m_urlAtProcessExit; }
     FrameLoadState::State loadStateAtProcessExit() const { return m_loadStateAtProcessExit; }
 
index 807a892..3863350 100644 (file)
@@ -186,6 +186,9 @@ PassRefPtr<WebPageProxy> WebProcessProxy::createWebPage(PageClient& pageClient,
 
 void WebProcessProxy::addExistingWebPage(WebPageProxy* webPage, uint64_t pageID)
 {
+    ASSERT(!m_pageMap.contains(pageID));
+    ASSERT(!globalPageMap().contains(pageID));
+
     m_pageMap.set(pageID, webPage);
     globalPageMap().set(pageID, webPage);
 #if PLATFORM(COCOA)
@@ -215,7 +218,7 @@ void WebProcessProxy::removeWebPage(uint64_t pageID)
 
     // If this was the last WebPage open in that web process, and we have no other reason to keep it alive, let it go.
     // We only allow this when using a network process, as otherwise the WebProcess needs to preserve its session state.
-    if (!m_context->usesNetworkProcess() || !canTerminateChildProcess())
+    if (!m_context->usesNetworkProcess() || state() == State::Terminated || !canTerminateChildProcess())
         return;
 
     abortProcessLaunchIfNeeded();
@@ -471,8 +474,10 @@ void WebProcessProxy::didFinishLaunching(ProcessLauncher* launcher, IPC::Connect
 {
     ChildProcessProxy::didFinishLaunching(launcher, connectionIdentifier);
 
-    for (auto& page : m_pageMap.values())
+    for (WebPageProxy* page : m_pageMap.values()) {
+        ASSERT(this == &page->process());
         page->processDidFinishLaunching();
+    }
 
     m_webConnection = WebConnectionToWebProcess::create(this);