Bug 174825 - ResourceLoadStatistics grandfathering happens much too often
Summary: ResourceLoadStatistics grandfathering happens much too often
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-07-25 11:06 PDT by Brady Eidson
Modified: 2017-07-25 22:44 PDT (History)
5 users (show)

See Also:


Attachments
Patch (44.51 KB, patch)
2017-07-25 15:10 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (45.19 KB, patch)
2017-07-25 15:32 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (46.78 KB, patch)
2017-07-25 16:50 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch to land later after EWS clears this previous one (45.60 KB, patch)
2017-07-25 17:20 PDT, Brady Eidson
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch w/binary (45.75 KB, patch)
2017-07-25 22:06 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2017-07-25 11:06:30 PDT
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.
Comment 1 Brady Eidson 2017-07-25 11:07:14 PDT
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>
Comment 2 Brady Eidson 2017-07-25 15:10:12 PDT
Created attachment 316395 [details]
Patch
Comment 3 Build Bot 2017-07-25 15:13:02 PDT
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.
Comment 4 Chris Dumez 2017-07-25 15:21:46 PDT
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.
Comment 5 Brady Eidson 2017-07-25 15:32:34 PDT
Created attachment 316397 [details]
Patch
Comment 6 Chris Dumez 2017-07-25 15:33:31 PDT
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).
Comment 7 Chris Dumez 2017-07-25 15:47:55 PDT
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.
Comment 8 Chris Dumez 2017-07-25 16:19:59 PDT
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.
Comment 9 Brady Eidson 2017-07-25 16:32:05 PDT
(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.
Comment 10 Brady Eidson 2017-07-25 16:32:58 PDT
(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.
Comment 11 Brady Eidson 2017-07-25 16:34:54 PDT
(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. :)
Comment 12 Chris Dumez 2017-07-25 16:36:47 PDT
(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.
Comment 13 Brady Eidson 2017-07-25 16:50:10 PDT
Created attachment 316406 [details]
Patch
Comment 14 Build Bot 2017-07-25 16:52:42 PDT
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.
Comment 15 Chris Dumez 2017-07-25 16:54:25 PDT
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 ".
Comment 16 Chris Dumez 2017-07-25 16:56:44 PDT
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.
Comment 17 Brady Eidson 2017-07-25 16:58:03 PDT
(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.
Comment 18 Brady Eidson 2017-07-25 16:58:51 PDT
I'll make it an enum class, but am letting EWS finish on this version then will land manually
Comment 19 Brady Eidson 2017-07-25 17:08:04 PDT
Comment on attachment 316406 [details]
Patch

For EWS
Comment 20 Brady Eidson 2017-07-25 17:20:11 PDT
Created attachment 316410 [details]
Patch to land later after EWS clears this previous one
Comment 21 WebKit Commit Bot 2017-07-25 21:59:13 PDT
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
Comment 22 Brady Eidson 2017-07-25 22:06:19 PDT
Created attachment 316432 [details]
Patch w/binary
Comment 23 Build Bot 2017-07-25 22:08:27 PDT
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.
Comment 24 WebKit Commit Bot 2017-07-25 22:44:08 PDT
Comment on attachment 316432 [details]
Patch w/binary

Clearing flags on attachment: 316432

Committed r219904: <http://trac.webkit.org/changeset/219904>
Comment 25 WebKit Commit Bot 2017-07-25 22:44:10 PDT
All reviewed patches have been landed.  Closing bug.