Bug 203224 – Implement dumpResourceLoadStatistics in SQLite ITP Database

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

dumpResourceLoadStatistics is not implemented in the ITP SQLite database yet.

[Radar WebKit Bug Importer]

<rdar://problem/56482165>

[Kate Cheney]

Created attachment 381590 [details]
Patch

[EWS Watchlist]

Comment on attachment 381590 [details]
Patch

Attachment 381590 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13164679

New failing tests:
fast/repaint/backgroundSizeRepaint.html

[EWS Watchlist]

Created attachment 381606 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Kate Cheney]

Created attachment 381608 [details]
Patch

[John Wilander]

Comment on attachment 381590 [details]
Patch

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

Overall good. See comments. I'd like someone with a bit more SQLite experience to look at those pieces.

> Source/WebKit/ChangeLog:50
> +        for this domain still exist in the table.

Good. Let's have the new name spelled out here in the change log. Did you change it in the memory store too?

I believe the new name is setIsScheduledForWebsiteDataRemoval and I wonder if we should make it clear that it's only used for non-cookie website data as of now? I prefer that and a subsequent name change over too broad of a name now.

> Source/WebKit/ChangeLog:57
> +        delete should check all domains, not just prevalent ones.

Correct.

> Source/WebKit/ChangeLog:62
> +        no longer guaranteed.

Hmm. We're still only removing non-cookie website data for non-prevalent resources, right? I'm thinking about the name shouldRemoveAllWebsiteDataFor.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:200
> +static const String v1ObservedDomainsTableSchema()

I may behind on our naming schemes. Is this a function name with a version in it? If so, it looks like version 10 with the capital O? If we want the version in the name we could put it at the end instead.

Could the version instead be a constant and a function to retrieve it? I'm thinking observedDomainsTableSchema(enum version).

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:289
> +        // If there is no ObservedDomains table at all, or there is an error executing the fetch, delete the file

Missing period.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:301
> +    ASSERT(!currentSchema.isEmpty());

Could this be a RELEASE_ASSERT? How bad would it be if the schema is not empty?

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:303
> +    // If the schema in the ResourceLoadStatistics directory is not the current schema, delete the database file

Missing period.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2003
> +    return resourceStatistic.isScheduledForWebsiteDataRemoval && !hasHadUnexpiredRecentUserInteraction(resourceStatistic, OperatingDatesWindow::Short) && (!shouldCheckForGrandfathering || !resourceStatistic.grandfathered);

Good to see this implemented!

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:135
> +    void resourceToString(StringBuilder&, String);

Can the String be const String&? Can the function be const?

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:140
> +    String getDomainStringFromDomainID(unsigned);

Can this be a const function? I don't know if the SQLite stuff prohibits that.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:141
> +    String getSubStatisticStatement(const String&);

Ditto, can it be const?

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:142
> +    void appendSubStatisticList(StringBuilder&, const String& tableName, String& domain);

Can domain be const?

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:176
> +    bool hasHadUnexpiredRecentUserInteraction(const DomainData&, OperatingDatesWindow);

Can this function be const?

> LayoutTests/ChangeLog:10
> +        for database store.

Were there any meaningful changes needed? If so, point them out. If not, point that out. :)

[Kate Cheney]

Comment on attachment 381590 [details]
Patch

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

Thanks for looking over this John! Yes to all the const qualifiers, thanks for pointing those out. I think the big questions are

1. is the current database functionality for shouldRemoveAllWebsiteDataFor and shouldRemoveAllButCookiesFor as its written in this patch look okay to you?

2. What is the best way to implement versioning for the database? This might be a question better suited for Brady

>> Source/WebKit/ChangeLog:50
>> +        for this domain still exist in the table.
> 
> Good. Let's have the new name spelled out here in the change log. Did you change it in the memory store too?
> 
> I believe the new name is setIsScheduledForWebsiteDataRemoval and I wonder if we should make it clear that it's only used for non-cookie website data as of now? I prefer that and a subsequent name change over too broad of a name now.

Sounds good, so maybe something like "isScheduledForAllButCookieDataRemoval?

>> Source/WebKit/ChangeLog:62
>> +        no longer guaranteed.
> 
> Hmm. We're still only removing non-cookie website data for non-prevalent resources, right? I'm thinking about the name shouldRemoveAllWebsiteDataFor.

I'm not sure I follow. Right now, shouldRemoveAllWebsiteDataFor requires the resource to be prevalent, but shouldRemoveAllButCookiesFor does not. Is this correct behavior?

>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:200
>> +static const String v1ObservedDomainsTableSchema()
> 
> I may behind on our naming schemes. Is this a function name with a version in it? If so, it looks like version 10 with the capital O? If we want the version in the name we could put it at the end instead.
> 
> Could the version instead be a constant and a function to retrieve it? I'm thinking observedDomainsTableSchema(enum version).

I can CC Brady Eidson to check over the SQL stuff, but he suggested avoiding any version numbers and checking the create table commands themselves to be sure that there's definitely no crashes if the version number somehow gets messed up.  I agree the name looks funny, I can definitely change it :)

>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:301
>> +    ASSERT(!currentSchema.isEmpty());
> 
> Could this be a RELEASE_ASSERT? How bad would it be if the schema is not empty?

If it is empty, it won't equal v1ObservedDomainsTableSchema() and a new database/schema will be created below, so I think its ok. But it does indicate something weird is going on with the statement, so I definitely think an assert is necessary for debug mode at least

[John Wilander]

(In reply to Katherine_cheney from comment #7)
> Comment on attachment 381590 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381590&action=review
> 
> Thanks for looking over this John! Yes to all the const qualifiers, thanks
> for pointing those out. I think the big questions are
> 
> 1. is the current database functionality for shouldRemoveAllWebsiteDataFor
> and shouldRemoveAllButCookiesFor as its written in this patch look okay to
> you?

It does.

> 2. What is the best way to implement versioning for the database? This might
> be a question better suited for Brady

Agreed, Brady should know.

> >> Source/WebKit/ChangeLog:50
> >> +        for this domain still exist in the table.
> > 
> > Good. Let's have the new name spelled out here in the change log. Did you change it in the memory store too?
> > 
> > I believe the new name is setIsScheduledForWebsiteDataRemoval and I wonder if we should make it clear that it's only used for non-cookie website data as of now? I prefer that and a subsequent name change over too broad of a name now.
> 
> Sounds good, so maybe something like "isScheduledForAllButCookieDataRemoval?

Exactly.

> >> Source/WebKit/ChangeLog:62
> >> +        no longer guaranteed.
> > 
> > Hmm. We're still only removing non-cookie website data for non-prevalent resources, right? I'm thinking about the name shouldRemoveAllWebsiteDataFor.
> 
> I'm not sure I follow. Right now, shouldRemoveAllWebsiteDataFor requires the
> resource to be prevalent, but shouldRemoveAllButCookiesFor does not. Is this
> correct behavior?

Got it. I got worried that "This now needs to check if the resource is prevalent, because it is no longer guaranteed" indicated that both prevalent and non-prevalent domains might be subjected to all-data removal. But it sounds like it's the opposite, which is correct.

> >> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:200
> >> +static const String v1ObservedDomainsTableSchema()
> > 
> > I may behind on our naming schemes. Is this a function name with a version in it? If so, it looks like version 10 with the capital O? If we want the version in the name we could put it at the end instead.
> > 
> > Could the version instead be a constant and a function to retrieve it? I'm thinking observedDomainsTableSchema(enum version).
> 
> I can CC Brady Eidson to check over the SQL stuff, but he suggested avoiding
> any version numbers and checking the create table commands themselves to be
> sure that there's definitely no crashes if the version number somehow gets
> messed up.  I agree the name looks funny, I can definitely change it :)

Good.

> >> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:301
> >> +    ASSERT(!currentSchema.isEmpty());
> > 
> > Could this be a RELEASE_ASSERT? How bad would it be if the schema is not empty?
> 
> If it is empty, it won't equal v1ObservedDomainsTableSchema() and a new
> database/schema will be created below, so I think its ok. But it does
> indicate something weird is going on with the statement, so I definitely
> think an assert is necessary for debug mode at least

Got it. Sounds fine as-is then.

If/when Brady is happy with the DB parts, ping me and I'll r+.

[Brady Eidson]

(In reply to John Wilander from comment #6)

> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:200
> > +static const String v1ObservedDomainsTableSchema()
> 
> I may behind on our naming schemes. Is this a function name with a version
> in it? If so, it looks like version 10 with the capital O? If we want the
> version in the name we could put it at the end instead.
> 
> Could the version instead be a constant and a function to retrieve it? I'm
> thinking observedDomainsTableSchema(enum version).

I wouldn't spend too much time trying to future-proof-engineer this. Things get messier than this ideal the moment you have more than one version.

I had pointed Kate to SQLiteIDBBackingStore which quite well establishes this exact naming scheme. But simply because the 'O' hadn't caused the readability problem!

I'm totally happy with putting v0 at the end:
observedDomainsTableSchemaV0(...);

[John Wilander]

(In reply to Brady Eidson from comment #9)
> (In reply to John Wilander from comment #6)
> 
> > 
> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:200
> > > +static const String v1ObservedDomainsTableSchema()
> > 
> > I may behind on our naming schemes. Is this a function name with a version
> > in it? If so, it looks like version 10 with the capital O? If we want the
> > version in the name we could put it at the end instead.
> > 
> > Could the version instead be a constant and a function to retrieve it? I'm
> > thinking observedDomainsTableSchema(enum version).
> 
> I wouldn't spend too much time trying to future-proof-engineer this. Things
> get messier than this ideal the moment you have more than one version.
> 
> I had pointed Kate to SQLiteIDBBackingStore which quite well establishes
> this exact naming scheme. But simply because the 'O' hadn't caused the
> readability problem!
> 
> I'm totally happy with putting v0 at the end:
> observedDomainsTableSchemaV0(...);

OK, then we're good with that change. Marking r+ but please address the comments before landing.

[John Wilander]

Comment on attachment 381608 [details]
Patch

Let me know when to cq+.

[Brent Fulgham]

Comment on attachment 381608 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:84
> +constexpr auto topFrameLinkDecorationsFromQuery = "INSERT OR IGNORE INTO TopFrameLinkDecorationsFrom (toDomainID, fromDomainID) SELECT ?, domainID FROM ObservedDomains WHERE registrableDomain in ( "_s;

Oops! I guess this was inconsistent with the other orderings.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:304
> +    if (currentSchema != v1ObservedDomainsTableSchema()) {

Is this based on something Brady recommended? I would have thought a simple integer value would have been sufficient (but perhaps easy to get out of sync with the database).

Do we need to do any case filtering or whitespace handling here? I'm not sure if the return value from the query is guaranteed to be string-comparison-identical on all platforms/language settings.

[Kate Cheney]

(In reply to Brent Fulgham from comment #12)
> Comment on attachment 381608 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381608&action=review
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:84
> > +constexpr auto topFrameLinkDecorationsFromQuery = "INSERT OR IGNORE INTO TopFrameLinkDecorationsFrom (toDomainID, fromDomainID) SELECT ?, domainID FROM ObservedDomains WHERE registrableDomain in ( "_s;
> 
> Oops! I guess this was inconsistent with the other orderings.
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:304
> > +    if (currentSchema != v1ObservedDomainsTableSchema()) {
> 
> Is this based on something Brady recommended? I would have thought a simple
> integer value would have been sufficient (but perhaps easy to get out of
> sync with the database).

Yes, he recommended it based on the SQLiteIDBBackingStore for that exact reason. 
> 
> Do we need to do any case filtering or whitespace handling here? I'm not
> sure if the return value from the query is guaranteed to be
> string-comparison-identical on all platforms/language settings.

Good point! I talked with Brady and he said some platforms include quotes around the table names and some don't. I'll make a change to account for that then upload a new patch with all the changes

[Kate Cheney]

Created attachment 381696 [details]
Patch

[Kate Cheney]

Sorry for the delay, I wanted to make sure everything was working on iOS too. One thing to note for the future -- the create table queries stored in the sql table do not have semi-colons, so the schema we compare against also should not have semi-colons to make sure we aren't overwriting the database on each open!

[John Wilander]

Comment on attachment 381696 [details]
Patch

👍🏼

[WebKit Commit Bot]

Comment on attachment 381696 [details]
Patch

Clearing flags on attachment: 381696

Committed r251501: <https://trac.webkit.org/changeset/251501>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

The new test http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-database.html

added in https://trac.webkit.org/changeset/251501/webkit

is failing often on iOS

history:
https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2FresourceLoadStatistics%2Fwebsite-data-removal-for-site-navigated-to-with-link-decoration-database.html

Diff:
--- /Volumes/Data/slave/ios-simulator-13-release-tests-wk2/build/layout-test-results/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-database-expected.txt
+++ /Volumes/Data/slave/ios-simulator-13-release-tests-wk2/build/layout-test-results/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-database-actual.txt
@@ -10,7 +10,7 @@
 After deletion: Client-side cookie exists.
 After deletion: Regular server-side cookie exists.
 After deletion: LocalStorage entry does not exist.
-After deletion: IDB entry does not exist.
+Before deletion: IDB entry does not exist.
 
 
 Resource load statistics:

[Kate Cheney]

(In reply to Truitt Savell from comment #19)
> The new test
> http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-
> with-link-decoration-database.html
> 
> added in https://trac.webkit.org/changeset/251501/webkit
> 
> is failing often on iOS
> 
> history:
> https://results.webkit.org/?suite=layout-
> tests&test=http%2Ftests%2FresourceLoadStatistics%2Fwebsite-data-removal-for-
> site-navigated-to-with-link-decoration-database.html
> 
> Diff:
> ---
> /Volumes/Data/slave/ios-simulator-13-release-tests-wk2/build/layout-test-
> results/http/tests/resourceLoadStatistics/website-data-removal-for-site-
> navigated-to-with-link-decoration-database-expected.txt
> +++
> /Volumes/Data/slave/ios-simulator-13-release-tests-wk2/build/layout-test-
> results/http/tests/resourceLoadStatistics/website-data-removal-for-site-
> navigated-to-with-link-decoration-database-actual.txt
> @@ -10,7 +10,7 @@
>  After deletion: Client-side cookie exists.
>  After deletion: Regular server-side cookie exists.
>  After deletion: LocalStorage entry does not exist.
> -After deletion: IDB entry does not exist.
> +Before deletion: IDB entry does not exist.
>  
>  
>  Resource load statistics:


Saw this yesterday, fix about to be committed! See bug:

https://bugs.webkit.org/show_bug.cgi?id=203542