Bug 211004

Summary: Create a mechanism to add missing ITP Database tables when the schema is updated
Product: WebKit Reporter: Kate Cheney <katherine_cheney>
Component: WebKit Misc.Assignee: Kate Cheney <katherine_cheney>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, bfulgham, webkit-bug-importer, wilander
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=211652
https://bugs.webkit.org/show_bug.cgi?id=211653
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Kate Cheney 2020-04-24 16:27:53 PDT
Currently, the database drops all tables and recreates the schema if any table is missing. This is simple, but should probably be updated to use a better approach of adding any missing tables to the database.
Comment 1 Kate Cheney 2020-04-24 16:29:22 PDT
<rdar://problem/62261187>
Comment 2 Kate Cheney 2020-04-27 11:48:56 PDT
Created attachment 397708 [details]
Patch
Comment 3 Kate Cheney 2020-04-27 12:24:11 PDT
Created attachment 397714 [details]
Patch
Comment 4 Kate Cheney 2020-04-27 14:06:02 PDT
Created attachment 397733 [details]
Patch
Comment 5 John Wilander 2020-04-28 10:23:29 PDT
Comment on attachment 397733 [details]
Patch

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

Please address my comments. Other than that, looks good!

> Source/WebKit/ChangeLog:62
> +        the new table wasn't added upon opening the database.

Your description of the test sounds like the code is still broken. Is it a description of the bug? If so, please rephrase so that the test is described to test fixed functionality. Something like "This tests that the database is not dropped when a new table is added upon opening the database."

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:368
> +void ResourceLoadStatisticsDatabaseStore::isCorrectTableSchema(Vector<String>& missingTables)

We should renamed this function now that it no longer returns a boolean. Would checkForMissingTablesInSchema() work? See my subsequent comment on this.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:478
> +            RELEASE_LOG_ERROR(Network, "%p - ResourceLoadStatisticsDatabaseStore::addMissingTables failed to execute, error message: %" PUBLIC_LOG_STRING, this, m_database.lastErrorMsg());

I assume you want to carry on trying here and not return early on an error.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:493
> +    isCorrectTableSchema(missingTables);

Seeing how this is used, I think it would be better to have a function like this Optional<Vector<String>> ResourceLoadStatisticsDatabaseStore::getMissingTablesInSchema().

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:329
> +            completionHandler(false);

We should call ASSERT_NOT_REACHED() here since such a call should never happen. It's risky since a call to completionHandler(false) will be interpreted as a passing test.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1593
> +            return;

This assumes there is only one network process. Is that what you want?

> Tools/ChangeLog:10
> +        the ITP database file, then ensures the pre-seeded data has been

ensures the pre-seeded data is …

> Tools/ChangeLog:11
> +        migrated over and that the schema has all tables (including the

the schema now has all tables …

> Tools/ChangeLog:12
> +        missing one).

previously missing one
Comment 6 Kate Cheney 2020-04-28 10:42:05 PDT
Comment on attachment 397733 [details]
Patch

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

Thanks John!

>> Source/WebKit/ChangeLog:62
>> +        the new table wasn't added upon opening the database.
> 
> Your description of the test sounds like the code is still broken. Is it a description of the bug? If so, please rephrase so that the test is described to test fixed functionality. Something like "This tests that the database is not dropped when a new table is added upon opening the database."

Agreed this wording is confusing, I will update it. Your suggestion sounds good.

>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:478
>> +            RELEASE_LOG_ERROR(Network, "%p - ResourceLoadStatisticsDatabaseStore::addMissingTables failed to execute, error message: %" PUBLIC_LOG_STRING, this, m_database.lastErrorMsg());
> 
> I assume you want to carry on trying here and not return early on an error.

Yes. I think a best-effort approach is what we've done in the past, which means trying to add as many tables as possible.

>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:493
>> +    isCorrectTableSchema(missingTables);
> 
> Seeing how this is used, I think it would be better to have a function like this Optional<Vector<String>> ResourceLoadStatisticsDatabaseStore::getMissingTablesInSchema().

Good idea.

>> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:329
>> +            completionHandler(false);
> 
> We should call ASSERT_NOT_REACHED() here since such a call should never happen. It's risky since a call to completionHandler(false) will be interpreted as a passing test.

Good idea, I never thought of that. I'll add it in.

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1593
>> +            return;
> 
> This assumes there is only one network process. Is that what you want?

You're right. This is what I see anywhere else in WebsiteDataStore where the completion handler returns a bool value (see hasLocalStorageForTesting, hasIsolatedSessionForTesting, isResourceLoadStatisticsEphemeral, isRegisteredAsSubresourceUnder, isRegisteredAsRedirectingTo, etc). I wonder if this is a subtle bug in the ITP test code?
Comment 7 Kate Cheney 2020-04-28 11:24:38 PDT
Comment on attachment 397733 [details]
Patch

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

>>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1593
>>> +            return;
>> 
>> This assumes there is only one network process. Is that what you want?
> 
> You're right. This is what I see anywhere else in WebsiteDataStore where the completion handler returns a bool value (see hasLocalStorageForTesting, hasIsolatedSessionForTesting, isResourceLoadStatisticsEphemeral, isRegisteredAsSubresourceUnder, isRegisteredAsRedirectingTo, etc). I wonder if this is a subtle bug in the ITP test code?

Thinking about it again, I believe we do want to assume there is only one network process. The test creates a single network process, and we don't need/want the possibility of multiple callbacks.
Comment 8 Kate Cheney 2020-04-28 11:54:47 PDT
Created attachment 397863 [details]
Patch for landing
Comment 9 EWS 2020-04-28 12:40:53 PDT
Committed r260841: <https://trac.webkit.org/changeset/260841>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397863 [details].
Comment 10 Radar WebKit Bug Importer 2020-04-28 12:41:23 PDT
<rdar://problem/62538236>