This bug tracks the work of getting the ITP-related LayoutTests working when running with the ResourceLoadStatistics database backend.
<rdar://problem/54213551>
Created attachment 378422 [details] Patch
Created attachment 378489 [details] Patch
I ran the failing iOS tests on both the memory store and database store and they seem to be failing in both cases (all 4 memory-store-versions are skipped in the layout test expectations). I am happy to look into why they are failing, but I wanted to add a comment because I don't think the fails necessarily have to do with the logic of the database store.
Created attachment 378649 [details] Patch
(In reply to Katherine_cheney from comment #4) > I ran the failing iOS tests on both the memory store and database store and > they seem to be failing in both cases (all 4 memory-store-versions are > skipped in the layout test expectations). I am happy to look into why they > are failing, but I wanted to add a comment because I don't think the fails > necessarily have to do with the logic of the database store. These tests rely on functions not supported by iOS. I changed the layout test expectations to reflect this.
Comment on attachment 378649 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378649&action=review > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:862 > + for (auto& regDomain : domains) No abbreviations please. regDomain == :( > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:158 > + initializeStatisticsStore(RuntimeEnabledFeatures::sharedFeatures().isITPDatabaseEnabled(), resourceLoadStatisticsDirectory.isolatedCopy(), shouldIncludeLocalhost); No .isolatedCopy() here, see comment below. Also, this is the first time I see RuntimeEnabledFeatures::sharedFeatures() used in the network process, this is usually for WebProcesses. > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:172 > + postTask([this, databaseEnabled = databaseEnabled, resourceLoadStatisticsDirectory = resourceLoadStatisticsDirectory, shouldIncludeLocalhost] { This is not thread-safe: resourceLoadStatisticsDirectory = resourceLoadStatisticsDirectory.isolatedCopy() Also, no need for "= databaseEnabled", please omit. > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:193 > + void initializeStatisticsStore(bool, const String& resourceLoadStatisticsDirectory, ShouldIncludeLocalhost); Can't we re-construct the whole WebResourceLoadStatisticsStore instead of making such a method public?
(In reply to Chris Dumez from comment #8) > Comment on attachment 378649 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378649&action=review > > Also, this is the first time I see RuntimeEnabledFeatures::sharedFeatures() > used in the network process, this is usually for WebProcesses. Is there another way to check if the ITPDatabase flag is set? > > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:193 > > + void initializeStatisticsStore(bool, const String& resourceLoadStatisticsDirectory, ShouldIncludeLocalhost); > > Can't we re-construct the whole WebResourceLoadStatisticsStore instead of > making such a method public? Maybe. How would the NetworkProcess be able to change the NetworkSession's m_resourceLoadStatistics value to make it refer to the new store? I think the variable is protected.
(In reply to Katherine_cheney from comment #9) > (In reply to Chris Dumez from comment #8) > > Comment on attachment 378649 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=378649&action=review > > > > Also, this is the first time I see RuntimeEnabledFeatures::sharedFeatures() > > used in the network process, this is usually for WebProcesses. > > Is there another way to check if the ITPDatabase flag is set? I doubt this flag is even set in the NetworkProcess in the first place, although I have not checked. Somebody probably needs to tell the network process at some point, and we need to store the information. > > > > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:193 > > > + void initializeStatisticsStore(bool, const String& resourceLoadStatisticsDirectory, ShouldIncludeLocalhost); > > > > Can't we re-construct the whole WebResourceLoadStatisticsStore instead of > > making such a method public? > > Maybe. How would the NetworkProcess be able to change the NetworkSession's > m_resourceLoadStatistics value to make it refer to the new store? I think > the variable is protected. A NetworkSession method, like it is already done for NetworkSession::setResourceLoadStatisticsEnabled().
(In reply to Chris Dumez from comment #10) > (In reply to Katherine_cheney from comment #9) > > (In reply to Chris Dumez from comment #8) > > > Comment on attachment 378649 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=378649&action=review > > > > > > Also, this is the first time I see RuntimeEnabledFeatures::sharedFeatures() > > > used in the network process, this is usually for WebProcesses. > > > > Is there another way to check if the ITPDatabase flag is set? > > I doubt this flag is even set in the NetworkProcess in the first place, > although I have not checked. Somebody probably needs to tell the network > process at some point, and we need to store the information. I see now that the flag is set here: WebCore::RuntimeEnabledFeatures::sharedFeatures().setIsITPDatabaseEnabled(parameters.shouldEnableITPDatabase); That said, only ITP does this at the moment and I am not sure this is good practice to use RuntimeEnabledFeatures in the NetworkProcess.
Created attachment 378891 [details] Patch
Comment on attachment 378891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378891&action=review > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:458 > + auto registrableDomainID = domainID(loadStatistics.registrableDomain).value(); Why this change? The code looked a bit safer before? However, it is unconditionally de-refing an optional. > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:162 > + FileSystem::makeAllDirectories(resourceLoadStatisticsDirectory); It feels like this should be in the ResourceLoadStatisticsDatabaseStore, right before opening. > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:192 > + void flushAndDestroyPersistentStore(); Why is this public now? > Source/WebKit/NetworkProcess/NetworkSession.cpp:166 > + m_resourceLoadStatistics->flushAndDestroyPersistentStore(); This seems wrong, you should be calling destroyResourceLoadStatistics() instead. > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:270 > +void WKWebsiteDataStoreSetUseITPDatabase(WKWebsiteDataStoreRef dataStoreRef, bool value) Something is odd here, why use a completion handler internally but not expose it on your SPI. Either you need a completion handler (and you should expose it here), or you don't and the implementation should stop passing a completion handler around. > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:59 > + Unnecessary change.
(In reply to Chris Dumez from comment #13) > Comment on attachment 378891 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378891&action=review > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:458 > > + auto registrableDomainID = domainID(loadStatistics.registrableDomain).value(); > > Why this change? The code looked a bit safer before? However, it is > unconditionally de-refing an optional. the domainID() function could get called on a domain that hasn't been inserted into the database yet and returns an unsigned. I could alternatively ensure() the domain before every call to domainID() but I thought that would be adding a value to the database that was never supposed to be inserted. > > > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:162 > > + FileSystem::makeAllDirectories(resourceLoadStatisticsDirectory); > > It feels like this should be in the ResourceLoadStatisticsDatabaseStore, > right before opening. > forgot to take this out from debugging. > > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:192 > > + void flushAndDestroyPersistentStore(); > > Why is this public now? > > > Source/WebKit/NetworkProcess/NetworkSession.cpp:166 > > + m_resourceLoadStatistics->flushAndDestroyPersistentStore(); > > This seems wrong, you should be calling destroyResourceLoadStatistics() > instead. > changing now. > > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:270 > > +void WKWebsiteDataStoreSetUseITPDatabase(WKWebsiteDataStoreRef dataStoreRef, bool value) > > Something is odd here, why use a completion handler internally but not > expose it on your SPI. Either you need a completion handler (and you should > expose it here), or you don't and the implementation should stop passing a > completion handler around. > I don't think one is necessary. I'll take it out. Is it failing the gtk/wpe builds because of the # if ENABLED(RESOURCE_LOAD_STATISTICS) check surrounding the variable in the header?
Comment on attachment 378891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378891&action=review > Source/WebKit/NetworkProcess/NetworkProcess.cpp:317 > + m_ITPDatabaseEnabled = parameters.shouldEnableITPDatabase; You likely want to protect this with: #if ENABLE(RESOURCE_LOAD_STATISTICS) #endif
Created attachment 378893 [details] Patch
Comment on attachment 378893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378893&action=review > Source/WebCore/ChangeLog:3 > + Enable LayoutTests using ResourceLoadStatistics SQLite backend (195420) As explained offline, I am a bit perplexed about how this is working given that resourceLoadStatisticsUpdated() is being ignored by the SQLite backend. Our is the SQLite backend supposed to get new statistics from the WebContent process? > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:371 > + if (firstDomainID == WTF::nullopt) if (!firstDomainID) > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:460 > + if (registrableDomainID == WTF::nullopt) if (!registrableDomainID) > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:194 > + void flushAndDestroyPersistentStore(); Could you please put it back where it was? :) > Source/WebKit/NetworkProcess/NetworkProcess.h:556 > + bool m_ITPDatabaseEnabled; Needs a default initializer.
Created attachment 378906 [details] Patch
Comment on attachment 378906 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378906&action=review The code change looks sane but an ITP expert should take a look too. I do have a few more improvement ideas: > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:376 > + if (statement.bindInt(1, firstDomainID.value()) != SQLITE_OK FYI, this can be written as *firstDomainID, which is shorter but your call. Some people prefer value(). > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:457 > + nit: Unnecessary change. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:489 > + m_observedDomainCount.prepare(); I personally find it confusing that this is not part of prepareStatement(). Could we move it back to prepareStatements() and move the call to prepareStatements() before the call to isEmpty()? > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:509 > m_database.runVacuumCommand(); I think we should move this to the constructor, right after opening the database. Alternatively, we could turn-on auto-vacuum (via turnOnIncrementalAutoVacuum(), also in the constructor) since WebKit may be running for a long time. We should check with Brent what the actual reason for this vacuuming and if auto-vacuum would suffice. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:511 > + if (!prepareStatements()) { I would also avoid having to call prepareStatements() in 2 places. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:563 > + SQLiteStatement findSubresources(m_database, makeString("SELECT SubresourceUniqueRedirectsFrom.fromDomainID from SubresourceUniqueRedirectsFrom INNER JOIN ObservedDomains ON ObservedDomains.domainID = SubresourceUniqueRedirectsFrom.fromDomainID WHERE subresourceDomainID = ", String::number(primaryDomainID), " AND ObservedDomains.isPrevalent = 0")); I don't think we should be using makeString() in SQLiteStatement. Rather, we should use ? in the string and then the appropriate bind function. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:573 > + SQLiteStatement findTopFrames(m_database, makeString("SELECT TopFrameUniqueRedirectsFrom.fromDomainID from TopFrameUniqueRedirectsFrom INNER JOIN ObservedDomains ON ObservedDomains.domainID = TopFrameUniqueRedirectsFrom.fromDomainID WHERE targetDomainID = ", String::number(primaryDomainID), " AND ObservedDomains.isPrevalent = 0")); ditto. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:674 > + SQLiteStatement subframeUnderTopFrameDomainsCountStatement(m_database, makeString("SELECT subframeDomainID, COUNT(topFrameDomainID) FROM SubframeUnderTopFrameDomains WHERE subframeDomainID IN (", domainIDsOfInterest, ") GROUP BY subframeDomainID")); ditto. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1306 > + if (!m_database.tableExists("ObservedDomains"_s)) { Do we really need this check given the call to clearAllTables() above? > Source/WebKit/NetworkProcess/NetworkProcess.cpp:706 > + networkSession->createNewResourceLoadStatisticStore(); We should probably only call this if m_ITPDatabaseEnabled has actually changed. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1257 > +bool NetworkProcess::isITPDatabaseEnabled() Should be const. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1259 > + return m_ITPDatabaseEnabled; Should probably be inlined in the header. > Source/WebKit/NetworkProcess/NetworkProcess.h:556 > + bool m_ITPDatabaseEnabled = false; We like this format better: bool m_isITPDatabaseEnabled { false }; Both the curly brackets for initialization, and the "is" prefix. As per WebKit coding style, Boolean variables need a is/has/... prefix. > Source/WebKit/NetworkProcess/NetworkSession.cpp:164 > +void NetworkSession::createNewResourceLoadStatisticStore() Given what this does, I would name it recreateResourceLoadStatisticStore() > LayoutTests/ChangeLog:22 > + * http/tests/resourceLoadStatistics/add-blocking-to-redirect-database-expected.txt: Added. I did not review the tests. I think Brent and/or John should also take a look.
(In reply to Chris Dumez from comment #19) > Comment on attachment 378906 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378906&action=review Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:489 > > + m_observedDomainCount.prepare(); > > I personally find it confusing that this is not part of prepareStatement(). > Could we move it back to prepareStatements() and move the call to > prepareStatements() before the call to isEmpty()? > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:509 > > m_database.runVacuumCommand(); > > I think we should move this to the constructor, right after opening the > database. Alternatively, we could turn-on auto-vacuum (via > turnOnIncrementalAutoVacuum(), also in the constructor) since WebKit may be > running for a long time. > We should check with Brent what the actual reason for this vacuuming and if > auto-vacuum would suffice. > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:511 > > + if (!prepareStatements()) { > > I would also avoid having to call prepareStatements() in 2 places. > I agree its maybe not the best situation. The reasoning for the weird prepareStatements() placement is that statements prepared before a vacuum have to be re-prepared after the vacuum before they can be used. So I could prepare all the statements before the isEmpty() call, but might end up having to prepare them again after the vacuum if isEmpty() is false. And I think that I would also have to call finalize() on all the statements before preparing again because calling prepare() on an already-prepared statement crashes. Not sure if that's any better, what do you think? > > LayoutTests/ChangeLog:22 > > + * http/tests/resourceLoadStatistics/add-blocking-to-redirect-database-expected.txt: Added. > > I did not review the tests. I think Brent and/or John should also take a > look. +1
Comment on attachment 378906 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378906&action=review >>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:509 >>> m_database.runVacuumCommand(); >> >> I think we should move this to the constructor, right after opening the database. Alternatively, we could turn-on auto-vacuum (via turnOnIncrementalAutoVacuum(), also in the constructor) since WebKit may be running for a long time. >> We should check with Brent what the actual reason for this vacuuming and if auto-vacuum would suffice. > > I agree its maybe not the best situation. The reasoning for the weird prepareStatements() placement is that statements prepared before a vacuum have to be re-prepared after the vacuum before they can be used. So I could prepare all the statements before the isEmpty() call, but might end up having to prepare them again after the vacuum if isEmpty() is false. And I think that I would also have to call finalize() on all the statements before preparing again because calling prepare() on an already-prepared statement crashes. Not sure if that's any better, what do you think? My proposal is to move vacuuming to the constructor, right after opening, as I mentioned. Once this is done, there is no longer any issue with doing prepareStatements() only in one place.
(In reply to Chris Dumez from comment #21) > Comment on attachment 378906 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378906&action=review > > >>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:509 > >>> m_database.runVacuumCommand(); > >> > >> I think we should move this to the constructor, right after opening the database. Alternatively, we could turn-on auto-vacuum (via turnOnIncrementalAutoVacuum(), also in the constructor) since WebKit may be running for a long time. > >> We should check with Brent what the actual reason for this vacuuming and if auto-vacuum would suffice. > > > > I agree its maybe not the best situation. The reasoning for the weird prepareStatements() placement is that statements prepared before a vacuum have to be re-prepared after the vacuum before they can be used. So I could prepare all the statements before the isEmpty() call, but might end up having to prepare them again after the vacuum if isEmpty() is false. And I think that I would also have to call finalize() on all the statements before preparing again because calling prepare() on an already-prepared statement crashes. Not sure if that's any better, what do you think? > > My proposal is to move vacuuming to the constructor, right after opening, as > I mentioned. Once this is done, there is no longer any issue with doing > prepareStatements() only in one place. Which works unless the vacuum command is needed more than once, in which case it might take some more restructuring to call prepareStatements() each time. We can wait to see what Brent and/or John says about vacuuming.
(In reply to Katherine_cheney from comment #22) > (In reply to Chris Dumez from comment #21) > > Comment on attachment 378906 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=378906&action=review > > > > >>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:509 > > >>> m_database.runVacuumCommand(); > > >> > > >> I think we should move this to the constructor, right after opening the database. Alternatively, we could turn-on auto-vacuum (via turnOnIncrementalAutoVacuum(), also in the constructor) since WebKit may be running for a long time. > > >> We should check with Brent what the actual reason for this vacuuming and if auto-vacuum would suffice. > > > > > > I agree its maybe not the best situation. The reasoning for the weird prepareStatements() placement is that statements prepared before a vacuum have to be re-prepared after the vacuum before they can be used. So I could prepare all the statements before the isEmpty() call, but might end up having to prepare them again after the vacuum if isEmpty() is false. And I think that I would also have to call finalize() on all the statements before preparing again because calling prepare() on an already-prepared statement crashes. Not sure if that's any better, what do you think? > > > > My proposal is to move vacuuming to the constructor, right after opening, as > > I mentioned. Once this is done, there is no longer any issue with doing > > prepareStatements() only in one place. > > > Which works unless the vacuum command is needed more than once, in which > case it might take some more restructuring to call prepareStatements() each > time. We can wait to see what Brent and/or John says about vacuuming. I don't understand your comment. Vacuuming already only happens once, since it is called in populateFromMemoryStore() which is only called once from the constructor.
(In reply to Chris Dumez from comment #23) > (In reply to Katherine_cheney from comment #22) > > (In reply to Chris Dumez from comment #21) > > > Comment on attachment 378906 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=378906&action=review > > > > > > >>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:509 > > > >>> m_database.runVacuumCommand(); > > > >> > > > >> I think we should move this to the constructor, right after opening the database. Alternatively, we could turn-on auto-vacuum (via turnOnIncrementalAutoVacuum(), also in the constructor) since WebKit may be running for a long time. > > > >> We should check with Brent what the actual reason for this vacuuming and if auto-vacuum would suffice. > > > > > > > > I agree its maybe not the best situation. The reasoning for the weird prepareStatements() placement is that statements prepared before a vacuum have to be re-prepared after the vacuum before they can be used. So I could prepare all the statements before the isEmpty() call, but might end up having to prepare them again after the vacuum if isEmpty() is false. And I think that I would also have to call finalize() on all the statements before preparing again because calling prepare() on an already-prepared statement crashes. Not sure if that's any better, what do you think? > > > > > > My proposal is to move vacuuming to the constructor, right after opening, as > > > I mentioned. Once this is done, there is no longer any issue with doing > > > prepareStatements() only in one place. > > > > > > Which works unless the vacuum command is needed more than once, in which > > case it might take some more restructuring to call prepareStatements() each > > time. We can wait to see what Brent and/or John says about vacuuming. > > I don't understand your comment. Vacuuming already only happens once, since > it is called in populateFromMemoryStore() which is only called once from the > constructor. Thats what I originally thought, but I checked and noticed it is also is called in syncStorageIfNeeded() and syncStorageImmediately()
(In reply to Katherine_cheney from comment #24) > (In reply to Chris Dumez from comment #23) > > (In reply to Katherine_cheney from comment #22) > > > (In reply to Chris Dumez from comment #21) > > > > Comment on attachment 378906 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=378906&action=review > > > > > > > > >>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:509 > > > > >>> m_database.runVacuumCommand(); > > > > >> > > > > >> I think we should move this to the constructor, right after opening the database. Alternatively, we could turn-on auto-vacuum (via turnOnIncrementalAutoVacuum(), also in the constructor) since WebKit may be running for a long time. > > > > >> We should check with Brent what the actual reason for this vacuuming and if auto-vacuum would suffice. > > > > > > > > > > I agree its maybe not the best situation. The reasoning for the weird prepareStatements() placement is that statements prepared before a vacuum have to be re-prepared after the vacuum before they can be used. So I could prepare all the statements before the isEmpty() call, but might end up having to prepare them again after the vacuum if isEmpty() is false. And I think that I would also have to call finalize() on all the statements before preparing again because calling prepare() on an already-prepared statement crashes. Not sure if that's any better, what do you think? > > > > > > > > My proposal is to move vacuuming to the constructor, right after opening, as > > > > I mentioned. Once this is done, there is no longer any issue with doing > > > > prepareStatements() only in one place. > > > > > > > > > Which works unless the vacuum command is needed more than once, in which > > > case it might take some more restructuring to call prepareStatements() each > > > time. We can wait to see what Brent and/or John says about vacuuming. > > > > I don't understand your comment. Vacuuming already only happens once, since > > it is called in populateFromMemoryStore() which is only called once from the > > constructor. > > Thats what I originally thought, but I checked and noticed it is also is > called in syncStorageIfNeeded() and syncStorageImmediately() Ah, there are more calls to runVacuumCommand() (not more calls to populateFromMemoryStore()). I personally think we should drop these calls and enable auto-vacuum, unless Brent had a specific reason for manual vacuuming.
(In reply to Chris Dumez from comment #25) > > Thats what I originally thought, but I checked and noticed it is also is > > called in syncStorageIfNeeded() and syncStorageImmediately() > > Ah, there are more calls to runVacuumCommand() (not more calls to > populateFromMemoryStore()). I personally think we should drop these calls > and enable auto-vacuum, unless Brent had a specific reason for manual > vacuuming. I am fine with auto-vacuuming. I don't recall having any specific reason for doing it manually. I think I was just mechanically converting all file-based operations into equivalent SQLite things, and this fell out of that process. I support following Chris's suggestion here.
Created attachment 379091 [details] Patch
(In reply to Chris Dumez from comment #19) > Comment on attachment 378906 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378906&action=review > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:573 > > + SQLiteStatement findTopFrames(m_database, makeString("SELECT TopFrameUniqueRedirectsFrom.fromDomainID from TopFrameUniqueRedirectsFrom INNER JOIN ObservedDomains ON ObservedDomains.domainID = TopFrameUniqueRedirectsFrom.fromDomainID WHERE targetDomainID = ", String::number(primaryDomainID), " AND ObservedDomains.isPrevalent = 0")); > > ditto. > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:674 > > + SQLiteStatement subframeUnderTopFrameDomainsCountStatement(m_database, makeString("SELECT subframeDomainID, COUNT(topFrameDomainID) FROM SubframeUnderTopFrameDomains WHERE subframeDomainID IN (", domainIDsOfInterest, ") GROUP BY subframeDomainID")); > > ditto. > > > I don't think you can bind lists, even if they are in string format. But I updated the domainID check!
This crash on EWS is concerning: Thread 23 Crashed:: Dispatch queue: WebResourceLoadStatisticsStore Process Data Queue 0 com.apple.WebKit 0x00000001068f5ef4 WebKit::ResourceLoadStatisticsDatabaseStore::setPrevalentResource(WebCore::RegistrableDomain const&, WebKit::ResourceLoadPrevalence) + 528 1 com.apple.WebKit 0x0000000106917dcc WTF::Detail::CallableWrapper<WebKit::WebResourceLoadStatisticsStore::setPrevalentResource(WebCore::RegistrableDomain const&, WTF::CompletionHandler<void ()>&&)::$_43, void>::call() + 40 2 libdispatch.dylib 0x0000000109efdccf _dispatch_call_block_and_release + 12 3 libdispatch.dylib 0x0000000109efed02 _dispatch_client_callout + 8 4 libdispatch.dylib 0x0000000109f05720 _dispatch_lane_serial_drain + 705 5 libdispatch.dylib 0x0000000109f06261 _dispatch_lane_invoke + 398 6 libdispatch.dylib 0x0000000109f0efcb _dispatch_workloop_worker_thread + 645 7 libsystem_pthread.dylib 0x000000010a2e0611 _pthread_wqthread + 421 8 libsystem_pthread.dylib 0x000000010a2e03fd start_wqthread + 13
(In reply to Chris Dumez from comment #29) > This crash on EWS is concerning: > Thread 23 Crashed:: Dispatch queue: WebResourceLoadStatisticsStore Process > Data Queue > 0 com.apple.WebKit 0x00000001068f5ef4 > WebKit::ResourceLoadStatisticsDatabaseStore::setPrevalentResource(WebCore:: > RegistrableDomain const&, WebKit::ResourceLoadPrevalence) + 528 > 1 com.apple.WebKit 0x0000000106917dcc > WTF::Detail::CallableWrapper<WebKit::WebResourceLoadStatisticsStore:: > setPrevalentResource(WebCore::RegistrableDomain const&, > WTF::CompletionHandler<void ()>&&)::$_43, void>::call() + 40 > 2 libdispatch.dylib 0x0000000109efdccf > _dispatch_call_block_and_release + 12 > 3 libdispatch.dylib 0x0000000109efed02 > _dispatch_client_callout + 8 > 4 libdispatch.dylib 0x0000000109f05720 > _dispatch_lane_serial_drain + 705 > 5 libdispatch.dylib 0x0000000109f06261 _dispatch_lane_invoke > + 398 > 6 libdispatch.dylib 0x0000000109f0efcb > _dispatch_workloop_worker_thread + 645 > 7 libsystem_pthread.dylib 0x000000010a2e0611 _pthread_wqthread + 421 > 8 libsystem_pthread.dylib 0x000000010a2e03fd start_wqthread + 13 That was from the iOS WK2 EWS: https://ews-build.webkit.org/results/iOS-12-Simulator-WK2-Tests-EWS/r379091-3948/results.html
Comment on attachment 379091 [details] Patch r- given the crash on EWS.
Comment on attachment 379091 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379091&action=review > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:556 > + SQLiteStatement findSubresources(m_database, makeString("SELECT SubresourceUniqueRedirectsFrom.fromDomainID from SubresourceUniqueRedirectsFrom INNER JOIN ObservedDomains ON ObservedDomains.domainID = SubresourceUniqueRedirectsFrom.fromDomainID WHERE subresourceDomainID = ? AND ObservedDomains.isPrevalent = 0")); Same as below, why is this calling makeString() ? > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:560 > + ASSERT_NOT_REACHED(); Shouldn't this return early? > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:564 > + auto newDomainID = findSubresources.getColumnInt(0); Usually, I am all for using auto, but int is actually shorter than auto here. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:570 > + SQLiteStatement findTopFrames(m_database, makeString("SELECT TopFrameUniqueRedirectsFrom.fromDomainID from TopFrameUniqueRedirectsFrom INNER JOIN ObservedDomains ON ObservedDomains.domainID = TopFrameUniqueRedirectsFrom.fromDomainID WHERE targetDomainID = ? AND ObservedDomains.isPrevalent = 0")); Why is this calling makeString()? Looks like you're passing it a single literal? Why not simply "SELECT TopFrameUniqueRedirectsFrom.fromDomainID from TopFrameUniqueRedirectsFrom INNER JOIN ObservedDomains ON ObservedDomains.domainID = TopFrameUniqueRedirectsFrom.fromDomainID WHERE targetDomainID = ? AND ObservedDomains.isPrevalent = 0"_s ? > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:575 > + ASSERT_NOT_REACHED(); Shouldn't this return early? > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:579 > + auto newDomainID = findTopFrames.getColumnInt(0); int ?
Created attachment 379274 [details] Patch
The WinCairo and Mac-wk2 failures do not seem related to this patch. I am hitting the same test failures (on Mac-wk2) without this change.
Comment on attachment 379274 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379274&action=review I think this looks great! I did realize while reviewing that we log a number of errors that could leak private data in the form of visited domains. Please switch to using RELEASE_LOG_IF_ALLOWED for messages that involve the domain strings. r- to fix the log statements, but everything else looks great! > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:389 > RELEASE_LOG_ERROR(Network, "%p - ResourceLoadStatisticsDatabaseStore::m_insertDomainRelationshipStatement failed to bind, error message: %{public}s", this, m_database.lastErrorMsg()); This might leak private data (the secondDomain.string() could be interesting to someone). I recommend switching to RELEASE_LOG_IF_ALLOWED (see later review comment for details). > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:394 > + bool relationshipExists = !!statement.getColumnInt(0); Oh boy. I think I did that. :-| > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:451 > RELEASE_LOG_ERROR(Network, "%p - ResourceLoadStatisticsDatabaseStore::domainIDFromString failed, error message: %{public}s", this, m_database.lastErrorMsg()); RELEASE_LOG_IF_ALLOWED > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:579 > + RELEASE_LOG_ERROR(Network, "%p - ResourceLoadStatisticsDatabaseStore::recursivelyFindNonPrevalentDomainsThatRedirectedToThisDomain failed, error message: %{public}s", this, m_database.lastErrorMsg()); This could be a privacy issue, since the JOIN result will contain the strings of the visited domains. Please switch to 'RELEASE_LOG_IF_ALLOWED" (see WebCore/Modules/applepay/PaymentCoordinator.cpp for an example) > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:981 > + if (m_mostRecentUserInteractionStatement.bindInt(1, hadUserInteraction) != SQLITE_OK Good catch! > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1024 > + RELEASE_LOG_ERROR(Network, "%p - ResourceLoadStatisticsDatabaseStore::m_hadUserInteractionStatement failed, error message: %{public}s", this, m_database.lastErrorMsg()); RELEASE_LOG_IF_ALLOWED (because of domain.string()) > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1121 > RELEASE_LOG_ERROR(Network, "%p - ResourceLoadStatisticsDatabaseStore::predicateValueForDomain failed to bind, error message: %{public}s", this, m_database.lastErrorMsg()); Ditto RELEASE_LOG_IF_ALLOWED, since the log message might show the domain that was bound to the predicate. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1291 > + RELEASE_LOG_ERROR(Network, "%p - ResourceLoadStatisticsDatabaseStore::ensureResourceStatisticsForRegistrableDomain failed, error message: %{public}s", this, m_database.lastErrorMsg()); RELEASE_LOG_IF_ALLOWED > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1346 > + || statement.bindText(1, domain.string()) != SQLITE_OK Check error messages related to this statement (domain.string()) and use RELEASE_LOG_IF_ALLOWED if logging.
Created attachment 379487 [details] Patch
It looks like your patch is out-of-sync with trunk: In file included from /Volumes/Data/worker/iOS-13-Build-EWS/build/WebKitBuild/Release-iphoneos/DerivedSources/WebKit2/unified-sources/UnifiedSource45.cpp:5: /Volumes/Data/worker/iOS-13-Build-EWS/build/Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:274:35: error: no member named 'websiteDataStore' in 'WebKit::WebsiteDataStore' WebKit::toImpl(dataStoreRef)->websiteDataStore().setUseITPDatabase(value); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ 1 error generated.
Created attachment 379505 [details] Patch
Comment on attachment 379505 [details] Patch Awesome. Thank you! r=me
Comment on attachment 379505 [details] Patch Clearing flags on attachment: 379505 Committed r250324: <https://trac.webkit.org/changeset/250324>
All reviewed patches have been landed. Closing bug.
All Mac WK2 test buildbots are failing since this change. Apple High Sierra Debug WK2 (Tests) https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK2%20%28Tests%29/builds/9636 Apple High Sierra Release WK2 (Tests) https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/13308 Apple Mojave Debug WK2 (Tests) https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK2%20%28Tests%29/builds/4885 Apple Mojave Release WK2 (Tests) https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK2%20%28Tests%29/builds/6901
The test failures seem to be due to WKTR not outputting text output. It seems to be writing out the render tree.
This breaks the mac WK2 EWS as well (https://ews-build.webkit.org/#/builders/15). With so many test failures on trunk, EWS can't determine if a patch introduce any new test failure.
Rolled out in r250344: <https://trac.webkit.org/changeset/250344> since this broke a ton of testing....looks like EWS had flagged the change too https://ews-build.webkit.org/#/builders/15/builds/3975
Reopening to attach new patch.
Created attachment 379565 [details] Patch
Sorry about this... I should have been more aware of why the mac-wk2 tests were failing. I think this patch should fix it, but I’m going to leave the review flag off until we see what the bots say.
Created attachment 379582 [details] Patch for landing
(In reply to Katherine_cheney from comment #48) > Sorry about this... I should have been more aware of why the mac-wk2 tests > were failing. > > I think this patch should fix it, but I’m going to leave the review flag off > until we see what the bots say. We need to see why having the sandbox consumption step here caused this issue. I'm worried that we may not cleanly destroy these extensions on the UIProcess side when the client side (WebContent or Network processes) shut down.
Comment on attachment 379582 [details] Patch for landing r=me. Relanding after correcting test issue.
Comment on attachment 379582 [details] Patch for landing Rejecting attachment 379582 [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-03', 'apply-attachment', '--no-update', '--non-interactive', 379582, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 5000 characters of output: s/http/tests/resourceLoadStatistics/prevalent-resource-with-user-interaction-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-with-user-interaction-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-with-user-interaction-timeout-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-with-user-interaction-timeout-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-without-user-interaction-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-without-user-interaction-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/prune-statistics-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/prune-statistics-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-iframe-redirect-ip-to-localhost-to-ip-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-iframe-redirect-ip-to-localhost-to-ip-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-iframe-redirect-localhost-to-ip-to-localhost-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-iframe-redirect-localhost-to-ip-to-localhost-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-nesting-iframe-with-non-sandboxed-iframe-redirect-ip-to-localhost-to-ip-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-nesting-iframe-with-non-sandboxed-iframe-redirect-ip-to-localhost-to-ip-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-nesting-iframe-with-non-sandboxed-iframe-redirect-localhost-to-ip-to-localhost-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-nesting-iframe-with-non-sandboxed-iframe-redirect-localhost-to-ip-to-localhost-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-nesting-iframe-with-sandboxed-iframe-redirect-ip-to-localhost-to-ip-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-nesting-iframe-with-sandboxed-iframe-redirect-ip-to-localhost-to-ip-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-nesting-iframe-with-sandboxed-iframe-redirect-localhost-to-ip-to-localhost-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-nesting-iframe-with-sandboxed-iframe-redirect-localhost-to-ip-to-localhost-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/set-custom-prevalent-resource-in-debug-mode-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/set-custom-prevalent-resource-in-debug-mode-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/strip-referrer-to-origin-for-prevalent-subresource-redirects-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/strip-referrer-to-origin-for-prevalent-subresource-redirects-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/strip-referrer-to-origin-for-prevalent-subresource-requests-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/strip-referrer-to-origin-for-prevalent-subresource-requests-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/switch-session-on-navigation-to-prevalent-with-interaction-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/switch-session-on-navigation-to-prevalent-with-interaction-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/telemetry-generation.html patching file LayoutTests/http/tests/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/user-interaction-reported-after-website-data-removal-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/user-interaction-reported-after-website-data-removal-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html patching file LayoutTests/platform/ios/TestExpectations Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Brent Fulgham']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: https://webkit-queues.webkit.org/results/13070834
Created attachment 379649 [details] Patch for landing
Comment on attachment 379649 [details] Patch for landing r=me (rebased patch upload)
Comment on attachment 379649 [details] Patch for landing Rejecting attachment 379649 [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-03', 'apply-attachment', '--no-update', '--non-interactive', 379649, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 5000 characters of output: s/http/tests/resourceLoadStatistics/prevalent-resource-with-user-interaction-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-with-user-interaction-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-with-user-interaction-timeout-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-with-user-interaction-timeout-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-without-user-interaction-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-without-user-interaction-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/prune-statistics-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/prune-statistics-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-iframe-redirect-ip-to-localhost-to-ip-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-iframe-redirect-ip-to-localhost-to-ip-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-iframe-redirect-localhost-to-ip-to-localhost-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-iframe-redirect-localhost-to-ip-to-localhost-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-nesting-iframe-with-non-sandboxed-iframe-redirect-ip-to-localhost-to-ip-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-nesting-iframe-with-non-sandboxed-iframe-redirect-ip-to-localhost-to-ip-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-nesting-iframe-with-non-sandboxed-iframe-redirect-localhost-to-ip-to-localhost-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-nesting-iframe-with-non-sandboxed-iframe-redirect-localhost-to-ip-to-localhost-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-nesting-iframe-with-sandboxed-iframe-redirect-ip-to-localhost-to-ip-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-nesting-iframe-with-sandboxed-iframe-redirect-ip-to-localhost-to-ip-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-nesting-iframe-with-sandboxed-iframe-redirect-localhost-to-ip-to-localhost-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/sandboxed-nesting-iframe-with-sandboxed-iframe-redirect-localhost-to-ip-to-localhost-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/set-custom-prevalent-resource-in-debug-mode-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/set-custom-prevalent-resource-in-debug-mode-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/strip-referrer-to-origin-for-prevalent-subresource-redirects-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/strip-referrer-to-origin-for-prevalent-subresource-redirects-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/strip-referrer-to-origin-for-prevalent-subresource-requests-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/strip-referrer-to-origin-for-prevalent-subresource-requests-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/switch-session-on-navigation-to-prevalent-with-interaction-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/switch-session-on-navigation-to-prevalent-with-interaction-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/telemetry-generation.html patching file LayoutTests/http/tests/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/user-interaction-reported-after-website-data-removal-database-expected.txt patching file LayoutTests/http/tests/resourceLoadStatistics/user-interaction-reported-after-website-data-removal-database.html patching file LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html patching file LayoutTests/platform/ios/TestExpectations Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Brent Fulgham']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: https://webkit-queues.webkit.org/results/13070992
Created attachment 379658 [details] Patch
Comment on attachment 379658 [details] Patch Another attempt to land! r=me
Comment on attachment 379658 [details] Patch Clearing flags on attachment: 379658 Committed r250393: <https://trac.webkit.org/changeset/250393>