Bug 205844 - Add correct grandfathering functionality to the ITP database backend
Summary: Add correct grandfathering functionality to the ITP database backend
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: katherine_cheney
URL:
Keywords: InRadar
: 205753 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-01-06 17:38 PST by katherine_cheney
Modified: 2020-01-13 12:59 PST (History)
5 users (show)

See Also:


Attachments
Patch (26.14 KB, patch)
2020-01-07 10:36 PST, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch (30.69 KB, patch)
2020-01-07 16:46 PST, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch (31.31 KB, patch)
2020-01-08 11:15 PST, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch (31.25 KB, patch)
2020-01-10 17:59 PST, katherine_cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description katherine_cheney 2020-01-06 17:38:37 PST
The ITP database should grandfather statistics with the same functionality as the memory store
Comment 1 Radar WebKit Bug Importer 2020-01-06 17:39:29 PST
<rdar://problem/58360450>
Comment 2 katherine_cheney 2020-01-07 10:36:17 PST
Created attachment 386990 [details]
Patch
Comment 3 katherine_cheney 2020-01-07 16:46:13 PST
Created attachment 387053 [details]
Patch
Comment 4 Brent Fulgham 2020-01-07 17:09:19 PST
Comment on attachment 387053 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:315
> +        m_shouldGrandfatherStatistics = false;

I feel like we should use the ShouldGrandfatherStatistics enum here, since it exists.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:261
> +    bool m_shouldGrandfatherStatistics { false };

ShouldGrandfatherStatistics m_shouldGrandfatherStatistics { ShouldGrandfatherStatistics::No };

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:955
> +            downcast<ResourceLoadStatisticsDatabaseStore>(*m_statisticsStore).setShouldGrandfatherStatistics(true);

Why isn't this case handled below, in the completion handler for 'm_statisticsStore->clear()', where we call 'grandfatherExistingWebsiteData'?
Comment 5 katherine_cheney 2020-01-08 08:31:56 PST
(In reply to Brent Fulgham from comment #4)
> Comment on attachment 387053 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387053&action=review
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:315
> > +        m_shouldGrandfatherStatistics = false;
> 
> I feel like we should use the ShouldGrandfatherStatistics enum here, since
> it exists.
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:261
> > +    bool m_shouldGrandfatherStatistics { false };
> 
> ShouldGrandfatherStatistics m_shouldGrandfatherStatistics {
> ShouldGrandfatherStatistics::No };
> 

Agreed! I will use this if we keep the boolean name, see my comment below about potentially changing it.

> > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:955
> > +            downcast<ResourceLoadStatisticsDatabaseStore>(*m_statisticsStore).setShouldGrandfatherStatistics(true);
> 
> Why isn't this case handled below, in the completion handler for
> 'm_statisticsStore->clear()', where we call 'grandfatherExistingWebsiteData'?

I put it here because it's logically different than the shouldGrandfather variable below and corresponds with the existence of the plist, not on the website data being removed. Looking at it now, I'm wondering if I should call the boolean something different than m_shouldGrandfatherStatistics. Something like "m_isNewResourceLoadStatisticsFile" seems more accurate for the purpose it serves.
Comment 6 katherine_cheney 2020-01-08 08:34:27 PST
So many failing/crashing iOS API tests... They aren't the ones I changed or added but could be related? I'll look into them
Comment 7 Chris Dumez 2020-01-08 08:44:45 PST
Comment on attachment 387053 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:138
> +    bool shouldGrandfatherStatistics() { return m_shouldGrandfatherStatistics; }

const

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:234
> +            ResourceLoadStatisticsDatabaseStore& databaseStore = downcast<ResourceLoadStatisticsDatabaseStore>(*m_statisticsStore);

auto&

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:241
> +                postTaskReply([this, protectedThis = protectedThis.copyRef(), completionHandler = WTFMove(completionHandler)]() mutable {

WTFMove(protectedThis) ?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:176
> +    NSURL *statisticsDirectoryURL = [NSURL fileURLWithPath:[@"~/Library/WebKit/TestWebKitAPI/WebsiteData/ResourceLoadStatistics" stringByExpandingTildeInPath] isDirectory:YES];

Seems wrong to use ~ in testing. We normally use NSTemporaryDirectory() in tests.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:303
> +    NSURL *statisticsDirectoryURL = [NSURL fileURLWithPath:[@"~/Library/WebKit/TestWebKitAPI/WebsiteData/ResourceLoadStatistics" stringByExpandingTildeInPath] isDirectory:YES];

Ditto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:418
> +    NSURL *statisticsDirectoryURL = [NSURL fileURLWithPath:[@"~/Library/WebKit/TestWebKitAPI/WebsiteData/ResourceLoadStatistics" stringByExpandingTildeInPath] isDirectory:YES];

ditto
Comment 8 Chris Dumez 2020-01-08 08:45:38 PST
Comment on attachment 387053 [details]
Patch

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

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:176
>> +    NSURL *statisticsDirectoryURL = [NSURL fileURLWithPath:[@"~/Library/WebKit/TestWebKitAPI/WebsiteData/ResourceLoadStatistics" stringByExpandingTildeInPath] isDirectory:YES];
> 
> Seems wrong to use ~ in testing. We normally use NSTemporaryDirectory() in tests.

I mean something like:
[NSURL fileURLWithPath:[NSTemporaryDirectory() stringByAppendingPathComponent:@"ResourceLoadStatistics"]
Comment 9 katherine_cheney 2020-01-08 11:01:47 PST
(In reply to Chris Dumez from comment #7)
> Comment on attachment 387053 [details]
> Patch
> 

Thanks for the comments!

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387053&action=review
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:138
> > +    bool shouldGrandfatherStatistics() { return m_shouldGrandfatherStatistics; }
> 
> const
> 

This one gives me a compiler error: error: 'const' type qualifier on return type has no effect


> > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:234
> > +            ResourceLoadStatisticsDatabaseStore& databaseStore = downcast<ResourceLoadStatisticsDatabaseStore>(*m_statisticsStore);
> 
> auto&
> 
> > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:241
> > +                postTaskReply([this, protectedThis = protectedThis.copyRef(), completionHandler = WTFMove(completionHandler)]() mutable {
> 
> WTFMove(protectedThis) ?
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:176
> > +    NSURL *statisticsDirectoryURL = [NSURL fileURLWithPath:[@"~/Library/WebKit/TestWebKitAPI/WebsiteData/ResourceLoadStatistics" stringByExpandingTildeInPath] isDirectory:YES];
> 
> Seems wrong to use ~ in testing. We normally use NSTemporaryDirectory() in
> tests.
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:303
> > +    NSURL *statisticsDirectoryURL = [NSURL fileURLWithPath:[@"~/Library/WebKit/TestWebKitAPI/WebsiteData/ResourceLoadStatistics" stringByExpandingTildeInPath] isDirectory:YES];
> 
> Ditto
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:418
> > +    NSURL *statisticsDirectoryURL = [NSURL fileURLWithPath:[@"~/Library/WebKit/TestWebKitAPI/WebsiteData/ResourceLoadStatistics" stringByExpandingTildeInPath] isDirectory:YES];
> 
> ditto


This does seem strange, but this follows the same pattern as IndexDB and LocalStorage (see LocalStorageClear.mm or IndexUpgradeToV2.mm or WebsiteDataStoreCustomPaths.mm). Unlike the layout tests, it seems like the ITP store is always in the same place for TestWebKitAPI.
Comment 10 katherine_cheney 2020-01-08 11:15:24 PST
Created attachment 387115 [details]
Patch
Comment 11 John Wilander 2020-01-08 12:03:04 PST
I remember Brady made a change early on so that we do attempt to grandfather any website data if the user has just deleted all website data. This case means ITP has just been told to delete all of *its* data but in parallel, all other website data has also been deleted and there is no reason to try to scan all those website data types in search for something to grandfather.

We should make sure to maintain that behavior.
Comment 12 katherine_cheney 2020-01-08 12:40:38 PST
(In reply to John Wilander from comment #11)
> I remember Brady made a change early on so that we do attempt to grandfather
> any website data if the user has just deleted all website data. This case
> means ITP has just been told to delete all of *its* data but in parallel,
> all other website data has also been deleted and there is no reason to try
> to scan all those website data types in search for something to grandfather.
> 
> We should make sure to maintain that behavior.

I believe this case is covered in the GrandfatherCallbackDatabase test. More specifically, in this snippet:

 225    grandfatheredFlag = false;
 226    doneFlag = false;
 227    [dataStore removeDataOfTypes:[WKWebsiteDataStore _allWebsiteDataTypesIncludingPrivate]  modifiedSince:[NSDate distantPast] completionHandler:^ {
 228        doneFlag = true;
 229    }];
 230
 231    TestWebKitAPI::Util::run(&doneFlag);
 232    TestWebKitAPI::Util::spinRunLoop(10);
 233
 234    // The website data store remove should have completed, but since we removed all of the data types that are monitored by resource load statistics,
 235    // no grandfathering call should have been made. Note that the database file will not be deleted like in the persistent storage case, only cleared.
 236    EXPECT_FALSE(grandfatheredFlag);
 237
 238    doneFlag = false;
 239    [dataStore removeDataOfTypes:[NSSet setWithObjects:WKWebsiteDataTypeCookies, _WKWebsiteDataTypeResourceLoadStatistics, nil] modifiedSince:[NSDate distantPast] completionHandler:^ {
 240        doneFlag = true;
 241    }];
 242
 243    // Since we did not remove every data type covered by resource load statistics, we do expect that grandfathering took place again.
 244    // If the test hangs waiting on either of these conditions, it has failed.
 245    TestWebKitAPI::Util::run(&grandfatheredFlag);


This grandfathering actually happens in the WebResourceLoadStatisticsStore regardless of the store type (memory or database), so it should be unaffected by this patch.
Comment 13 John Wilander 2020-01-10 16:23:03 PST
Comment on attachment 387115 [details]
Patch

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

Overall, looks good to me. See inline comments before landing.

I looked up the existing code for avoiding grandfathering when all the website data ITP cares about is deleted anyway.

It's in NetworkProcess::deleteWebsiteData() and looks like this:

#if ENABLE(RESOURCE_LOAD_STATISTICS)
    if (websiteDataTypes.contains(WebsiteDataType::ResourceLoadStatistics)) {
        if (auto* networkSession = this->networkSession(sessionID)) {
            if (auto* resourceLoadStatistics = networkSession->resourceLoadStatistics()) {
                auto deletedTypesRaw = websiteDataTypes.toRaw();
                auto monitoredTypesRaw = WebResourceLoadStatisticsStore::monitoredDataTypes().toRaw();
                
                // If we are deleting all of the data types that the resource load statistics store monitors
                // we do not need to re-grandfather old data.
                auto shouldGrandfather = ((monitoredTypesRaw & deletedTypesRaw) == monitoredTypesRaw) ? ShouldGrandfatherStatistics::No : ShouldGrandfatherStatistics::Yes;
                
                resourceLoadStatistics->scheduleClearInMemoryAndPersistent(modifiedSince, shouldGrandfather, [clearTasksHandler = clearTasksHandler.copyRef()] { });
            }
        }
    }
#endif

Putting it here for documentation.

> Source/WebKit/ChangeLog:3
> +        The ITP database backend does not grandfather statistics correctly

I would rephrase this to say what the patch does rather than what the bug is.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:138
> +    bool isNewResourceLoadStatisticsDatabaseFile() { return m_isNewResourceLoadStatisticsDatabaseFile; }

This function can be const.

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:239
> +                databaseStore.setIsNewResourceLoadStatisticsDatabaseFile(false);

Good. Important that we reset the state.

> Tools/ChangeLog:3
> +        The ITP database backend does not grandfather statistics correctly

Same thing for the title.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:115
> +        // the memory store and testing grandfathering.

This comment was confusion to me. We're in a clause where the database is enabled and the comments deals with the memory store note being enabled.

Can this be clarified somehow? I think there are similarly confusing comments further down.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:201
> +        // the database store and testing grandfathering.

This comment is confusing since clearly we're in a clause where the database store is not enabled. Could this be rephrased as "Since the database store …"?
Comment 14 katherine_cheney 2020-01-10 17:57:14 PST
(In reply to John Wilander from comment #13)
> Comment on attachment 387115 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387115&action=review
> 
> Overall, looks good to me. See inline comments before landing.
> 

Thanks for looking it over!

> I looked up the existing code for avoiding grandfathering when all the
> website data ITP cares about is deleted anyway.
> 
> It's in NetworkProcess::deleteWebsiteData() and looks like this:
> 
> #if ENABLE(RESOURCE_LOAD_STATISTICS)
>     if (websiteDataTypes.contains(WebsiteDataType::ResourceLoadStatistics)) {
>         if (auto* networkSession = this->networkSession(sessionID)) {
>             if (auto* resourceLoadStatistics =
> networkSession->resourceLoadStatistics()) {
>                 auto deletedTypesRaw = websiteDataTypes.toRaw();
>                 auto monitoredTypesRaw =
> WebResourceLoadStatisticsStore::monitoredDataTypes().toRaw();
>                 
>                 // If we are deleting all of the data types that the
> resource load statistics store monitors
>                 // we do not need to re-grandfather old data.
>                 auto shouldGrandfather = ((monitoredTypesRaw &
> deletedTypesRaw) == monitoredTypesRaw) ? ShouldGrandfatherStatistics::No :
> ShouldGrandfatherStatistics::Yes;
>                 
>                
> resourceLoadStatistics->scheduleClearInMemoryAndPersistent(modifiedSince,
> shouldGrandfather, [clearTasksHandler = clearTasksHandler.copyRef()] { });
>             }
>         }
>     }
> #endif
> 
> Putting it here for documentation.
> 

Good idea.


Going to upload with these changes and let the bots run before landing.
Comment 15 katherine_cheney 2020-01-10 17:59:08 PST
Created attachment 387409 [details]
Patch
Comment 16 WebKit Commit Bot 2020-01-11 06:58:05 PST
Comment on attachment 387409 [details]
Patch

Clearing flags on attachment: 387409

Committed r254397: <https://trac.webkit.org/changeset/254397>
Comment 17 WebKit Commit Bot 2020-01-11 06:58:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 katherine_cheney 2020-01-13 12:59:19 PST
*** Bug 205753 has been marked as a duplicate of this bug. ***