Bug 172730 - Make ResourceLoadStatistics testing more reliable
Summary: Make ResourceLoadStatistics testing more reliable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-05-30 14:03 PDT by Brent Fulgham
Modified: 2017-05-31 09:58 PDT (History)
7 users (show)

See Also:


Attachments
Patch (34.34 KB, patch)
2017-05-30 22:37 PDT, Brent Fulgham
aestes: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2017-05-30 14:03:00 PDT
The tests for ResourceLoadStatistics are inherently flaky, because they expose the algorithm to various inputs, then immediately attempts to read back state even though the statistics are being processed in a background queue.

The test harness needs to be revised to work more like the scrolling tests, where a callback is registered to be called when the work is complete.
Comment 1 Brent Fulgham 2017-05-30 14:03:26 PDT
<rdar://problem/32028373>
Comment 2 Brent Fulgham 2017-05-30 22:37:49 PDT
Created attachment 311574 [details]
Patch
Comment 3 Andy Estes 2017-05-31 09:21:28 PDT
Comment on attachment 311574 [details]
Patch

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

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:146
> +            RunLoop::main().dispatch([this, protectedThis = makeRef(*this)] () mutable {
> +                WebProcessProxy::notifyPageStatisticsAndDataRecordsProcessed();
> +            });

It's not obvious why `this` has to be captured here since the lambda calls a static function.
Comment 4 Brent Fulgham 2017-05-31 09:30:38 PDT
Comment on attachment 311574 [details]
Patch

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

>> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:146
>> +            });
> 
> It's not obvious why `this` has to be captured here since the lambda calls a static function.

Good point. I'll remove that and double-check before landing.
Comment 5 Brent Fulgham 2017-05-31 09:58:55 PDT
Committed r217606: <http://trac.webkit.org/changeset/217606>