The ITP database should grandfather statistics with the same functionality as the memory store
<rdar://problem/58360450>
Created attachment 386990 [details] Patch
Created attachment 387053 [details] Patch
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'?
(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.
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 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 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"]
(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.
Created attachment 387115 [details] Patch
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.
(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 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 …"?
(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.
Created attachment 387409 [details] Patch
Comment on attachment 387409 [details] Patch Clearing flags on attachment: 387409 Committed r254397: <https://trac.webkit.org/changeset/254397>
All reviewed patches have been landed. Closing bug.
*** Bug 205753 has been marked as a duplicate of this bug. ***