Bug 174825 – ResourceLoadStatistics grandfathering happens much too often

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brady Eidson]

ResourceLoadStatistics grandfathering happens much too often

There's a few things in play here.

It's possible to launch a WebKit app without the plist on the filesystem, do the grandfathering, then never have the plist written out to the filesystem. This means grandfathering will happen again and again at every launch.

It's also bizarre than an app can removeWebsiteData to clear all data types, but then we immediately turn around and try to grandfather data again - Even though there's definitely none there!

This patch makes the grandfathering much smarter and adds an API test to prove it.

[Brady Eidson]

Note: Preventing grandfathering on launch when possible is important because it fires up child processes (Databases, Plugins, etc) and leaves them running. Power/Memory/Time waste.

<rdar://problem/32655834>

[Brady Eidson]

Created attachment 316395 [details]
Patch

[Build Bot]

Attachment 316395 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:131:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:195:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1305:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:130:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 316395 [details]
Patch

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

> Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:305
> +void ResourceLoadStatisticsPersistentStorage::clearLastStatisticsWriteTime()

It is weird to expose this internal concept outside the class? Why do we need it. I assume want to bypass the throttling? If so, I feel it would be a lot clean to pass a flag to scheduleOrWriteMemoryStore() instead.

[Brady Eidson]

Created attachment 316397 [details]
Patch

[Chris Dumez]

Comment on attachment 316395 [details]
Patch

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

I see you uploaded a new patch, will keep reviewing on the new iteration instead.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:196
> +        WebProcessProxy::deleteWebsiteDataForTopPrivatelyControlledDomainsInAllPersistentDataStores(WebResourceLoadStatisticsStore::monitoredDataTypes(), WTFMove(prevalentResourceDomains), m_parameters.shouldNotifyPagesWhenDataRecordsWereScanned, [this, protectedThis = WTFMove(protectedThis)](const HashSet<String>& domainsWithDeletedWebsiteData) mutable {

Do we really need this WebResourceLoadStatisticsStore:: ?

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:266
> +            logTestingEvent(ASCIILiteral("Grandfathered"));

Technically grandfathering has not necessarily happened at this point. Should this get called at the end of the m_statisticsQueue->dispatch() block above? By the way, if we moved it the block above, I believe we could always dispatch to the main runloop in logTestingEvent() and add an assertion that it is always called on a background thread.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:847
> +            if (m_statisticsTestingCallback)

Why this check? I don't think we hurts to make a call even if it is unset.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:107
> +    enum class ShouldGrandfather {

I think it would look better on one line. Also, I feel it would make sense to have No first, and Yes second (Because no is usually 0 and yes is usually 1).

[Chris Dumez]

Comment on attachment 316397 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:493
> +- (void)_setResourceLoadStatisticsTestingCallback:(void (^)(WKWebsiteDataStore *, NSString *))callback

Why doesn't the call site wait until it has set the callback to actually enable the feature? It seems a bit odd to have an SPI that does 2 things, especially considering we have an SPI to toggle the feature already.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/ResourceLoadStatistics.mm:51
> +        EXPECT_TRUE([message isEqualToString:@"Grandfathered"]);

I think this should be ASSERT_TRUE since you set the flag to true below.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/ResourceLoadStatistics.mm:90
> +    while (true) {

This code is duplicated from above. It'd be nice to move it to a utility function.

[Chris Dumez]

Comment on attachment 316397 [details]
Patch

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

Please see my comments on this iteration AND the previous one.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:478
> +    store->scheduleClearInMemoryAndPersistent(std::chrono::system_clock::now() - std::chrono::hours(hours), WebKit::WebResourceLoadStatisticsStore::ShouldGrandfather::Yes);

ditto.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:493
>> +- (void)_setResourceLoadStatisticsTestingCallback:(void (^)(WKWebsiteDataStore *, NSString *))callback
> 
> Why doesn't the call site wait until it has set the callback to actually enable the feature? It seems a bit odd to have an SPI that does 2 things, especially considering we have an SPI to toggle the feature already.

Never mind this comment, I missed what enableResourceLoadStatisticsAndSetTestingCallback() was doing.

> Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.h:53
> +        WriteLater,

I don't like this naming since we may write immediately even if you pass WriteLater (i.e. if there has been no write in the last 5 minutes).
I suggest enum ShouldForceImmediateWrite { No, Yes }; or similar.

> Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.h:55
> +    void scheduleOrWriteMemoryStore(WriteTime);

I think the default parameter value should be to not force an immediate write, given that it is the common case. Then some call sites will look shorter.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:72
> +    void initialize();

I don't like having a separate initialize function.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1098
> +    if (dataTypes.contains(WebsiteDataType::ResourceLoadStatistics) && m_resourceLoadStatistics) {

This looks duplicated. Can we move to a separate function?

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1321
> +        m_resourceLoadStatistics->setStatisticsTestingCallback(WTFMove(callback));

I think this could be passed to WebResourceLoadStatisticsStore::create(), with the other lambda. Then we wouldn't need the initialize() function below.

[Brady Eidson]

(In reply to Chris Dumez from comment #8)
> Comment on attachment 316397 [details]
> 
> > Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.h:55
> > +    void scheduleOrWriteMemoryStore(WriteTime);
> 
> I think the default parameter value should be to not force an immediate
> write, given that it is the common case. Then some call sites will look
> shorter.

There's only two callsites.

And default arguments are the devil.

[Brady Eidson]

(In reply to Chris Dumez from comment #8)
> Comment on attachment 316397 [details]
> 
> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:72
> > +    void initialize();
> 
> I don't like having a separate initialize function.
> 
> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1098
> > +    if (dataTypes.contains(WebsiteDataType::ResourceLoadStatistics) && m_resourceLoadStatistics) {
> 
> This looks duplicated. Can we move to a separate function?

Close, but not quite. Don't want to overload having a time vs not having one.

[Brady Eidson]

(In reply to Chris Dumez from comment #8)
> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1321
> > +        m_resourceLoadStatistics->setStatisticsTestingCallback(WTFMove(callback));
> 
> I think this could be passed to WebResourceLoadStatisticsStore::create(),
> with the other lambda. Then we wouldn't need the initialize() function below.

That really makes this code sloppier (Why should WebsiteDataStore have to hold on to a callback it doesn't need or understand?) but I got the sense on IRC you really want that, so, okay. :)

[Chris Dumez]

(In reply to Brady Eidson from comment #11)
> (In reply to Chris Dumez from comment #8)
> > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1321
> > > +        m_resourceLoadStatistics->setStatisticsTestingCallback(WTFMove(callback));
> > 
> > I think this could be passed to WebResourceLoadStatisticsStore::create(),
> > with the other lambda. Then we wouldn't need the initialize() function below.
> 
> That really makes this code sloppier (Why should WebsiteDataStore have to
> hold on to a callback it doesn't need or understand?) but I got the sense on
> IRC you really want that, so, okay. :)

 I am not suggesting WebsiteDataStore holds on to the callback, just pass it to the StatisticsStore upon creation.

[Brady Eidson]

Created attachment 316406 [details]
Patch

[Build Bot]

Attachment 316406 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:148:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:129:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:133:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:193:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1305:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:130:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 7 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 316406 [details]
Patch

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

> Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:282
> +void ResourceLoadStatisticsPersistentStorage::scheduleOrWriteMemoryStore(enum ForceImmediateWrite forceImmediateWrite)

We don't need the "enum " here.

> Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:287
> +    if (forceImmediateWrite != ForceImmediateWrite && timeSinceLastWrite < minimumWriteInterval) {

This looks pretty confusing: forceImmediateWrite != ForceImmediateWrite

> Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.h:55
> +    enum ForceImmediateWrite {

Why didn't you keep the enum class?

> Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.h:59
> +    void scheduleOrWriteMemoryStore(enum ForceImmediateWrite);

don't need the "enum ".

[Chris Dumez]

Comment on attachment 316406 [details]
Patch

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

> Source/WebKit/ChangeLog:39
> +        (WebKit::WebResourceLoadStatisticsStore::initialize): Separated out from the constructor so a callback can be registered

This is outdated.

>> Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.h:55
>> +    enum ForceImmediateWrite {
> 
> Why didn't you keep the enum class?

If you don't want to use an enum class, at least let's use a better name, like WriteMode maybe.

[Brady Eidson]

(In reply to Chris Dumez from comment #15)
> Comment on attachment 316406 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316406&action=review
> 
> > Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:282
> > +void ResourceLoadStatisticsPersistentStorage::scheduleOrWriteMemoryStore(enum ForceImmediateWrite forceImmediateWrite)
> 
> We don't need the "enum " here.

You do! When your enum name and one of the enum values are the same, you have to specify whether you're referring to the the type or the value with the enum keyword.

> 
> > Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:287
> > +    if (forceImmediateWrite != ForceImmediateWrite && timeSinceLastWrite < minimumWriteInterval) {
> 
> This looks pretty confusing: forceImmediateWrite != ForceImmediateWrite

If I change it to an enum class it won't look much better:
forceImmediateWrite != ForceImmediateWrite::Yes

> 
> > Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.h:55
> > +    enum ForceImmediateWrite {
> 
> Why didn't you keep the enum class?

It was not an enum class before!

> 
> > Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.h:59
> > +    void scheduleOrWriteMemoryStore(enum ForceImmediateWrite);
> 
> don't need the "enum ".

Same comment as above.

[Brady Eidson]

I'll make it an enum class, but am letting EWS finish on this version then will land manually

[Brady Eidson]

Comment on attachment 316406 [details]
Patch

For EWS

[Brady Eidson]

Created attachment 316410 [details]
Patch to land later after EWS clears this previous one

[WebKit Commit Bot]

Comment on attachment 316410 [details]
Patch to land later after EWS clears this previous one

Rejecting attachment 316410 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 316410, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
or: the Git diff contains a binary file without the binary data in line: "Binary files /dev/null and b/Tools/TestWebKitAPI/Tests/WebKit2Cocoa/EmptyGrandfatheredResourceLoadStatistics.plist differ".  Be sure to use the --binary flag when invoking "git diff" with diffs containing binary files. at /Volumes/Data/EWS/WebKit/Tools/Scripts/VCSUtils.pm line 870, <ARGV> line 654.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 25 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/4188470

[Brady Eidson]

Created attachment 316432 [details]
Patch w/binary

[Build Bot]

Attachment 316432 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:148:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:129:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:133:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:193:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1305:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:130:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 7 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 316432 [details]
Patch w/binary

Clearing flags on attachment: 316432

Committed r219904: <http://trac.webkit.org/changeset/219904>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.