Changeset 251672 in webkit

Timestamp:

Oct 28, 2019 2:38:26 PM (5 years ago)

Author:

Kate Cheney

Message:

Layout Test http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html is a flaky failure
​https://bugs.webkit.org/show_bug.cgi?id=203491
<rdar://problem/56674176>

Reviewed by Chris Dumez.

No new tests, this change is tested by the existing resourceLoadStatistics
tests.

This test started flaking when a new memory store was being created
between tests to maintain consistency. The call to grandfatherExistingWebsiteData
from populateMemoryStoreFromDisk in the persistent storage was
async, causing a race condition that led to occasional failures.
Adding a completion handler and changing the callsite of
populateMemoryStoreFromDisk should fix this problem.

• NetworkProcess/Classifier/ResourceLoadStatisticsPersistentStorage.cpp:

(WebKit::ResourceLoadStatisticsPersistentStorage::ResourceLoadStatisticsPersistentStorage):

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

(WebKit::WebResourceLoadStatisticsStore::WebResourceLoadStatisticsStore):
The persistent store in the databaseEnabled case was never being used
and is unnecessary.

(WebKit::WebResourceLoadStatisticsStore::populateMemoryStoreFromDisk):
Since persistent storage only exists when using the memory store,
populateMemoryStoreFromDisk should check if
m_persistentStorage has been initialized.

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

(WebKit::NetworkProcess::setUseITPDatabase):

• NetworkProcess/NetworkSession.cpp:

(WebKit::NetworkSession::setResourceLoadStatisticsEnabled):
(WebKit::NetworkSession::recreateResourceLoadStatisticStore):

• NetworkProcess/NetworkSession.h:

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• NetworkProcess/Classifier/ResourceLoadStatisticsPersistentStorage.cpp (modified) (1 diff)
• NetworkProcess/Classifier/ResourceLoadStatisticsPersistentStorage.h (modified) (2 diffs)
• NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp (modified) (2 diffs)
• NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h (modified) (1 diff)
• NetworkProcess/NetworkProcess.cpp (modified) (1 diff)
• NetworkProcess/NetworkSession.cpp (modified) (2 diffs)
• NetworkProcess/NetworkSession.h (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-10-28  Kate Cheney  <katherine_cheney@apple.com>
+
+        Layout Test http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html is a flaky failure
+        https://bugs.webkit.org/show_bug.cgi?id=203491
+        <rdar://problem/56674176>
+
+        Reviewed by Chris Dumez.
+
+        No new tests, this change is tested by the existing resourceLoadStatistics
+        tests.
+
+        This test started flaking when a new memory store was being created
+        between tests to maintain consistency. The call to grandfatherExistingWebsiteData
+        from populateMemoryStoreFromDisk in the persistent storage was
+        async, causing a race condition that led to occasional failures.
+        Adding a completion handler and changing the callsite of
+        populateMemoryStoreFromDisk should fix this problem.
+
+        * NetworkProcess/Classifier/ResourceLoadStatisticsPersistentStorage.cpp:
+        (WebKit::ResourceLoadStatisticsPersistentStorage::ResourceLoadStatisticsPersistentStorage):
+        * NetworkProcess/Classifier/ResourceLoadStatisticsPersistentStorage.h:
+        * NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:
+        (WebKit::WebResourceLoadStatisticsStore::WebResourceLoadStatisticsStore):
+        The persistent store in the databaseEnabled case was never being used
+        and is unnecessary.
+
+        (WebKit::WebResourceLoadStatisticsStore::populateMemoryStoreFromDisk):
+        Since persistent storage only exists when using the memory store,
+        populateMemoryStoreFromDisk should check if
+        m_persistentStorage has been initialized.
+
+        * NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:
+        * NetworkProcess/NetworkProcess.cpp:
+        (WebKit::NetworkProcess::setUseITPDatabase):
+        * NetworkProcess/NetworkSession.cpp:
+        (WebKit::NetworkSession::setResourceLoadStatisticsEnabled):
+        (WebKit::NetworkSession::recreateResourceLoadStatisticStore):
+        * NetworkProcess/NetworkSession.h:
+
 2019-10-28  Wenson Hsieh  <wenson_hsieh@apple.com>
 

trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsPersistentStorage.cpp

@@ -65 +65 @@
     m_memoryStore.setPersistentStorage(*this);
 
-    populateMemoryStoreFromDisk();
     startMonitoringDisk();
 }

trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsPersistentStorage.h

@@ -55 +55 @@
     enum class ForceImmediateWrite { No, Yes, };
     void scheduleOrWriteMemoryStore(ForceImmediateWrite);
+    void populateMemoryStoreFromDisk();
 
 private:
@@ -65 +66 @@
 
     void writeMemoryStoreToDisk();
-    void populateMemoryStoreFromDisk();
     void excludeFromBackup() const;
     void refreshMemoryStoreFromDisk();

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

@@ -163 +163 @@
 
             auto memoryStore = makeUnique<ResourceLoadStatisticsMemoryStore>(*this, m_statisticsQueue, shouldIncludeLocalhost);
-            auto persistentStore = makeUnique<ResourceLoadStatisticsPersistentStorage>(*memoryStore, m_statisticsQueue, resourceLoadStatisticsDirectory);
-
             downcast<ResourceLoadStatisticsDatabaseStore>(*m_statisticsStore.get()).populateFromMemoryStore(*memoryStore);
         } else {
@@ -222 +220 @@
     });
     semaphore.wait();
+}
+
+void WebResourceLoadStatisticsStore::populateMemoryStoreFromDisk(CompletionHandler<void()>&& completionHandler)
+{
+    ASSERT(RunLoop::isMain());
+    
+    postTask([this, completionHandler = WTFMove(completionHandler)]() mutable {
+        if (m_persistentStorage)
+            m_persistentStorage->populateMemoryStoreFromDisk();
+
+        postTaskReply(WTFMove(completionHandler));
+    });
 }
 

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

@@ -107 +107 @@
     WTF::WorkQueue& statisticsQueue() { return m_statisticsQueue.get(); }
 
+    void populateMemoryStoreFromDisk(CompletionHandler<void()>&&);
     void setNotifyPagesWhenDataRecordsWereScanned(bool);
     void setNotifyPagesWhenTelemetryWasCaptured(bool, CompletionHandler<void()>&&);

trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp

@@ -666 +666 @@
         if (m_isITPDatabaseEnabled != value) {
             m_isITPDatabaseEnabled = value;
-            networkSession->recreateResourceLoadStatisticStore();
-        }
-        completionHandler();
+            networkSession->recreateResourceLoadStatisticStore(WTFMove(completionHandler));
+        } else
+            completionHandler();
     } else {
         ASSERT_NOT_REACHED();

trunk/Source/WebKit/NetworkProcess/NetworkSession.cpp

@@ -165 +165 @@
 
     m_resourceLoadStatistics = WebResourceLoadStatisticsStore::create(*this, m_resourceLoadStatisticsDirectory, m_shouldIncludeLocalhostInResourceLoadStatistics);
+    m_resourceLoadStatistics->populateMemoryStoreFromDisk([] { });
 
     if (m_enableResourceLoadStatisticsDebugMode == EnableResourceLoadStatisticsDebugMode::Yes)
@@ -174 +175 @@
 }
 
-void NetworkSession::recreateResourceLoadStatisticStore()
+void NetworkSession::recreateResourceLoadStatisticStore(CompletionHandler<void()>&& completionHandler)
 {
     destroyResourceLoadStatistics();
     m_resourceLoadStatistics = WebResourceLoadStatisticsStore::create(*this, m_resourceLoadStatisticsDirectory, m_shouldIncludeLocalhostInResourceLoadStatistics);
+    m_resourceLoadStatistics->populateMemoryStoreFromDisk(WTFMove(completionHandler));
 }
 

trunk/Source/WebKit/NetworkProcess/NetworkSession.h

@@ -85 +85 @@
     WebResourceLoadStatisticsStore* resourceLoadStatistics() const { return m_resourceLoadStatistics.get(); }
     void setResourceLoadStatisticsEnabled(bool);
-    void recreateResourceLoadStatisticStore();
+    void recreateResourceLoadStatisticStore(CompletionHandler<void()>&&);
     bool isResourceLoadStatisticsEnabled() const;
     void notifyResourceLoadStatisticsProcessed();