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
Kate Cheney
2020-04-24 16:27:53 PDT
Created attachment 397708 [details]
Patch
Created attachment 397714 [details]
Patch
Created attachment 397733 [details]
Patch
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 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 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. Created attachment 397863 [details]
Patch for landing
Committed r260841: <https://trac.webkit.org/changeset/260841> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397863 [details]. |