Changeset 243142 in webkit


Ignore:
Timestamp:
Mar 19, 2019 9:48:40 AM (5 years ago)
Author:
Chris Dumez
Message:

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:
Location:
trunk/Source/WebKit
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r243141 r243142  
     12019-03-19  Chris Dumez  <cdumez@apple.com>
     2
     3        Spew: Unhandled web process message 'VisitedLinkTableController:VisitedLinkStateChanged'
     4        https://bugs.webkit.org/show_bug.cgi?id=194787
     5        <rdar://problem/48175520>
     6
     7        Reviewed by Geoffrey Garen.
     8
     9        The unhandled 'VisitedLinkTableController:VisitedLinkStateChanged' message logging happens
     10        when IPC is sent to a WebProcess which does not have a VisitedLinkTableController with the
     11        given identifier. VisitedLinkTableController are kept alive by the WebPage in the WebProcess
     12        side so this indicates that there is no WebPage using this VisitedLinkTableController anymore.
     13
     14        In the UIProcess side, our tracking of who is using which VisitedLinkStore was very poor.
     15        WebPageProxy objects would ask their process to register itself with the page's visitedLinkStore
     16        as soon as the WebPage object has been created on the WebProcess side. This part was fine.
     17        However, unregistration from the visitedLinkStores would only happen when either the
     18        visitedLinkStore would get destroyed or when the WebProcess would shutdown. This means that
     19        WebProcess could stay registered with a visitedLinkStore even after the page that was using it
     20        has been closed, which would lead to such logging.
     21
     22        To address the issue, the WebProcessProxy now keeps track for which pages are using which
     23        visitedLinkStore. When a visitedLinkStore is used by a page for the first time, the
     24        WebProcessProxy will register itself with the visitedLinkStore. Similarly, when the last page
     25        using a given visitedLinkStore is closed, the process unregisters itself from the
     26        visitedLinkStore, thus avoiding the bug.
     27
     28        I also simplified a lot the logic for having a page telling the WebProcessProxy it started
     29        using a visitedLinkStore. Previously, it would have to wait until the process is done launching
     30        before notifying the WebProcessProxy. Now, the WebPageProxy merely tells the WebProcessProxy
     31        that it is starting to use a visitedLinkStore as soon as it sent the CreateWebPage IPC to the
     32        WebProcess (no matter if the process is still launching or not). At this point, the
     33        WebProcessProxy registers the page as a user of the visitedLinkStore and takes care of waiting
     34        until it is done launching before registering itself with the visitedLinkStore.
     35
     36        * UIProcess/ProvisionalPageProxy.cpp:
     37        (WebKit::ProvisionalPageProxy::~ProvisionalPageProxy):
     38        (WebKit::ProvisionalPageProxy::initializeWebPage):
     39        (WebKit::ProvisionalPageProxy::processDidFinishLaunching): Deleted.
     40        (WebKit::ProvisionalPageProxy::finishInitializingWebPageAfterProcessLaunch): Deleted.
     41        * UIProcess/ProvisionalPageProxy.h:
     42        * UIProcess/VisitedLinkStore.cpp:
     43        (WebKit::VisitedLinkStore::~VisitedLinkStore):
     44        (WebKit::VisitedLinkStore::addProcess):
     45        * UIProcess/WebPageProxy.cpp:
     46        (WebKit::WebPageProxy::finishAttachingToWebProcess):
     47        (WebKit::WebPageProxy::initializeWebPage):
     48        (WebKit::WebPageProxy::resetStateAfterProcessExited):
     49        (WebKit::WebPageProxy::finishInitializingWebPageAfterProcessLaunch): Deleted.
     50        (WebKit::WebPageProxy::processDidFinishLaunching): Deleted.
     51        * UIProcess/WebPageProxy.h:
     52        * UIProcess/WebProcessProxy.cpp:
     53        (WebKit::WebProcessProxy::shutDown):
     54        (WebKit::WebProcessProxy::removeWebPage):
     55        (WebKit::WebProcessProxy::addVisitedLinkStoreUser):
     56        (WebKit::WebProcessProxy::removeVisitedLinkStoreUser):
     57        (WebKit::WebProcessProxy::addWebUserContentControllerProxy):
     58        (WebKit::WebProcessProxy::didFinishLaunching):
     59        (WebKit::WebProcessProxy::addVisitedLinkStore): Deleted.
     60        (WebKit::WebProcessProxy::didDestroyVisitedLinkStore): Deleted.
     61        * UIProcess/WebProcessProxy.h:
     62
    1632019-03-19  Alex Christensen  <achristensen@webkit.org>
    264

  • trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp

    r242371 r243142  
    100100    m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID());
    101101    m_process->send(Messages::WebPage::Close(), m_page.pageID());
     102    m_process->removeVisitedLinkStoreUser(m_page.visitedLinkStore(), m_page.pageID());
    102103
    103104    RunLoop::main().dispatch([process = m_process.copyRef()] {
    … …  
    137138}
    138139
    139 void ProvisionalPageProxy::processDidFinishLaunching()
    140 {
    141     RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "processDidFinishLaunching: pageID = %" PRIu64, m_page.pageID());
    142     finishInitializingWebPageAfterProcessLaunch();
    143 }
    144 
    145 void ProvisionalPageProxy::finishInitializingWebPageAfterProcessLaunch()
    146 {
    147     ASSERT(m_process->state() == WebProcessProxy::State::Running);
    148 
    149     // FIXME: The WebPageProxy delays adding the visited link store until after the process has launched
    150     // so the ProvisionalPageProxy does the same. However, do we really need to?
    151     m_process->addVisitedLinkStore(m_page.visitedLinkStore());
    152 }
    153 
    154140void ProvisionalPageProxy::initializeWebPage()
    155141{
    … …  
    159145    parameters.isProcessSwap = true;
    160146    m_process->send(Messages::WebProcess::CreateWebPage(m_page.pageID(), parameters), 0);
    161 
    162     if (m_process->state() == WebProcessProxy::State::Running)
    163         finishInitializingWebPageAfterProcessLaunch();
     147    m_process->addVisitedLinkStoreUser(m_page.visitedLinkStore(), m_page.pageID());
    164148}
    165149

  • trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.h

    r242406 r243142  
    8080    void cancel();
    8181
    82     void processDidFinishLaunching();
    8382    void processDidTerminate();
    8483    void connectionWillOpen(IPC::Connection&);
    … …  
    116115
    117116    void initializeWebPage();
    118     void finishInitializingWebPageAfterProcessLaunch();
    119117
    120118    WebPageProxy& m_page;

  • trunk/Source/WebKit/UIProcess/VisitedLinkStore.cpp

    r235265 r243142  
    4343VisitedLinkStore::~VisitedLinkStore()
    4444{
    45     for (WebProcessProxy* process : m_processes) {
    46         process->removeMessageReceiver(Messages::VisitedLinkStore::messageReceiverName(), identifier());
    47         process->didDestroyVisitedLinkStore(*this);
    48     }
     45    RELEASE_ASSERT(m_processes.isEmpty());
    4946}
    5047
    … …  
    5754{
    5855    ASSERT(process.state() == WebProcessProxy::State::Running);
     56    ASSERT(!m_processes.contains(&process));
    5957
    6058    if (!m_processes.add(&process).isNewEntry)

  • trunk/Source/WebKit/UIProcess/WebPageProxy.cpp

    r243110 r243142  
    820820        if (isProcessSwap != IsProcessSwap::Yes)
    821821            m_webProcessLifetimeTracker.webPageEnteringWebProcess(m_process);
    822         processDidFinishLaunching();
    823822    }
    824823
    … …  
    959958    process().send(Messages::WebProcess::CreateWebPage(m_pageID, creationParameters(m_process, *m_drawingArea)), 0);
    960959
    961     m_needsToFinishInitializingWebPageAfterProcessLaunch = true;
    962     finishInitializingWebPageAfterProcessLaunch();
    963 }
    964 
    965 void WebPageProxy::finishInitializingWebPageAfterProcessLaunch()
    966 {
    967     if (!m_needsToFinishInitializingWebPageAfterProcessLaunch)
    968         return;
    969     if (m_process->state() != WebProcessProxy::State::Running)
    970         return;
    971 
    972     m_needsToFinishInitializingWebPageAfterProcessLaunch = false;
    973     m_process->addVisitedLinkStore(m_visitedLinkStore);
     960    m_process->addVisitedLinkStoreUser(visitedLinkStore(), m_pageID);
    974961}
    975962
    … …  
    51145101}
    51155102
    5116 void WebPageProxy::processDidFinishLaunching()
    5117 {
    5118     ASSERT(m_process->state() == WebProcessProxy::State::Running);
    5119     finishInitializingWebPageAfterProcessLaunch();
    5120 }
    5121 
    51225103#if ENABLE(NETSCAPE_PLUGIN_API)
    51235104void WebPageProxy::unavailablePluginButtonClicked(uint32_t opaquePluginUnavailabilityReason, const String& mimeType, const String& pluginURLString, const String& pluginspageAttributeURLString, const String& frameURLString, const String& pageURLString)
    … …  
    69246905    m_hasRunningProcess = false;
    69256906    m_isPageSuspended = false;
    6926 
    6927     m_needsToFinishInitializingWebPageAfterProcessLaunch = false;
    69286907
    69296908    m_editorState = EditorState();

  • trunk/Source/WebKit/UIProcess/WebPageProxy.h

    r243110 r243142  
    12341234    void webProcessWillShutDown();
    12351235
    1236     void processDidFinishLaunching();
    1237 
    12381236    void didSaveToPageCache();
    12391237       
    … …  
    19751973    void didResignInputElementStrongPasswordAppearance(const UserData&);
    19761974
    1977     void finishInitializingWebPageAfterProcessLaunch();
    1978 
    19791975    void handleMessage(IPC::Connection&, const String& messageName, const UserData& messageBody);
    19801976    void handleSynchronousMessage(IPC::Connection&, const String& messageName, const UserData& messageBody, UserData& returnUserData);
    … …  
    22262222    bool m_canRunModal { false };
    22272223
    2228     bool m_needsToFinishInitializingWebPageAfterProcessLaunch { false };
    2229 
    22302224    bool m_isInPrintingMode { false };
    22312225    bool m_isPerformingDOMPrintOperation { false };

  • trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp

    r243094 r243142  
    299299    m_frameMap.clear();
    300300
    301     for (auto* visitedLinkStore : m_visitedLinkStores)
     301    for (auto* visitedLinkStore : m_visitedLinkStoresWithUsers.keys())
    302302        visitedLinkStore->removeProcess(*this);
    303     m_visitedLinkStores.clear();
     303    m_visitedLinkStoresWithUsers.clear();
    304304
    305305    for (auto* webUserContentControllerProxy : m_webUserContentControllerProxies)
    … …  
    393393        m_processPool->pageEndUsingWebsiteDataStore(webPage.pageID(), webPage.websiteDataStore());
    394394
     395    removeVisitedLinkStoreUser(webPage.visitedLinkStore(), webPage.pageID());
     396
    395397    updateBackgroundResponsivenessTimer();
    396398
    … …  
    398400}
    399401
    400 void WebProcessProxy::addVisitedLinkStore(VisitedLinkStore& store)
    401 {
    402     m_visitedLinkStores.add(&store);
    403     store.addProcess(*this);
     402void WebProcessProxy::addVisitedLinkStoreUser(VisitedLinkStore& visitedLinkStore, uint64_t pageID)
     403{
     404    auto users = m_visitedLinkStoresWithUsers.ensure(&visitedLinkStore, [] {
     405        return HashSet<uint64_t> { };
     406    }).iterator->value;
     407
     408    ASSERT(!users.contains(pageID));
     409    users.add(pageID);
     410
     411    if (users.size() == 1 && state() == State::Running)
     412        visitedLinkStore.addProcess(*this);
     413}
     414
     415void WebProcessProxy::removeVisitedLinkStoreUser(VisitedLinkStore& visitedLinkStore, uint64_t pageID)
     416{
     417    auto it = m_visitedLinkStoresWithUsers.find(&visitedLinkStore);
     418    if (it == m_visitedLinkStoresWithUsers.end())
     419        return;
     420
     421    auto& users = it->value;
     422    users.remove(pageID);
     423    if (users.isEmpty()) {
     424        m_visitedLinkStoresWithUsers.remove(it);
     425        visitedLinkStore.removeProcess(*this);
     426    }
    404427}
    405428
    … …  
    408431    m_webUserContentControllerProxies.add(&proxy);
    409432    proxy.addProcess(*this, parameters);
    410 }
    411 
    412 void WebProcessProxy::didDestroyVisitedLinkStore(VisitedLinkStore& store)
    413 {
    414     ASSERT(m_visitedLinkStores.contains(&store));
    415     m_visitedLinkStores.remove(&store);
    416433}
    417434
    … …  
    740757    }
    741758
    742     for (WebPageProxy* page : m_pageMap.values()) {
    743         ASSERT(this == &page->process());
    744         page->processDidFinishLaunching();
    745     }
    746 
    747     for (auto* provisionalPage : m_provisionalPages) {
    748         ASSERT(this == &provisionalPage->process());
    749         provisionalPage->processDidFinishLaunching();
    750     }
    751 
    752 
    753759    RELEASE_ASSERT(!m_webConnection);
    754760    m_webConnection = WebConnectionToWebProcess::create(this);
    755761
    756762    m_processPool->processDidFinishLaunching(this);
     763
     764    for (auto* visitedLinkStore : m_visitedLinkStoresWithUsers.keys())
     765        visitedLinkStore->addProcess(*this);
    757766
    758767#if PLATFORM(IOS_FAMILY)

  • trunk/Source/WebKit/UIProcess/WebProcessProxy.h

    r243094 r243142  
    147147    virtual bool isServiceWorkerProcess() const { return false; }
    148148
    149     void addVisitedLinkStore(VisitedLinkStore&);
     149    void didCreateWebPageInProcess(uint64_t pageID);
     150
     151    void addVisitedLinkStoreUser(VisitedLinkStore&, uint64_t pageID);
     152    void removeVisitedLinkStoreUser(VisitedLinkStore&, uint64_t pageID);
     153
    150154    void addWebUserContentControllerProxy(WebUserContentControllerProxy&, WebPageCreationParameters&);
    151     void didDestroyVisitedLinkStore(VisitedLinkStore&);
    152155    void didDestroyWebUserContentControllerProxy(WebUserContentControllerProxy&);
    153156
    … …  
    429432    UserInitiatedActionMap m_userInitiatedActionMap;
    430433
    431     HashSet<VisitedLinkStore*> m_visitedLinkStores;
     434    HashMap<VisitedLinkStore*, HashSet<uint64_t/* pageID */>> m_visitedLinkStoresWithUsers;
    432435    HashSet<WebUserContentControllerProxy*> m_webUserContentControllerProxies;
    433436
Note: See TracChangeset for help on using the changeset viewer.