Bug 205844 – Add correct grandfathering functionality to the ITP database backend

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

[Kate Cheney]

The ITP database should grandfather statistics with the same functionality as the memory store

[Radar WebKit Bug Importer]

<rdar://problem/58360450>

[Kate Cheney]

Created attachment 386990 [details]
Patch

[Kate Cheney]

Created attachment 387053 [details]
Patch

[Brent Fulgham]

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'?

[Kate Cheney]

(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.

[Kate Cheney]

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

[Chris Dumez]

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

[Chris Dumez]

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"]

[Kate Cheney]

(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.

[Kate Cheney]

Created attachment 387115 [details]
Patch

[John Wilander]

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.

[Kate Cheney]

(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.

[John Wilander]

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 …"?

[Kate Cheney]

(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.

[Kate Cheney]

Created attachment 387409 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 387409 [details]
Patch

Clearing flags on attachment: 387409

Committed r254397: <https://trac.webkit.org/changeset/254397>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Kate Cheney]

*** Bug 205753 has been marked as a duplicate of this bug. ***