Bug 195420 – Enable LayoutTests using ResourceLoadStatistics SQLite backend

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

This bug tracks the work of getting the ITP-related LayoutTests working when running with the ResourceLoadStatistics database backend.

[Radar WebKit Bug Importer]

<rdar://problem/54213551>

[Kate Cheney]

Created attachment 378422 [details]
Patch

[Kate Cheney]

Created attachment 378489 [details]
Patch

[Kate Cheney]

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.

[Kate Cheney]

Created attachment 378649 [details]
Patch

[Kate Cheney]

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

[Chris Dumez]

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?

[Kate Cheney]

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

[Chris Dumez]

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

[Chris Dumez]

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

[Kate Cheney]

Created attachment 378891 [details]
Patch

[Chris Dumez]

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.

[Kate Cheney]

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

[Chris Dumez]

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

[Kate Cheney]

Created attachment 378893 [details]
Patch

[Chris Dumez]

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.

[Kate Cheney]

Created attachment 378906 [details]
Patch

[Chris Dumez]

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.

[Kate Cheney]

(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

[Chris Dumez]

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.

[Kate Cheney]

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

[Chris Dumez]

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

[Kate Cheney]

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

[Chris Dumez]

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

[Brent Fulgham]

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

[Kate Cheney]

Created attachment 379091 [details]
Patch

[Kate Cheney]

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

[Chris Dumez]

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

[Chris Dumez]

(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

[Chris Dumez]

Comment on attachment 379091 [details]
Patch

r- given the crash on EWS.

[Chris Dumez]

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 ?

[Kate Cheney]

Created attachment 379274 [details]
Patch

[Brent Fulgham]

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.

[Brent Fulgham]

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.

[Kate Cheney]

Created attachment 379487 [details]
Patch

[Brent Fulgham]

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.

[Kate Cheney]

Created attachment 379505 [details]
Patch

[Brent Fulgham]

Comment on attachment 379505 [details]
Patch

Awesome. Thank you! r=me

[WebKit Commit Bot]

Comment on attachment 379505 [details]
Patch

Clearing flags on attachment: 379505

Committed r250324: <https://trac.webkit.org/changeset/250324>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Fujii Hironori]

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

[Brent Fulgham]

The test failures seem to be due to WKTR not outputting text output. It seems to be writing out the render tree.

[Aakash Jain]

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.

[Jonathan Bedard]

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

[Kate Cheney]

Reopening to attach new patch.

[Kate Cheney]

Created attachment 379565 [details]
Patch

[Kate Cheney]

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.

[Kate Cheney]

Created attachment 379582 [details]
Patch for landing

[Brent Fulgham]

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

[Brent Fulgham]

Comment on attachment 379582 [details]
Patch for landing

r=me. Relanding after correcting test issue.

[WebKit Commit Bot]

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

[Kate Cheney]

Created attachment 379649 [details]
Patch for landing

[Brent Fulgham]

Comment on attachment 379649 [details]
Patch for landing

r=me (rebased patch upload)

[WebKit Commit Bot]

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

[Kate Cheney]

Created attachment 379658 [details]
Patch

[Brent Fulgham]

Comment on attachment 379658 [details]
Patch

Another attempt to land! r=me

[WebKit Commit Bot]

Comment on attachment 379658 [details]
Patch

Clearing flags on attachment: 379658

Committed r250393: <https://trac.webkit.org/changeset/250393>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.