Bug 172155 – Resource Load Statistics: Grandfather domains for existing data records

• Component: WebKit2
• Status: RESOLVED FIXED

[John Wilander]

We should grandfather domains for existing data records so allow ample time to capture user interaction.

[John Wilander]

rdar://problem/24913532

[John Wilander]

Created attachment 310207 [details]
Patch

[John Wilander]

Created attachment 310270 [details]
Patch

[Build Bot]

Comment on attachment 310270 [details]
Patch

Attachment 310270 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3751624

New failing tests:
http/tests/loading/resourceLoadStatistics/grandfathering.html

[Build Bot]

Created attachment 310279 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[John Wilander]

Created attachment 310286 [details]
Patch

[John Wilander]

Created attachment 310288 [details]
Patch

[John Wilander]

The test case is not timing out on my machine which is why I'm trying a few things here to see what the bots do.

[Build Bot]

Comment on attachment 310288 [details]
Patch

Attachment 310288 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3752175

New failing tests:
http/tests/loading/resourceLoadStatistics/grandfathering.html

[Build Bot]

Created attachment 310292 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[John Wilander]

Created attachment 310303 [details]
Patch

[John Wilander]

Created attachment 310312 [details]
Patch

[Alex Christensen]

Comment on attachment 310312 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:47
> +// 3 days in seconds
> +static auto grandfatheringTime = 259200;

static const auto secondsPerDay = 24 * 3600;
static auto grandfatheringTime = 3 * secondsPerDay;
etc.

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:103
> +    if (version > 3) {

static const auto minimumVersionWithGrandfathering = 3;

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:346
> +    double now = currentTime();

This local variable seems excessive.

> Source/WebCore/loader/ResourceLoadStatisticsStore.h:94
> +    double m_endOfGrandfatheringTimestamp { 0 };
> +    double m_lastTimeDataRecordsWereRemoved { 0 };

double -> Seconds?

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:268
> +void WebProcessProxy::topPrivatelyControlledDomainsWithWebiteData(OptionSet<WebsiteDataType> dataTypes, bool shouldNotifyPage, std::function<void(HashSet<String>&&)> completionHandler)

I know we use std::function elsewhere in this file, but I don't think we should keep adding std::function where we can use Function, even if it differs in style from nearby code, unless it would require huge refactoring to not.

> Source/WebKit2/UIProcess/API/C/WKResourceLoadStatisticsManager.h:-50
> -    WK_EXPORT void WKResourceLoadStatisticsManagerSetMinimumTimeBetweeenDataRecordsRemoval(double seconds);

I assume this has never been adopted so we don't need to keep it in for binary compatibility with anything.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:526
> +        for (auto&& dataRecord : existingDataRecords)

auto&&
cool!

[John Wilander]

Created attachment 310459 [details]
Patch for landing

[John Wilander]

(In reply to Alex Christensen from comment #13)
> Comment on attachment 310312 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310312&action=review
> 
> > Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:47
> > +// 3 days in seconds
> > +static auto grandfatheringTime = 259200;
> 
> static const auto secondsPerDay = 24 * 3600;
> static auto grandfatheringTime = 3 * secondsPerDay;
> etc.

Good suggestion. Fixed.

> > Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:103
> > +    if (version > 3) {
> 
> static const auto minimumVersionWithGrandfathering = 3;

Fixed.

> > Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:346
> > +    double now = currentTime();
> 
> This local variable seems excessive.

True.

> > Source/WebCore/loader/ResourceLoadStatisticsStore.h:94
> > +    double m_endOfGrandfatheringTimestamp { 0 };
> > +    double m_lastTimeDataRecordsWereRemoved { 0 };
> 
> double -> Seconds?

This I think goes into the bucked of changes we should do in an overhaul. Andy brought up the same thing earlier.

> > Source/WebKit2/UIProcess/WebProcessProxy.cpp:268
> > +void WebProcessProxy::topPrivatelyControlledDomainsWithWebiteData(OptionSet<WebsiteDataType> dataTypes, bool shouldNotifyPage, std::function<void(HashSet<String>&&)> completionHandler)
> 
> I know we use std::function elsewhere in this file, but I don't think we
> should keep adding std::function where we can use Function, even if it
> differs in style from nearby code, unless it would require huge refactoring
> to not.

Yes, I know you've said this before and so has Andy. Should I file a Bugzilla to work through the WebProcessProxy and the WebsiteDataStore classes and do this change?

> > Source/WebKit2/UIProcess/API/C/WKResourceLoadStatisticsManager.h:-50
> > -    WK_EXPORT void WKResourceLoadStatisticsManagerSetMinimumTimeBetweeenDataRecordsRemoval(double seconds);
> 
> I assume this has never been adopted so we don't need to keep it in for
> binary compatibility with anything.

I was never implemented.

> > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:526
> > +        for (auto&& dataRecord : existingDataRecords)
> 
> auto&&
> cool!

Thanks, Alex!

[WebKit Commit Bot]

Comment on attachment 310459 [details]
Patch for landing

Clearing flags on attachment: 310459

Committed r217014: <http://trac.webkit.org/changeset/217014>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #16)
> Comment on attachment 310459 [details]
> Patch for landing
> 
> Clearing flags on attachment: 310459
> 
> Committed r217014: <http://trac.webkit.org/changeset/217014>
This change caused LayoutTests to exit early with crashes on mac-wk2:

https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r217014%20(1561)/accessibility/accessibility-crash-setattribute-crash-log.txt

https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20(Tests)/builds/1561

[Ryan Haddad]

Reverted r217014 for reason:

This change caused mac-wk2 LayoutTests to exit early due to crashes.

Committed r217020: <http://trac.webkit.org/changeset/217020>

[John Wilander]

Created attachment 310522 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 310522 [details]
Patch

Clearing flags on attachment: 310522

Committed r217068: <http://trac.webkit.org/changeset/217068>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.