Changeset 260791 in webkit


Ignore:
Timestamp:
Apr 27, 2020, 3:33:33 PM (5 years ago)
Author:
achristensen@apple.com
Message:

Stop waiting for a BinarySemaphore on the main thread in the NetworkProcess
​https://bugs.webkit.org/show_bug.cgi?id=211080
<rdar://problem/62377357>

Reviewed by Darin Adler and Chris Dumez.

There was an out-of-date comment suggesting we needed to use a semaphore, but we've since added these in the destructor:
RELEASE_ASSERT(!m_statisticsStore);
RELEASE_ASSERT(!m_persistentStorage);
This indicates that flushAndDestroyPersistentStore is called before the destructor, at which time it is safe to add a reference to keep it alive.
WebResourceLoadStatisticsStore is also marked as WTF::DestructionThread::Main so this should do everything we need.
We also flush these databases to disk before closing like we did cookies.

In order to keep tests working as they are, I needed to make recreateResourceLoadStatisticStore have a CompletionHandler and have all
WebResourceLoadStatisticsStores share the same queue to serialize background tasks between WebResourceLoadStatisticsStores with and without database stores
sequentially to avoid opening a SQLiteDatabase before the previous WebResourceLoadStatisticsStore had closed it.

  • NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:

(WebKit::sharedStatisticsQueue):
(WebKit::WebResourceLoadStatisticsStore::WebResourceLoadStatisticsStore):
(WebKit::WebResourceLoadStatisticsStore::didDestroyNetworkSession):
(WebKit::WebResourceLoadStatisticsStore::flushAndDestroyPersistentStore):
(WebKit::WebResourceLoadStatisticsStore::applicationWillTerminate): Deleted.

  • NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:
  • NetworkProcess/NetworkProcess.cpp:

(WebKit::NetworkProcess::didClose):

  • NetworkProcess/NetworkSession.cpp:

(WebKit::NetworkSession::~NetworkSession):
(WebKit::NetworkSession::destroyResourceLoadStatistics):
(WebKit::NetworkSession::setResourceLoadStatisticsEnabled):
(WebKit::NetworkSession::recreateResourceLoadStatisticStore):

  • NetworkProcess/NetworkSession.h:
Location:
trunk/Source/WebKit
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r260790 r260791  
     12020-04-27  Alex Christensen  <achristensen@webkit.org>
     2
     3        Stop waiting for a BinarySemaphore on the main thread in the NetworkProcess
     4        https://bugs.webkit.org/show_bug.cgi?id=211080
     5        <rdar://problem/62377357>
     6
     7        Reviewed by Darin Adler and Chris Dumez.
     8
     9        There was an out-of-date comment suggesting we needed to use a semaphore, but we've since added these in the destructor:
     10        RELEASE_ASSERT(!m_statisticsStore);
     11        RELEASE_ASSERT(!m_persistentStorage);
     12        This indicates that flushAndDestroyPersistentStore is called before the destructor, at which time it is safe to add a reference to keep it alive.
     13        WebResourceLoadStatisticsStore is also marked as WTF::DestructionThread::Main so this should do everything we need.
     14        We also flush these databases to disk before closing like we did cookies.
     15
     16        In order to keep tests working as they are, I needed to make recreateResourceLoadStatisticStore have a CompletionHandler and have all
     17        WebResourceLoadStatisticsStores share the same queue to serialize background tasks between WebResourceLoadStatisticsStores with and without database stores
     18        sequentially to avoid opening a SQLiteDatabase before the previous WebResourceLoadStatisticsStore had closed it.
     19
     20        * NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:
     21        (WebKit::sharedStatisticsQueue):
     22        (WebKit::WebResourceLoadStatisticsStore::WebResourceLoadStatisticsStore):
     23        (WebKit::WebResourceLoadStatisticsStore::didDestroyNetworkSession):
     24        (WebKit::WebResourceLoadStatisticsStore::flushAndDestroyPersistentStore):
     25        (WebKit::WebResourceLoadStatisticsStore::applicationWillTerminate): Deleted.
     26        * NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:
     27        * NetworkProcess/NetworkProcess.cpp:
     28        (WebKit::NetworkProcess::didClose):
     29        * NetworkProcess/NetworkSession.cpp:
     30        (WebKit::NetworkSession::~NetworkSession):
     31        (WebKit::NetworkSession::destroyResourceLoadStatistics):
     32        (WebKit::NetworkSession::setResourceLoadStatisticsEnabled):
     33        (WebKit::NetworkSession::recreateResourceLoadStatisticStore):
     34        * NetworkProcess/NetworkSession.h:
     35
    1362020-04-27  Alex Christensen  <achristensen@webkit.org>
    237

  • trunk/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp

    r260668 r260791  
    165165}
    166166
     167static Ref<WorkQueue> sharedStatisticsQueue()
     168{
     169    static NeverDestroyed<Ref<WorkQueue>> queue(WorkQueue::create("WebResourceLoadStatisticsStore Process Data Queue", WorkQueue::Type::Serial, WorkQueue::QOS::Utility));
     170    return queue.get().copyRef();
     171}
     172
    167173WebResourceLoadStatisticsStore::WebResourceLoadStatisticsStore(NetworkSession& networkSession, const String& resourceLoadStatisticsDirectory, ShouldIncludeLocalhost shouldIncludeLocalhost, ResourceLoadStatistics::IsEphemeral isEphemeral)
    168174    : m_networkSession(makeWeakPtr(networkSession))
    169     , m_statisticsQueue(WorkQueue::create("WebResourceLoadStatisticsStore Process Data Queue", WorkQueue::Type::Serial, WorkQueue::QOS::Utility))
     175    , m_statisticsQueue(sharedStatisticsQueue())
    170176    , m_dailyTasksTimer(RunLoop::main(), this, &WebResourceLoadStatisticsStore::performDailyTasks)
    171177    , m_isEphemeral(isEphemeral)
    … …  
    210216}
    211217
    212 void WebResourceLoadStatisticsStore::didDestroyNetworkSession()
     218void WebResourceLoadStatisticsStore::didDestroyNetworkSession(CompletionHandler<void()>&& completionHandler)
    213219{
    214220    ASSERT(RunLoop::isMain());
    215221
    216222    m_networkSession = nullptr;
    217     flushAndDestroyPersistentStore();
     223    flushAndDestroyPersistentStore(WTFMove(completionHandler));
    218224}
    219225
    … …  
    235241}
    236242
    237 void WebResourceLoadStatisticsStore::flushAndDestroyPersistentStore()
     243void WebResourceLoadStatisticsStore::flushAndDestroyPersistentStore(CompletionHandler<void()>&& completionHandler)
    238244{
    239245    RELEASE_ASSERT(RunLoop::isMain());
    240246
    241     // Make sure we destroy the persistent store on the background queue and wait for it to die
    242     // synchronously since it has a C++ reference to us. Blocking nature of this task allows us
    243     // to not maintain a WebResourceLoadStatisticsStore reference for the duration of dispatch,
    244     // avoiding double-deletion issues when this is invoked from the destructor.
    245     BinarySemaphore semaphore;
    246     m_statisticsQueue->dispatch([&semaphore, this] {
     247    // Make sure we destroy the persistent store on the background queue and stay alive until it
     248    // is destroyed because it has a C++ reference to us.
     249    m_statisticsQueue->dispatch([this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)] () mutable {
    247250        m_persistentStorage = nullptr;
    248251        m_statisticsStore = nullptr;
    249         semaphore.signal();
    250     });
    251     semaphore.wait();
     252        RunLoop::main().dispatch(WTFMove(completionHandler));
     253    });
    252254}
    253255
    … …  
    679681}
    680682
    681 void WebResourceLoadStatisticsStore::applicationWillTerminate()
    682 {
    683     ASSERT(RunLoop::isMain());
    684     if (!isEphemeral())
    685         flushAndDestroyPersistentStore();
    686 }
    687 
    688683void WebResourceLoadStatisticsStore::performDailyTasks()
    689684{

  • trunk/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h

    r260668 r260791  
    199199};
    200200
    201     void didDestroyNetworkSession();
     201    void didDestroyNetworkSession(CompletionHandler<void()>&&);
    202202
    203203    static const OptionSet<WebsiteDataType>& monitoredDataTypes();
    … …  
    212212
    213213    void grantStorageAccess(const SubFrameDomain&, const TopFrameDomain&, WebCore::FrameIdentifier, WebCore::PageIdentifier, StorageAccessPromptWasShown, CompletionHandler<void(StorageAccessWasGranted, StorageAccessPromptWasShown)>&&);
    214 
    215     void applicationWillTerminate();
    216214
    217215    void logFrameNavigation(const NavigatedToDomain&, const TopFrameDomain&, const NavigatedFromDomain&, bool isRedirect, bool isMainFrame, Seconds delayAfterMainFrameDocumentLoad, bool wasPotentiallyInitiatedByUser);
    … …  
    318316    StorageAccessStatus storageAccessStatus(const String& subFramePrimaryDomain, const String& topFramePrimaryDomain);
    319317
    320     void flushAndDestroyPersistentStore();
     318    void flushAndDestroyPersistentStore(CompletionHandler<void()>&&);
    321319
    322320    WeakPtr<NetworkSession> m_networkSession;

  • trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp

    r260668 r260791  
    260260    ASSERT(RunLoop::isMain());
    261261
    262     // Make sure we flush all cookies to disk before exiting.
    263     platformSyncAllCookies([this] {
     262    auto callbackAggregator = CallbackAggregator::create([this] {
     263        ASSERT(RunLoop::isMain());
    264264        stopRunLoop();
    265265    });
     266
     267    // Make sure we flush all cookies and resource load statistics to disk before exiting.
     268#if ENABLE(RESOURCE_LOAD_STATISTICS)
     269    forEachNetworkSession([&] (auto& networkSession) {
     270        networkSession.destroyResourceLoadStatistics([callbackAggregator = callbackAggregator.copyRef()] { });
     271    });
     272#endif
     273    platformSyncAllCookies([callbackAggregator = callbackAggregator.copyRef()] { });
    266274}
    267275

  • trunk/Source/WebKit/NetworkProcess/NetworkSession.cpp

    r260169 r260791  
    136136{
    137137#if ENABLE(RESOURCE_LOAD_STATISTICS)
    138     destroyResourceLoadStatistics();
    139 #endif
    140 }
    141 
    142 #if ENABLE(RESOURCE_LOAD_STATISTICS)
    143 void NetworkSession::destroyResourceLoadStatistics()
     138    destroyResourceLoadStatistics([] { });
     139#endif
     140}
     141
     142#if ENABLE(RESOURCE_LOAD_STATISTICS)
     143void NetworkSession::destroyResourceLoadStatistics(CompletionHandler<void()>&& completionHandler)
    144144{
    145145    if (!m_resourceLoadStatistics)
    146         return;
    147 
    148     m_resourceLoadStatistics->didDestroyNetworkSession();
     146        return completionHandler();
     147
     148    m_resourceLoadStatistics->didDestroyNetworkSession(WTFMove(completionHandler));
    149149    m_resourceLoadStatistics = nullptr;
    150150}
    … …  
    171171        storageSession->setResourceLoadStatisticsEnabled(enable);
    172172    if (!enable) {
    173         destroyResourceLoadStatistics();
     173        destroyResourceLoadStatistics([] { });
    174174        return;
    175175    }
    … …  
    192192void NetworkSession::recreateResourceLoadStatisticStore(CompletionHandler<void()>&& completionHandler)
    193193{
    194     destroyResourceLoadStatistics();
    195     m_resourceLoadStatistics = WebResourceLoadStatisticsStore::create(*this, m_resourceLoadStatisticsDirectory, m_shouldIncludeLocalhostInResourceLoadStatistics, (m_sessionID.isEphemeral() ? ResourceLoadStatistics::IsEphemeral::Yes : ResourceLoadStatistics::IsEphemeral::No));
    196     forwardResourceLoadStatisticsSettings();
    197     if (!m_sessionID.isEphemeral())
    198         m_resourceLoadStatistics->populateMemoryStoreFromDisk(WTFMove(completionHandler));
    199     else
    200         completionHandler();
     194    destroyResourceLoadStatistics([this, weakThis = makeWeakPtr(*this), completionHandler = WTFMove(completionHandler)] () mutable {
     195        if (!weakThis)
     196            return completionHandler();
     197        m_resourceLoadStatistics = WebResourceLoadStatisticsStore::create(*this, m_resourceLoadStatisticsDirectory, m_shouldIncludeLocalhostInResourceLoadStatistics, (m_sessionID.isEphemeral() ? ResourceLoadStatistics::IsEphemeral::Yes : ResourceLoadStatistics::IsEphemeral::No));
     198        forwardResourceLoadStatisticsSettings();
     199        if (!m_sessionID.isEphemeral())
     200            m_resourceLoadStatistics->populateMemoryStoreFromDisk(WTFMove(completionHandler));
     201        else
     202            completionHandler();
     203    });
    201204}
    202205

  • trunk/Source/WebKit/NetworkProcess/NetworkSession.h

    r260169 r260791  
    106106    void setThirdPartyCookieBlockingMode(WebCore::ThirdPartyCookieBlockingMode);
    107107    void setShouldEnbleSameSiteStrictEnforcement(WebCore::SameSiteStrictEnforcementEnabled);
     108    void destroyResourceLoadStatistics(CompletionHandler<void()>&&);
    108109#endif
    109110   
    … …  
    147148
    148149#if ENABLE(RESOURCE_LOAD_STATISTICS)
    149     void destroyResourceLoadStatistics();
    150150    void forwardResourceLoadStatisticsSettings();
    151151#endif
Note: See TracChangeset for help on using the changeset viewer.