Changeset 172660 in webkit

Timestamp:

Aug 15, 2014 4:56:57 PM (10 years ago)

Author:

ap@apple.com

Message:

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.

Location:

trunk/Source/WebKit2

Files:

• ChangeLog (modified) (1 diff)
• UIProcess/VisitedLinkProvider.cpp (modified) (3 diffs)
• UIProcess/WebPageProxy.cpp (modified) (4 diffs)
• UIProcess/WebPageProxy.h (modified) (1 diff)
• UIProcess/WebProcessProxy.cpp (modified) (3 diffs)

trunk/Source/WebKit2/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebKit2/UIProcess/VisitedLinkProvider.cpp

@@ -98 +98 @@
     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);
+    }
 }
 
@@ -176 +179 @@
 
     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);
     }
 }
@@ -230 +236 @@
 void VisitedLinkProvider::sendTable(WebProcessProxy& process)
 {
+    ASSERT(process.context().processes().contains(&process));
+
     SharedMemory::Handle handle;
     if (!m_table.sharedMemory()->createHandle(handle, SharedMemory::ReadOnly))

trunk/Source/WebKit2/UIProcess/WebPageProxy.cpp

@@ -546 +546 @@
 
     m_isValid = true;
+    m_process->removeWebPage(m_pageID);
 
     if (m_process->context().processModel() == ProcessModelSharedSecondaryProcess)
@@ -551 +552 @@
     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);
@@ -580 +585 @@
     if (item && item != m_backForwardList->currentItem())
         m_backForwardList->goToItem(item);
-    
+
+    ASSERT(!isValid());
     reattachToWebProcess();
 
@@ -1866 +1872 @@
 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.

trunk/Source/WebKit2/UIProcess/WebPageProxy.h

@@ -741 +741 @@
     bool isValid() const;
 
-    PassRefPtr<API::Array> relatedPages() const;
-
     const String& urlAtProcessExit() const { return m_urlAtProcessExit; }
     FrameLoadState::State loadStateAtProcessExit() const { return m_loadStateAtProcessExit; }

trunk/Source/WebKit2/UIProcess/WebProcessProxy.cpp

@@ -187 +187 @@
 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);
@@ -216 +219 @@
     // 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;
 
@@ -472 +475 @@
     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);