Bug 234281 – PCM: Remove old DB update and migration code, and add a unit test for destination token DB columns

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

[John Wilander]

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.

[Radar WebKit Bug Importer]

<rdar://problem/86443135>

[John Wilander]

Created attachment 447089 [details]
Patch

[John Wilander]

Thanks, Alex! I'll wait for Kate to comment before landing since this patch removes a bunch of code she wrote.

[Kate Cheney]

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.

[John Wilander]

(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.

[John Wilander]

Created attachment 447099 [details]
Patch

[John Wilander]

Comment on attachment 447099 [details]
Patch

Waiting for green EWS before landing.

[John Wilander]

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.

[EWS]

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].

[Kate Cheney]

(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.

[Ryan Haddad]

Rebaselined tests for iOS in https://trac.webkit.org/changeset/287102/webkit