The following layout test is failing on Mac WK2 Release and Debug, iOS Simulator Release and Debug http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html Probable cause: Test has been a flaky failure since it was created. However after the changes in https://trac.webkit.org/changeset/243348/webkit it became a more consistent failure. On the bots as well as local testing, test will fail more consistently in full parallel or in debug mode. It will fail with the same diff in full parallel or with 1 child process. Reproduced with: run-webkit-tests http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html --iterations 500 -f --ios-simulator --exit-after-n-failures=5 --no-retry run-webkit-tests http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html --iterations 500 --child-process 1 --ios-simulator --debug --exit-after-n-failures=5 --no-retry Flakiness Dashboard: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FresourceLoadStatistics%2Fwebsite-data-removal-for-site-navigated-to-with-link-decoration.html Diff: --- /Volumes/Data/slave/ios-simulator-12-release-tests-wk2/build/layout-test-results/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-expected.txt +++ /Volumes/Data/slave/ios-simulator-12-release-tests-wk2/build/layout-test-results/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-actual.txt @@ -10,7 +10,7 @@ After deletion: Client-side cookie exists. After deletion: Regular server-side cookie exists. -After deletion: IDB entry does not exist. +After deletion: IDB entry does exist. Resource load statistics: @@ -32,4 +32,4 @@ localhost gotLinkDecorationFromPrevalentResource: No isPrevalentResource: No isVeryPrevalentResource: No - dataRecordsRemoved: 1 + dataRecordsRemoved: 2
<rdar://problem/49345360>
Marked as flaky in https://trac.webkit.org/changeset/243554/webkit while waiting for a fix.
Created attachment 369498 [details] Patch
Created attachment 369499 [details] Patch
Comment on attachment 369499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369499&action=review > LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html:157 > + setTimeout(checkWebsiteDataAndContinue, 1000); It would be nice to avoid a fixed timeout, since those are sometimes flaky too. One way to do that is to refactor checkWebsiteDataAndContinue to test for success, and reschedule itself upon failure, up to some limit like 10s. That way, passes can be fast, and only failures are slow.
Created attachment 369559 [details] Patch
(In reply to Geoffrey Garen from comment #5) > Comment on attachment 369499 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=369499&action=review > > > LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html:157 > > + setTimeout(checkWebsiteDataAndContinue, 1000); > > It would be nice to avoid a fixed timeout, since those are sometimes flaky > too. > > One way to do that is to refactor checkWebsiteDataAndContinue to test for > success, and reschedule itself upon failure, up to some limit like 10s. That > way, passes can be fast, and only failures are slow. It turns out IDB is not the last data type gets deleted. Even if IDB files are deleted, resource statistics may have not been updated, so we cannot use missing IDB database to decide whether the deletion of website data finishes.
Comment on attachment 369559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369559&action=review > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:618 > + m_dumpResourceLoadStatisticsFunction = WTFMove(completionHandler); Let's call this m_dumpResourceLoadStatisticsCompletionHandler, and assert that it's null before this sets it.
Created attachment 370256 [details] Patch for landing
Comment on attachment 370256 [details] Patch for landing Clearing flags on attachment: 370256 Committed r245517: <https://trac.webkit.org/changeset/245517>
All reviewed patches have been landed. Closing bug.
Comment on attachment 370256 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=370256&action=review > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:628 > + ASSERT(!RunLoop::isMain()); This is not OK, the whole point of our abstraction is that WebResourceLoadStatisticsStore is always constructed / used / destroyed on the *main* thread. WebResourceLoadStatisticsStore::m_statisticsStore however is always constructed / used / destroyed on the background queue.
Comment on attachment 370256 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=370256&action=review >> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:628 >> + ASSERT(!RunLoop::isMain()); > > This is not OK, the whole point of our abstraction is that WebResourceLoadStatisticsStore is always constructed / used / destroyed on the *main* thread. > WebResourceLoadStatisticsStore::m_statisticsStore however is always constructed / used / destroyed on the background queue. I will follow-up.
(In reply to Chris Dumez from comment #13) > Comment on attachment 370256 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370256&action=review > > >> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:628 > >> + ASSERT(!RunLoop::isMain()); > > > > This is not OK, the whole point of our abstraction is that WebResourceLoadStatisticsStore is always constructed / used / destroyed on the *main* thread. > > WebResourceLoadStatisticsStore::m_statisticsStore however is always constructed / used / destroyed on the background queue. > > I will follow-up. Where is the follow up happening?
(In reply to Ryosuke Niwa from comment #14) > (In reply to Chris Dumez from comment #13) > > I will follow-up. > > Where is the follow up happening? https://bugs.webkit.org/show_bug.cgi?id=199468
(In reply to Ryan Haddad from comment #15) > (In reply to Ryosuke Niwa from comment #14) > > (In reply to Chris Dumez from comment #13) > > > I will follow-up. > > > > Where is the follow up happening? > https://bugs.webkit.org/show_bug.cgi?id=199468 Thanks!