Bug 211004 – Create a mechanism to add missing ITP Database tables when the schema is updated

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

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.

[Kate Cheney]

<rdar://problem/62261187>

[Kate Cheney]

Created attachment 397708 [details]
Patch

[Kate Cheney]

Created attachment 397714 [details]
Patch

[Kate Cheney]

Created attachment 397733 [details]
Patch

[John Wilander]

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

[Kate Cheney]

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?

[Kate Cheney]

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.

[Kate Cheney]

Created attachment 397863 [details]
Patch for landing

[EWS]

Committed r260841: <https://trac.webkit.org/changeset/260841>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397863 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62538236>