Bug 234281

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 Flags
Patch
none
Patch none

Description John Wilander 2021-12-13 17:36:36 PST
WebKit::Database::needsUpdatedSchema() has a comment saying it should be removed late 2021. WebKit::DatabaseUtilities::migrateDataToNewTablesIfNecessary() returns directly if WebKit::Database::needsUpdatedSchema() returns false so WebKit::DatabaseUtilities::migrateDataToNewTablesIfNecessary() should be removed at the same time. Likewise the static WebKit::insertDistinctValuesInTableStatement() in DatabaseUtilities.cpp will no longer be needed.

We should also add a unit test for the new columns for attributed measurements with destination tokens.
Comment 1 Radar WebKit Bug Importer 2021-12-13 17:37:01 PST
<rdar://problem/86443135>
Comment 2 John Wilander 2021-12-13 17:47:11 PST
Created attachment 447089 [details]
Patch
Comment 3 John Wilander 2021-12-13 18:08:29 PST
Thanks, Alex! I'll wait for Kate to comment before landing since this patch removes a bunch of code she wrote.
Comment 4 Kate Cheney 2021-12-13 18:11:13 PST
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.
Comment 5 John Wilander 2021-12-13 18:22:24 PST
(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.
Comment 6 John Wilander 2021-12-13 20:13:17 PST
Created attachment 447099 [details]
Patch
Comment 7 John Wilander 2021-12-13 20:13:44 PST
Comment on attachment 447099 [details]
Patch

Waiting for green EWS before landing.
Comment 8 John Wilander 2021-12-13 23:50:19 PST
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.
Comment 9 EWS 2021-12-13 23:57:53 PST
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].
Comment 10 Kate Cheney 2021-12-14 05:59:19 PST
(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.
Comment 11 Ryan Haddad 2021-12-15 13:13:39 PST
Rebaselined tests for iOS in https://trac.webkit.org/changeset/287102/webkit