Changeset 252123 in webkit

Timestamp:

Nov 5, 2019 9:17:37 PM (4 years ago)

Author:

Kate Cheney

Message:

Layout test website-data-removal-for-site-navigated-to-with-link-decoration.html is a flaky failure
​https://bugs.webkit.org/show_bug.cgi?id=203706
Source/WebKit:

<rdar://problem/56801615>

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::populateMemoryStoreFromDisk):

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

(WebKit::WebResourceLoadStatisticsStore::populateMemoryStoreFromDisk):

LayoutTests:

<rdar://problem/56801615>

Reviewed by Chris Dumez.

Since the state is reset between tests, the call to
setUseITPDatabase(false) is redundant.

• http/tests/resourceLoadStatistics/log-cross-site-load-with-link-decoration.html:
• http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/http/tests/resourceLoadStatistics/log-cross-site-load-with-link-decoration.html (modified) (1 diff)
• LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html (modified) (1 diff)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsPersistentStorage.cpp (modified) (5 diffs)
• Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsPersistentStorage.h (modified) (1 diff)
• Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2019-11-05  Kate Cheney  <katherine_cheney@apple.com>
+
+        Layout test website-data-removal-for-site-navigated-to-with-link-decoration.html is a flaky failure
+        https://bugs.webkit.org/show_bug.cgi?id=203706
+        <rdar://problem/56801615> 
+
+        Reviewed by Chris Dumez. 
+
+        Since the state is reset between tests, the call to 
+        setUseITPDatabase(false) is redundant. 
+
+        * http/tests/resourceLoadStatistics/log-cross-site-load-with-link-decoration.html:
+        * http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html:
+
 2019-11-05  Ryosuke Niwa  <rniwa@webkit.org>
 

trunk/LayoutTests/http/tests/resourceLoadStatistics/log-cross-site-load-with-link-decoration.html

@@ -9 +9 @@
 <script>
     if (window.testRunner) {
-        testRunner.setUseITPDatabase(false);
         testRunner.waitUntilDone();
         testRunner.dumpAsText();

trunk/LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html

@@ -11 +11 @@
 <br>
 <script>
-    testRunner.setUseITPDatabase(false);
     testRunner.waitUntilDone();
     testRunner.dumpAsText();

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-11-05  Kate Cheney  <katherine_cheney@apple.com>
+
+        Layout test website-data-removal-for-site-navigated-to-with-link-decoration.html is a flaky failure
+        https://bugs.webkit.org/show_bug.cgi?id=203706
+        <rdar://problem/56801615>
+
+        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::populateMemoryStoreFromDisk):
+        * NetworkProcess/Classifier/ResourceLoadStatisticsPersistentStorage.h:
+        * NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:
+        (WebKit::WebResourceLoadStatisticsStore::populateMemoryStoreFromDisk):
+
 2019-11-05  John Wilander  <wilander@apple.com>
 

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

@@ -184 +184 @@
 }
 
-void ResourceLoadStatisticsPersistentStorage::populateMemoryStoreFromDisk()
+void ResourceLoadStatisticsPersistentStorage::populateMemoryStoreFromDisk(CompletionHandler<void()>&& completionHandler)
 {
     ASSERT(!RunLoop::isMain());
@@ -190 +190 @@
     String filePath = resourceLogFilePath();
     if (filePath.isEmpty() || !FileSystem::fileExists(filePath)) {
-        m_memoryStore.grandfatherExistingWebsiteData([]() { });
+        m_memoryStore.grandfatherExistingWebsiteData(WTFMove(completionHandler));
         monitorDirectoryForNewStatistics();
         return;
@@ -197 +197 @@
     if (!hasFileChangedSince(filePath, m_lastStatisticsFileSyncTime)) {
         // No need to grandfather in this case.
+        completionHandler();
         return;
     }
@@ -204 +205 @@
     auto decoder = createForFile(filePath);
     if (!decoder) {
-        m_memoryStore.grandfatherExistingWebsiteData([]() { });
+        m_memoryStore.grandfatherExistingWebsiteData(WTFMove(completionHandler));
         return;
     }
@@ -215 +216 @@
 
     m_memoryStore.logTestingEvent("PopulatedWithoutGrandfathering"_s);
+    completionHandler();
 }
 

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

@@ -55 +55 @@
     enum class ForceImmediateWrite { No, Yes, };
     void scheduleOrWriteMemoryStore(ForceImmediateWrite);
-    void populateMemoryStoreFromDisk();
+    void populateMemoryStoreFromDisk(CompletionHandler<void()>&&);
 
 private:

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

@@ -226 +226 @@
     ASSERT(RunLoop::isMain());
     
-    postTask([this, completionHandler = WTFMove(completionHandler)]() mutable {
+    postTask([this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)]() mutable {
         if (m_persistentStorage)
-            m_persistentStorage->populateMemoryStoreFromDisk();
-
-        postTaskReply(WTFMove(completionHandler));
+            m_persistentStorage->populateMemoryStoreFromDisk([protectedThis = WTFMove(protectedThis), completionHandler = WTFMove(completionHandler)]() mutable {
+                postTaskReply(WTFMove(completionHandler));
+            });
+        else
+            postTaskReply(WTFMove(completionHandler));
     });
 }