Summary: | PCM: Remove old DB update and migration code, and add a unit test for destination token DB columns | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | John Wilander <wilander> | ||||||
Component: | WebKit Misc. | Assignee: | John Wilander <wilander> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | achristensen, katherine_cheney, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=234335 | ||||||||
Attachments: |
|
Description
John Wilander
2021-12-13 17:36:36 PST
Created attachment 447089 [details]
Patch
Thanks, Alex! I'll wait for Kate to comment before landing since this patch removes a bunch of code she wrote. Comment on attachment 447089 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=447089&action=review I'm still a bit fuzzy on what was causing the crash and how this will fix it. > Source/WebKit/NetworkProcess/DatabaseUtilities.cpp:-274 > -void DatabaseUtilities::migrateDataToNewTablesIfNecessary() Please keep this for ITP. It has shipped with schema changes and therefore needs migration code. PCM schema changes did not ship which is why it is OK to remove them. (In reply to Kate Cheney from comment #4) > Comment on attachment 447089 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=447089&action=review > > I'm still a bit fuzzy on what was causing the crash and how this will fix it. Agreed. Error message at the failed assertion was "DatabaseUtilities::migrateDataToNewTablesIfNecessary failed to rename table, error message: There is already another table or index with this name: _UnattributedPrivateClickMeasurement." > > Source/WebKit/NetworkProcess/DatabaseUtilities.cpp:-274 > > -void DatabaseUtilities::migrateDataToNewTablesIfNecessary() > > Please keep this for ITP. It has shipped with schema changes and therefore > needs migration code. PCM schema changes did not ship which is why it is OK > to remove them. Aha! Will do. Created attachment 447099 [details]
Patch
Comment on attachment 447099 [details]
Patch
Waiting for green EWS before landing.
mac-AS-debug-wk2 layout test failure is unrelated. ios-wk2 far behind and hasn't even started. api-ios is >50 minutes into running tests. Committed r287013 (245218@main): <https://commits.webkit.org/245218@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 447099 [details]. (In reply to John Wilander from comment #5) > (In reply to Kate Cheney from comment #4) > > Comment on attachment 447089 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=447089&action=review > > > > I'm still a bit fuzzy on what was causing the crash and how this will fix it. > > Agreed. Error message at the failed assertion was > "DatabaseUtilities::migrateDataToNewTablesIfNecessary failed to rename > table, error message: There is already another table or index with this > name: _UnattributedPrivateClickMeasurement." > This doesn't really address the root cause of the problem, which is that data migration failed to delete the altered tables at some point and then when trying again it crashes when it finds the existing tables. Instead this just leaves those altered tables around and stops migration. But I guess since existing tests are passing it's probably not a bug with ITP and this will be sufficient. One thing we could do to make this more robust in the future is check if any tables exist that we don't expect and delete them. > > > Source/WebKit/NetworkProcess/DatabaseUtilities.cpp:-274 > > > -void DatabaseUtilities::migrateDataToNewTablesIfNecessary() > > > > Please keep this for ITP. It has shipped with schema changes and therefore > > needs migration code. PCM schema changes did not ship which is why it is OK > > to remove them. > > Aha! Will do. Rebaselined tests for iOS in https://trac.webkit.org/changeset/287102/webkit |