Spew: Unhandled web process message 'VisitedLinkTableController:VisitedLinkStateChanged'
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Mar 2019 16:48:40 +0000 (16:48 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Mar 2019 16:48:40 +0000 (16:48 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194787
<rdar://problem/48175520>

Reviewed by Geoffrey Garen.

The unhandled 'VisitedLinkTableController:VisitedLinkStateChanged' message logging happens
when IPC is sent to a WebProcess which does not have a VisitedLinkTableController with the
given identifier. VisitedLinkTableController are kept alive by the WebPage in the WebProcess
side so this indicates that there is no WebPage using this VisitedLinkTableController anymore.

In the UIProcess side, our tracking of who is using which VisitedLinkStore was very poor.
WebPageProxy objects would ask their process to register itself with the page's visitedLinkStore
as soon as the WebPage object has been created on the WebProcess side. This part was fine.
However, unregistration from the visitedLinkStores would only happen when either the
visitedLinkStore would get destroyed or when the WebProcess would shutdown. This means that
WebProcess could stay registered with a visitedLinkStore even after the page that was using it
has been closed, which would lead to such logging.

To address the issue, the WebProcessProxy now keeps track for which pages are using which
visitedLinkStore. When a visitedLinkStore is used by a page for the first time, the
WebProcessProxy will register itself with the visitedLinkStore. Similarly, when the last page
using a given visitedLinkStore is closed, the process unregisters itself from the
visitedLinkStore, thus avoiding the bug.

I also simplified a lot the logic for having a page telling the WebProcessProxy it started
using a visitedLinkStore. Previously, it would have to wait until the process is done launching
before notifying the WebProcessProxy. Now, the WebPageProxy merely tells the WebProcessProxy
that it is starting to use a visitedLinkStore as soon as it sent the CreateWebPage IPC to the
WebProcess (no matter if the process is still launching or not). At this point, the
WebProcessProxy registers the page as a user of the visitedLinkStore and takes care of waiting
until it is done launching before registering itself with the visitedLinkStore.

* UIProcess/ProvisionalPageProxy.cpp:
(WebKit::ProvisionalPageProxy::~ProvisionalPageProxy):
(WebKit::ProvisionalPageProxy::initializeWebPage):
(WebKit::ProvisionalPageProxy::processDidFinishLaunching): Deleted.
(WebKit::ProvisionalPageProxy::finishInitializingWebPageAfterProcessLaunch): Deleted.
* UIProcess/ProvisionalPageProxy.h:
* UIProcess/VisitedLinkStore.cpp:
(WebKit::VisitedLinkStore::~VisitedLinkStore):
(WebKit::VisitedLinkStore::addProcess):
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::finishAttachingToWebProcess):
(WebKit::WebPageProxy::initializeWebPage):
(WebKit::WebPageProxy::resetStateAfterProcessExited):
(WebKit::WebPageProxy::finishInitializingWebPageAfterProcessLaunch): Deleted.
(WebKit::WebPageProxy::processDidFinishLaunching): Deleted.
* UIProcess/WebPageProxy.h:
* UIProcess/WebProcessProxy.cpp:
(WebKit::WebProcessProxy::shutDown):
(WebKit::WebProcessProxy::removeWebPage):
(WebKit::WebProcessProxy::addVisitedLinkStoreUser):
(WebKit::WebProcessProxy::removeVisitedLinkStoreUser):
(WebKit::WebProcessProxy::addWebUserContentControllerProxy):
(WebKit::WebProcessProxy::didFinishLaunching):
(WebKit::WebProcessProxy::addVisitedLinkStore): Deleted.
(WebKit::WebProcessProxy::didDestroyVisitedLinkStore): Deleted.
* UIProcess/WebProcessProxy.h:

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

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/ProvisionalPageProxy.cpp
Source/WebKit/UIProcess/ProvisionalPageProxy.h
Source/WebKit/UIProcess/VisitedLinkStore.cpp
Source/WebKit/UIProcess/WebPageProxy.cpp
Source/WebKit/UIProcess/WebPageProxy.h
Source/WebKit/UIProcess/WebProcessProxy.cpp
Source/WebKit/UIProcess/WebProcessProxy.h

index 726ba75..e085817 100644 (file)
@@ -1,3 +1,65 @@
+2019-03-19  Chris Dumez  <cdumez@apple.com>
+
+        Spew: Unhandled web process message 'VisitedLinkTableController:VisitedLinkStateChanged'
+        https://bugs.webkit.org/show_bug.cgi?id=194787
+        <rdar://problem/48175520>
+
+        Reviewed by Geoffrey Garen.
+
+        The unhandled 'VisitedLinkTableController:VisitedLinkStateChanged' message logging happens
+        when IPC is sent to a WebProcess which does not have a VisitedLinkTableController with the
+        given identifier. VisitedLinkTableController are kept alive by the WebPage in the WebProcess
+        side so this indicates that there is no WebPage using this VisitedLinkTableController anymore.
+
+        In the UIProcess side, our tracking of who is using which VisitedLinkStore was very poor.
+        WebPageProxy objects would ask their process to register itself with the page's visitedLinkStore
+        as soon as the WebPage object has been created on the WebProcess side. This part was fine.
+        However, unregistration from the visitedLinkStores would only happen when either the
+        visitedLinkStore would get destroyed or when the WebProcess would shutdown. This means that
+        WebProcess could stay registered with a visitedLinkStore even after the page that was using it
+        has been closed, which would lead to such logging.
+
+        To address the issue, the WebProcessProxy now keeps track for which pages are using which
+        visitedLinkStore. When a visitedLinkStore is used by a page for the first time, the
+        WebProcessProxy will register itself with the visitedLinkStore. Similarly, when the last page
+        using a given visitedLinkStore is closed, the process unregisters itself from the
+        visitedLinkStore, thus avoiding the bug.
+
+        I also simplified a lot the logic for having a page telling the WebProcessProxy it started
+        using a visitedLinkStore. Previously, it would have to wait until the process is done launching
+        before notifying the WebProcessProxy. Now, the WebPageProxy merely tells the WebProcessProxy
+        that it is starting to use a visitedLinkStore as soon as it sent the CreateWebPage IPC to the
+        WebProcess (no matter if the process is still launching or not). At this point, the
+        WebProcessProxy registers the page as a user of the visitedLinkStore and takes care of waiting
+        until it is done launching before registering itself with the visitedLinkStore.
+
+        * UIProcess/ProvisionalPageProxy.cpp:
+        (WebKit::ProvisionalPageProxy::~ProvisionalPageProxy):
+        (WebKit::ProvisionalPageProxy::initializeWebPage):
+        (WebKit::ProvisionalPageProxy::processDidFinishLaunching): Deleted.
+        (WebKit::ProvisionalPageProxy::finishInitializingWebPageAfterProcessLaunch): Deleted.
+        * UIProcess/ProvisionalPageProxy.h:
+        * UIProcess/VisitedLinkStore.cpp:
+        (WebKit::VisitedLinkStore::~VisitedLinkStore):
+        (WebKit::VisitedLinkStore::addProcess):
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::finishAttachingToWebProcess):
+        (WebKit::WebPageProxy::initializeWebPage):
+        (WebKit::WebPageProxy::resetStateAfterProcessExited):
+        (WebKit::WebPageProxy::finishInitializingWebPageAfterProcessLaunch): Deleted.
+        (WebKit::WebPageProxy::processDidFinishLaunching): Deleted.
+        * UIProcess/WebPageProxy.h:
+        * UIProcess/WebProcessProxy.cpp:
+        (WebKit::WebProcessProxy::shutDown):
+        (WebKit::WebProcessProxy::removeWebPage):
+        (WebKit::WebProcessProxy::addVisitedLinkStoreUser):
+        (WebKit::WebProcessProxy::removeVisitedLinkStoreUser):
+        (WebKit::WebProcessProxy::addWebUserContentControllerProxy):
+        (WebKit::WebProcessProxy::didFinishLaunching):
+        (WebKit::WebProcessProxy::addVisitedLinkStore): Deleted.
+        (WebKit::WebProcessProxy::didDestroyVisitedLinkStore): Deleted.
+        * UIProcess/WebProcessProxy.h:
+
 2019-03-19  Alex Christensen  <achristensen@webkit.org>
 
         Make WTFLogChannelState and WTFLogLevel enum classes
index 09a28b0..bbff68b 100644 (file)
@@ -99,6 +99,7 @@ ProvisionalPageProxy::~ProvisionalPageProxy()
 
     m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID());
     m_process->send(Messages::WebPage::Close(), m_page.pageID());
+    m_process->removeVisitedLinkStoreUser(m_page.visitedLinkStore(), m_page.pageID());
 
     RunLoop::main().dispatch([process = m_process.copyRef()] {
         process->maybeShutDown();
@@ -136,21 +137,6 @@ void ProvisionalPageProxy::cancel()
     didFailProvisionalLoadForFrame(m_mainFrame->frameID(), { }, m_navigationID, m_provisionalLoadURL, error, UserData { }); // Will delete |this|.
 }
 
-void ProvisionalPageProxy::processDidFinishLaunching()
-{
-    RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "processDidFinishLaunching: pageID = %" PRIu64, m_page.pageID());
-    finishInitializingWebPageAfterProcessLaunch();
-}
-
-void ProvisionalPageProxy::finishInitializingWebPageAfterProcessLaunch()
-{
-    ASSERT(m_process->state() == WebProcessProxy::State::Running);
-
-    // FIXME: The WebPageProxy delays adding the visited link store until after the process has launched
-    // so the ProvisionalPageProxy does the same. However, do we really need to?
-    m_process->addVisitedLinkStore(m_page.visitedLinkStore());
-}
-
 void ProvisionalPageProxy::initializeWebPage()
 {
     m_drawingArea = m_page.pageClient().createDrawingAreaProxy(m_process);
@@ -158,9 +144,7 @@ void ProvisionalPageProxy::initializeWebPage()
     auto parameters = m_page.creationParameters(m_process, *m_drawingArea);
     parameters.isProcessSwap = true;
     m_process->send(Messages::WebProcess::CreateWebPage(m_page.pageID(), parameters), 0);
-
-    if (m_process->state() == WebProcessProxy::State::Running)
-        finishInitializingWebPageAfterProcessLaunch();
+    m_process->addVisitedLinkStoreUser(m_page.visitedLinkStore(), m_page.pageID());
 }
 
 void ProvisionalPageProxy::loadData(API::Navigation& navigation, const IPC::DataReference& data, const String& MIMEType, const String& encoding, const String& baseURL, API::Object* userData, Optional<WebsitePoliciesData>&& websitePolicies)
index 16bd5cd..1ce9780 100644 (file)
@@ -79,7 +79,6 @@ public:
     void goToBackForwardItem(API::Navigation&, WebBackForwardListItem&, Optional<WebsitePoliciesData>&&);
     void cancel();
 
-    void processDidFinishLaunching();
     void processDidTerminate();
     void connectionWillOpen(IPC::Connection&);
 
@@ -115,7 +114,6 @@ private:
 #endif
 
     void initializeWebPage();
-    void finishInitializingWebPageAfterProcessLaunch();
 
     WebPageProxy& m_page;
     Ref<WebProcessProxy> m_process;
index 29bf944..fac3cdd 100644 (file)
@@ -42,10 +42,7 @@ Ref<VisitedLinkStore> VisitedLinkStore::create()
 
 VisitedLinkStore::~VisitedLinkStore()
 {
-    for (WebProcessProxy* process : m_processes) {
-        process->removeMessageReceiver(Messages::VisitedLinkStore::messageReceiverName(), identifier());
-        process->didDestroyVisitedLinkStore(*this);
-    }
+    RELEASE_ASSERT(m_processes.isEmpty());
 }
 
 VisitedLinkStore::VisitedLinkStore()
@@ -56,6 +53,7 @@ VisitedLinkStore::VisitedLinkStore()
 void VisitedLinkStore::addProcess(WebProcessProxy& process)
 {
     ASSERT(process.state() == WebProcessProxy::State::Running);
+    ASSERT(!m_processes.contains(&process));
 
     if (!m_processes.add(&process).isNewEntry)
         return;
index 4bb0141..cb56e4f 100644 (file)
@@ -819,7 +819,6 @@ void WebPageProxy::finishAttachingToWebProcess(IsProcessSwap isProcessSwap)
         // when the process was provisional.
         if (isProcessSwap != IsProcessSwap::Yes)
             m_webProcessLifetimeTracker.webPageEnteringWebProcess(m_process);
-        processDidFinishLaunching();
     }
 
     updateActivityState();
@@ -958,19 +957,7 @@ void WebPageProxy::initializeWebPage()
 
     process().send(Messages::WebProcess::CreateWebPage(m_pageID, creationParameters(m_process, *m_drawingArea)), 0);
 
-    m_needsToFinishInitializingWebPageAfterProcessLaunch = true;
-    finishInitializingWebPageAfterProcessLaunch();
-}
-
-void WebPageProxy::finishInitializingWebPageAfterProcessLaunch()
-{
-    if (!m_needsToFinishInitializingWebPageAfterProcessLaunch)
-        return;
-    if (m_process->state() != WebProcessProxy::State::Running)
-        return;
-
-    m_needsToFinishInitializingWebPageAfterProcessLaunch = false;
-    m_process->addVisitedLinkStore(m_visitedLinkStore);
+    m_process->addVisitedLinkStoreUser(visitedLinkStore(), m_pageID);
 }
 
 void WebPageProxy::close()
@@ -5113,12 +5100,6 @@ void WebPageProxy::webProcessWillShutDown()
     m_webProcessLifetimeTracker.webPageLeavingWebProcess(m_process);
 }
 
-void WebPageProxy::processDidFinishLaunching()
-{
-    ASSERT(m_process->state() == WebProcessProxy::State::Running);
-    finishInitializingWebPageAfterProcessLaunch();
-}
-
 #if ENABLE(NETSCAPE_PLUGIN_API)
 void WebPageProxy::unavailablePluginButtonClicked(uint32_t opaquePluginUnavailabilityReason, const String& mimeType, const String& pluginURLString, const String& pluginspageAttributeURLString, const String& frameURLString, const String& pageURLString)
 {
@@ -6924,8 +6905,6 @@ void WebPageProxy::resetStateAfterProcessExited(ProcessTerminationReason termina
     m_hasRunningProcess = false;
     m_isPageSuspended = false;
 
-    m_needsToFinishInitializingWebPageAfterProcessLaunch = false;
-
     m_editorState = EditorState();
     m_cachedFontAttributesAtSelectionStart.reset();
 
index 4c6cebf..4c05c24 100644 (file)
@@ -1233,8 +1233,6 @@ public:
     void connectionWillOpen(IPC::Connection&);
     void webProcessWillShutDown();
 
-    void processDidFinishLaunching();
-
     void didSaveToPageCache();
         
     void setScrollPinningBehavior(WebCore::ScrollPinningBehavior);
@@ -1974,8 +1972,6 @@ private:
 
     void didResignInputElementStrongPasswordAppearance(const UserData&);
 
-    void finishInitializingWebPageAfterProcessLaunch();
-
     void handleMessage(IPC::Connection&, const String& messageName, const UserData& messageBody);
     void handleSynchronousMessage(IPC::Connection&, const String& messageName, const UserData& messageBody, UserData& returnUserData);
 
@@ -2225,8 +2221,6 @@ private:
     // Whether it can run modal child web pages.
     bool m_canRunModal { false };
 
-    bool m_needsToFinishInitializingWebPageAfterProcessLaunch { false };
-
     bool m_isInPrintingMode { false };
     bool m_isPerformingDOMPrintOperation { false };
 
index 46e8316..9a37395 100644 (file)
@@ -298,9 +298,9 @@ void WebProcessProxy::shutDown()
         frame->webProcessWillShutDown();
     m_frameMap.clear();
 
-    for (auto* visitedLinkStore : m_visitedLinkStores)
+    for (auto* visitedLinkStore : m_visitedLinkStoresWithUsers.keys())
         visitedLinkStore->removeProcess(*this);
-    m_visitedLinkStores.clear();
+    m_visitedLinkStoresWithUsers.clear();
 
     for (auto* webUserContentControllerProxy : m_webUserContentControllerProxies)
         webUserContentControllerProxy->removeProcess(*this);
@@ -392,27 +392,44 @@ void WebProcessProxy::removeWebPage(WebPageProxy& webPage, EndsUsingDataStore en
     if (endsUsingDataStore == EndsUsingDataStore::Yes)
         m_processPool->pageEndUsingWebsiteDataStore(webPage.pageID(), webPage.websiteDataStore());
 
+    removeVisitedLinkStoreUser(webPage.visitedLinkStore(), webPage.pageID());
+
     updateBackgroundResponsivenessTimer();
 
     maybeShutDown();
 }
 
-void WebProcessProxy::addVisitedLinkStore(VisitedLinkStore& store)
+void WebProcessProxy::addVisitedLinkStoreUser(VisitedLinkStore& visitedLinkStore, uint64_t pageID)
 {
-    m_visitedLinkStores.add(&store);
-    store.addProcess(*this);
+    auto users = m_visitedLinkStoresWithUsers.ensure(&visitedLinkStore, [] {
+        return HashSet<uint64_t> { };
+    }).iterator->value;
+
+    ASSERT(!users.contains(pageID));
+    users.add(pageID);
+
+    if (users.size() == 1 && state() == State::Running)
+        visitedLinkStore.addProcess(*this);
 }
 
-void WebProcessProxy::addWebUserContentControllerProxy(WebUserContentControllerProxy& proxy, WebPageCreationParameters& parameters)
+void WebProcessProxy::removeVisitedLinkStoreUser(VisitedLinkStore& visitedLinkStore, uint64_t pageID)
 {
-    m_webUserContentControllerProxies.add(&proxy);
-    proxy.addProcess(*this, parameters);
+    auto it = m_visitedLinkStoresWithUsers.find(&visitedLinkStore);
+    if (it == m_visitedLinkStoresWithUsers.end())
+        return;
+
+    auto& users = it->value;
+    users.remove(pageID);
+    if (users.isEmpty()) {
+        m_visitedLinkStoresWithUsers.remove(it);
+        visitedLinkStore.removeProcess(*this);
+    }
 }
 
-void WebProcessProxy::didDestroyVisitedLinkStore(VisitedLinkStore& store)
+void WebProcessProxy::addWebUserContentControllerProxy(WebUserContentControllerProxy& proxy, WebPageCreationParameters& parameters)
 {
-    ASSERT(m_visitedLinkStores.contains(&store));
-    m_visitedLinkStores.remove(&store);
+    m_webUserContentControllerProxies.add(&proxy);
+    proxy.addProcess(*this, parameters);
 }
 
 void WebProcessProxy::didDestroyWebUserContentControllerProxy(WebUserContentControllerProxy& proxy)
@@ -739,22 +756,14 @@ void WebProcessProxy::didFinishLaunching(ProcessLauncher* launcher, IPC::Connect
         return;
     }
 
-    for (WebPageProxy* page : m_pageMap.values()) {
-        ASSERT(this == &page->process());
-        page->processDidFinishLaunching();
-    }
-
-    for (auto* provisionalPage : m_provisionalPages) {
-        ASSERT(this == &provisionalPage->process());
-        provisionalPage->processDidFinishLaunching();
-    }
-
-
     RELEASE_ASSERT(!m_webConnection);
     m_webConnection = WebConnectionToWebProcess::create(this);
 
     m_processPool->processDidFinishLaunching(this);
 
+    for (auto* visitedLinkStore : m_visitedLinkStoresWithUsers.keys())
+        visitedLinkStore->addProcess(*this);
+
 #if PLATFORM(IOS_FAMILY)
     if (connection()) {
         if (xpc_connection_t xpcConnection = connection()->xpcConnection())
index ff6a9ec..276490d 100644 (file)
@@ -146,9 +146,12 @@ public:
 
     virtual bool isServiceWorkerProcess() const { return false; }
 
-    void addVisitedLinkStore(VisitedLinkStore&);
+    void didCreateWebPageInProcess(uint64_t pageID);
+
+    void addVisitedLinkStoreUser(VisitedLinkStore&, uint64_t pageID);
+    void removeVisitedLinkStoreUser(VisitedLinkStore&, uint64_t pageID);
+
     void addWebUserContentControllerProxy(WebUserContentControllerProxy&, WebPageCreationParameters&);
-    void didDestroyVisitedLinkStore(VisitedLinkStore&);
     void didDestroyWebUserContentControllerProxy(WebUserContentControllerProxy&);
 
     RefPtr<API::UserInitiatedAction> userInitiatedActivity(uint64_t);
@@ -428,7 +431,7 @@ private:
     HashSet<ProvisionalPageProxy*> m_provisionalPages;
     UserInitiatedActionMap m_userInitiatedActionMap;
 
-    HashSet<VisitedLinkStore*> m_visitedLinkStores;
+    HashMap<VisitedLinkStore*, HashSet<uint64_t/* pageID */>> m_visitedLinkStoresWithUsers;
     HashSet<WebUserContentControllerProxy*> m_webUserContentControllerProxies;
 
     int m_numberOfTimesSuddenTerminationWasDisabled;