[Autofill profile] USS Migration causes autocomplete data to get filled in |
||||
Issue descriptionThis is a bug I never managed to repro locally. I've seen two pieces of evidence that suggest the problem can somewhere around the uss migrator: 1) Some users send two progress markers for the same type (autocomplete). It seems like we "import" the progress marker of autocomplete into the DB as the progress marker for autofill profile and later send it to the server. I've seen in locally (once, never managed to repro) and then in the experiment (errors of this kind peaked when the canary/dev experiment was started and faded out when it was stopped two days later). 2) Some users get metadata elements for autocomplete stored as elements for autofill_profile. This was clear in UMA (in order of hundreds), it did not happen for all users. I have one local profile where this happened as well and the elements have storage keys in the same format as autocomplete entries have it. Furthermore, it seems like an older copy of my autocomplete data from around March this year (when autocomplete USS was launched). I never managed to repro it locally (just switching on Directory sync to have directory filled up again, deleting USS metadata and switching back to USS did not work out as repro steps). Maybe there was a bug in the uss_migrator that it did not wipe the directory for autocomplete for some of our users and there is another bug that causes that we get that instead of autofill_profile content.
,
Oct 23
,
Oct 24
I think I've found the problem: AutofillTable::MigrateToVersion78AddModelTypeColumns() uses syncer::ModelTypeToHistogramInt to convert from ModelType to persisted index, but other places use AutofillTable::GetKeyValueForModelType which uses syncer::ModelTypeToStableIdentifier, which in turn again uses ModelTypeToHistogramInt *but adds 1* [1] to make the value strictly positive. So, AUTOFILL (=5) becomes AUTOFILL_PROFILE (=6). [1] https://cs.chromium.org/chromium/src/components/sync/syncable/model_type.cc?rcl=6b71572c6db6237ed340961a40b429ce6472817f&l=526
,
Oct 24
Oh, brilliant! We can discuss solutions & how to move forward (in other words who should do what) next Monday. But feel free to grab whatever task/CL in the mean-time. You deserve credit for this investigation!
,
Oct 24
Correction to the above: Actually AUTOFILL is 6 and AUTOFILL_PROFILE is 5. Fixing the actual problem in the code will be trivial. The question is if we need to (or even can) do anything about the messed-up data.
,
Oct 24
I think we need to MigrateToVersionN+1 and wipe the data for AUTOFILL_PROFILE before launching again. The fact that we've lost data for AUTOCOMPLETE in the last migration is no problem, it got re-fetched from the server.
,
Oct 24
Concurrent edits :) Right, I guess just wiping should be fine. The bad migration happened in Chrome 69, so at this point it's too late to save anything anyway. I'll prepare a CL, but not today :)
,
Oct 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1c7a9c59eba073b1c359d9abf379c164a81b2eb4 commit 1c7a9c59eba073b1c359d9abf379c164a81b2eb4 Author: Marc Treib <treib@chromium.org> Date: Tue Oct 30 07:32:26 2018 [Autofill] Fix badly-migrated Sync metadata from DB version 78 Version 78 (introduced in https://crrev.com/c/1063728) added support for storing metadata for multiple Sync ModelTypes. However, the migration code for existing Sync metadata (implicitly belonging to AUTOFILL) used the wrong ID when inserting the existing data into the updated tables. This CL fixes that by blowing away any data stored under the wrong ID. Note that the regular insert/update code (outside of migration) did use the correct ID, so we make sure to keep this data around. Bug: 895826 Change-Id: I41530c0a0918bdaefac2e65fd19d0e0c778da42c Reviewed-on: https://chromium-review.googlesource.com/c/1299140 Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org> Reviewed-by: Cait Phillips <caitkp@chromium.org> Reviewed-by: Jan Krcal <jkrcal@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#603822} [modify] https://crrev.com/1c7a9c59eba073b1c359d9abf379c164a81b2eb4/components/autofill/core/browser/webdata/autofill_table.cc [modify] https://crrev.com/1c7a9c59eba073b1c359d9abf379c164a81b2eb4/components/autofill/core/browser/webdata/autofill_table.h [add] https://crrev.com/1c7a9c59eba073b1c359d9abf379c164a81b2eb4/components/test/data/web_database/version_80.sql [modify] https://crrev.com/1c7a9c59eba073b1c359d9abf379c164a81b2eb4/components/webdata/common/BUILD.gn [modify] https://crrev.com/1c7a9c59eba073b1c359d9abf379c164a81b2eb4/components/webdata/common/web_database.cc [modify] https://crrev.com/1c7a9c59eba073b1c359d9abf379c164a81b2eb4/components/webdata/common/web_database_migration_unittest.cc
,
Oct 30
That should do it :) |
||||
►
Sign in to add a comment |
||||
Comment 1 by jkrcal@chromium.org
, Oct 16