Bug 207944

Summary:

(r256583) [ iOS ] http/tests/resourceLoadStatistics/prevalent-domains-per-page-database.html is a flaky timeout

Product:

WebKit

Reporter:

Truitt Savell <tsavell>

Component:

New Bugs

Assignee:

Kate Cheney <katherine_cheney>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, katherine_cheney, webkit-bot-watchers-bugzilla, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=207523

Attachments:

Description	Flags
Patch	none
Patch	none

Description Truitt Savell 2020-02-19 09:08:22 PST

http/tests/resourceLoadStatistics/prevalent-domains-per-page-database.html

This test was introduced in https://trac.webkit.org/changeset/256583/webkit

and is a flaky timeout sense introduction. there is also at least one crash in history. 

history:
https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2FresourceLoadStatistics%2Fprevalent-domains-per-page-database.html

Comment 1 Radar WebKit Bug Importer 2020-02-19 09:08:38 PST

<rdar://problem/59592361>

Comment 2 Kate Cheney 2020-02-20 08:22:22 PST

Created attachment 391289 [details]
Patch

Comment 3 Alexey Proskuryakov 2020-02-20 08:35:16 PST

Comment on attachment 391289 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391289&action=review

r=ews

> LayoutTests/http/tests/resourceLoadStatistics/prevalent-domains-per-page-database.html:20
> +                if (arrayOfDomains.length == 0) {
> +                    askForPrevalentResources();
> +                    return;

This is a tight loop, which is unfriendly to other tests running in parallel. Maybe do it on a 100 ms timer?

> LayoutTests/http/tests/resourceLoadStatistics/prevalent-domains-per-page-database.html:25
>                      testFailed("Domain was not successfully marked prevalent.");

Does this become dead code now, as we'll keep retrying until timeout? Maybe it's worth to limit retries to say 20 seconds, because timeout is not a very clear failure mode - we often assume that those are not real failures.

Comment 4 Kate Cheney 2020-02-20 08:40:00 PST

Comment on attachment 391289 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391289&action=review

>> LayoutTests/http/tests/resourceLoadStatistics/prevalent-domains-per-page-database.html:20
>> +                    return;
> 
> This is a tight loop, which is unfriendly to other tests running in parallel. Maybe do it on a 100 ms timer?

Sounds good, I'll add this.

>> LayoutTests/http/tests/resourceLoadStatistics/prevalent-domains-per-page-database.html:25
>>                      testFailed("Domain was not successfully marked prevalent.");
> 
> Does this become dead code now, as we'll keep retrying until timeout? Maybe it's worth to limit retries to say 20 seconds, because timeout is not a very clear failure mode - we often assume that those are not real failures.

Do you have an example of another test which does this? I've only seen retrying for a certain number of iterations, not based on time.

Comment 5 Alexey Proskuryakov 2020-02-20 10:09:55 PST

Yes, we have some tests with a setTimeout() for 20 seconds that prints a failure message and calls notifyDone(). We also have some that limit the number of iterations, but I don't know if 200 iterations 100 ms each are guaranteed to complete in a time that's close enough to 20 seconds. Probably not.

Comment 6 Kate Cheney 2020-02-20 10:59:56 PST

Created attachment 391310 [details]
Patch

Comment 7 Kate Cheney 2020-02-20 11:00:28 PST

Letting ews run.

Comment 8 Kate Cheney 2020-02-20 15:37:13 PST

This test uses UIHelper.activateAt(), which doesn't produce a user gesture that ITP reliably captures on iOS. This is a problem we’ve faced before with ITP tests, and our solution for now is to skip these tests on iOS.

Comment 9 Kate Cheney 2020-02-20 15:45:34 PST

Committed: https://trac.webkit.org/r257100