Bug 196307 – [ Mac WK2 iOS Sim] Layout Test http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html is a flaky failure

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Shawn Roberts]

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

[Radar WebKit Bug Importer]

<rdar://problem/49345360>

[Shawn Roberts]

Marked as flaky in https://trac.webkit.org/changeset/243554/webkit while waiting for a fix.

[Sihui Liu]

Created attachment 369498 [details]
Patch

[Sihui Liu]

Created attachment 369499 [details]
Patch

[Geoffrey Garen]

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.

[Sihui Liu]

Created attachment 369559 [details]
Patch

[Sihui Liu]

(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.

[Alex Christensen]

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.

[Sihui Liu]

Created attachment 370256 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 370256 [details]
Patch for landing

Clearing flags on attachment: 370256

Committed r245517: <https://trac.webkit.org/changeset/245517>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

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.

[Chris Dumez]

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.

[Ryosuke Niwa]

(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?

[Ryan Haddad]

(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

[Ryosuke Niwa]

(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!