New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 831316 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 0
Type: Bug



Sign in to add a comment

IndexedDB: Upgrade all v2 schemas that don't have blobs

Project Member Reported by dmu...@chromium.org, Apr 10 2018

Issue description

To help mitigate all possible problems, we need to upgrade all v2 IndexedDB schema databases that don't have blob keys ASAP.

This bug tracks that change, and I really need it to go in m66.
 
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 11 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a8aac34e30f00003bca4dbff4501705cd3cabfc8

commit a8aac34e30f00003bca4dbff4501705cd3cabfc8
Author: Daniel Murphy <dmurph@chromium.org>
Date: Wed Apr 11 17:58:01 2018

[IndexedDB] Update v2 schema to v3 for uncorrupt stores, fix enums.xml

For the last 4 years we 'saved' the new schema version number, so any v2
databases are never marked as v3. The 'migration' always deleted all
blob files.

This correctly upgrades v2 databases to v3 if they have no corrupt
blobs. To gather more information we don't blob away corrupt blob
backing stores yet, and instead log when we encounter them.

This also fixes the enums.xml values for IDB setup errors, which have
been missing 'DeleteIndex' and 'ClearObjectStore' (and subsequently have
had incorrect reporting on values > 23) since 2014:
https://crrev.com/b05831ee

Bug:  831316 
Change-Id: Ifa11292b6337be09184de1eed1c667cb99057a96
Reviewed-on: https://chromium-review.googlesource.com/1005984
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549928}
[modify] https://crrev.com/a8aac34e30f00003bca4dbff4501705cd3cabfc8/content/browser/indexed_db/indexed_db_backing_store.cc
[modify] https://crrev.com/a8aac34e30f00003bca4dbff4501705cd3cabfc8/content/browser/indexed_db/indexed_db_backing_store.h
[modify] https://crrev.com/a8aac34e30f00003bca4dbff4501705cd3cabfc8/content/browser/indexed_db/indexed_db_backing_store_unittest.cc
[modify] https://crrev.com/a8aac34e30f00003bca4dbff4501705cd3cabfc8/content/browser/indexed_db/indexed_db_leveldb_operations.cc
[modify] https://crrev.com/a8aac34e30f00003bca4dbff4501705cd3cabfc8/content/browser/indexed_db/indexed_db_leveldb_operations.h
[modify] https://crrev.com/a8aac34e30f00003bca4dbff4501705cd3cabfc8/content/browser/indexed_db/indexed_db_metadata_coding.cc
[modify] https://crrev.com/a8aac34e30f00003bca4dbff4501705cd3cabfc8/content/browser/indexed_db/indexed_db_metadata_coding.h
[modify] https://crrev.com/a8aac34e30f00003bca4dbff4501705cd3cabfc8/content/browser/indexed_db/indexed_db_reporting.cc
[modify] https://crrev.com/a8aac34e30f00003bca4dbff4501705cd3cabfc8/content/browser/indexed_db/indexed_db_reporting.h
[modify] https://crrev.com/a8aac34e30f00003bca4dbff4501705cd3cabfc8/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/a8aac34e30f00003bca4dbff4501705cd3cabfc8/tools/metrics/histograms/histograms.xml

Comment 3 by dmu...@chromium.org, Apr 11 2018

Labels: Merge-Request-66
Project Member

Comment 4 by sheriffbot@chromium.org, Apr 11 2018

Labels: -Merge-Request-66 Merge-Review-66 Hotlist-Merge-Review
This bug requires manual review: We are only 5 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 5 by dmu...@chromium.org, Apr 11 2018

Context:

There is a bug in IndexedDB (used by customers like Docs to save offline docs) where v2 schemas were never 'saved' as upgraded to v3. That, plus the 'migration' involved deleting all the 'blob' data stored in the database.

1. We stopped deleting all blob data in v2 databases, but we can't do the 'upgrade' to v3 yet because we have a bunch of corrupt v2 databases, and we only know they are corrupt because they are both v2 and have blob keys

2. We need to gather stats to see how many people have v2 vs v3 databases (this merge already happened), and we need to gather stats on how many people have 'corrupt' v2 databases (blob keys) vs 'non-corrupt' v2 databases

3. The last bug is the 'corrupt vs non corrupt' stats bit, as well as upgrading the databases that are save to upgrade to v3 (don't have blob keys). There are going to be databases that were 'non-corrupt', but then added blobs in m66. These blobs won't be deleted because we fixed that bug, but there is no way for us to know that the database isn't corrupt. We just know that it has blob keys.

This change is the last bit of 2, and all of 3. Docs needs the non-corrupt vs corrupt stats bit so they know the impact on their customers - they don't want us to blow away the corrupt databases right away, because then they would potentially lose a lot of user's unsaved changes if they have stuff saved offline.

Comment 6 by dmu...@chromium.org, Apr 11 2018

Labels: OS-Android OS-Chrome OS-Mac OS-Windows

Comment 7 by dmu...@chromium.org, Apr 11 2018

I think this is safe to merge because:

1. I did unittests of all code paths - there was a unittest for the deleted blob codepath before as well, but since that's now a no-op I removed the unittest.
2. There are UMAs on all the paths here - we should see people hitting v2, v3, v2-with-blobs, and v2-without-blobs in UMA. If this isn't happening in the canary, then something is wrong and we can revert. (or, more specifically, if the v2-with-blobs isn't happening, then something is wrong. I suspect maybe almost every v2 database has blobs - but that's what we're trying to answer here)

Comment 8 by dmu...@chromium.org, Apr 11 2018

Also, most of the change is some un-templating, all of which is tested.
Cc: cma...@chromium.org
Ok thanks for the details. We'll need a postmortem for this. Since our last beta is tomorrow for desktop, I'd like to get this in so we can test. 

cmasso@ - any issues/concerns from your side?
Yes there will definitely be a postmortem.
Labels: -Merge-Review-66 Merge-Approved-66
Branch:3359
Project Member

Comment 13 by bugdroid1@chromium.org, Apr 11 2018

Labels: -merge-approved-66 merge-merged-3359
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ccbbe23ba0739767f6537bfffec553727453ed2c

commit ccbbe23ba0739767f6537bfffec553727453ed2c
Author: Daniel Murphy <dmurph@chromium.org>
Date: Wed Apr 11 20:22:29 2018

[IndexedDB] Update v2 schema to v3 for uncorrupt stores, fix enums.xml

For the last 4 years we 'saved' the new schema version number, so any v2
databases are never marked as v3. The 'migration' always deleted all
blob files.

This correctly upgrades v2 databases to v3 if they have no corrupt
blobs. To gather more information we don't blob away corrupt blob
backing stores yet, and instead log when we encounter them.

This also fixes the enums.xml values for IDB setup errors, which have
been missing 'DeleteIndex' and 'ClearObjectStore' (and subsequently have
had incorrect reporting on values > 23) since 2014:
https://crrev.com/b05831ee

Bug:  831316 
Change-Id: Ifa11292b6337be09184de1eed1c667cb99057a96
Reviewed-on: https://chromium-review.googlesource.com/1005984
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549928}
(cherry picked from commit a8aac34e30f00003bca4dbff4501705cd3cabfc8)

TBR=dmurph@chromium.org

	modified:   content/browser/indexed_db/indexed_db_backing_store.cc
	modified:   content/browser/indexed_db/indexed_db_backing_store.h
	modified:   content/browser/indexed_db/indexed_db_backing_store_unittest.cc
	modified:   content/browser/indexed_db/indexed_db_leveldb_operations.cc
	modified:   content/browser/indexed_db/indexed_db_leveldb_operations.h
	modified:   content/browser/indexed_db/indexed_db_metadata_coding.cc
	modified:   content/browser/indexed_db/indexed_db_metadata_coding.h
	modified:   content/browser/indexed_db/indexed_db_reporting.cc
	modified:   content/browser/indexed_db/indexed_db_reporting.h
	modified:   tools/metrics/histograms/enums.xml
	modified:   tools/metrics/histograms/histograms.xml

Change-Id: I1e3684c3d14a8b12d56cad2ddae27367979ffa39
Reviewed-on: https://chromium-review.googlesource.com/1008277
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#688}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/ccbbe23ba0739767f6537bfffec553727453ed2c/content/browser/indexed_db/indexed_db_backing_store.cc
[modify] https://crrev.com/ccbbe23ba0739767f6537bfffec553727453ed2c/content/browser/indexed_db/indexed_db_backing_store.h
[modify] https://crrev.com/ccbbe23ba0739767f6537bfffec553727453ed2c/content/browser/indexed_db/indexed_db_backing_store_unittest.cc
[modify] https://crrev.com/ccbbe23ba0739767f6537bfffec553727453ed2c/content/browser/indexed_db/indexed_db_leveldb_operations.cc
[modify] https://crrev.com/ccbbe23ba0739767f6537bfffec553727453ed2c/content/browser/indexed_db/indexed_db_leveldb_operations.h
[modify] https://crrev.com/ccbbe23ba0739767f6537bfffec553727453ed2c/content/browser/indexed_db/indexed_db_metadata_coding.cc
[modify] https://crrev.com/ccbbe23ba0739767f6537bfffec553727453ed2c/content/browser/indexed_db/indexed_db_metadata_coding.h
[modify] https://crrev.com/ccbbe23ba0739767f6537bfffec553727453ed2c/content/browser/indexed_db/indexed_db_reporting.cc
[modify] https://crrev.com/ccbbe23ba0739767f6537bfffec553727453ed2c/content/browser/indexed_db/indexed_db_reporting.h
[modify] https://crrev.com/ccbbe23ba0739767f6537bfffec553727453ed2c/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/ccbbe23ba0739767f6537bfffec553727453ed2c/tools/metrics/histograms/histograms.xml

Status: Fixed (was: Assigned)

Sign in to add a comment