RESOLVED FIXED 102243
IndexedDB: Check SSV version when opening backing store
https://bugs.webkit.org/show_bug.cgi?id=102243
Summary IndexedDB: Check SSV version when opening backing store
Joshua Bell
Reported 2012-11-14 08:53:28 PST
Scenario: (1) SSV is revved from 5 to 6 to support a new data type (e.g. uchar strings) (2) IndexedDB records are persisted to disk using the new type (3) User "downgrades" to an older browser version without clearing IDB profile directory (4) IndexedDB opens backing store - everything looks fine (5) When opening a record, SSV fails decoding the data type from the future - sadness Ideally: (1) SSV is revved from 5 to 6 to support a new data type (e.g. uchar strings) (2) IndexedDB records are persisted to disk using the new type (3) User "downgrades" to an older browser version without clearing IDB profile directory (4) IndexedDB opens backing store, and detects that the SSV version is not known (5) IndexedDB does its recovery stuff (just like if there was an schema change) We either need to always bump the IDB internal schema version when the SSV encoding changes, or have IDB record the SSV version and check it as well as its internal schema version. Or something equally clever. Doing this when a record is read is insufficient - this MUST be done at backing store open time or data corruption can occur.
Attachments
Patch (10.95 KB, patch)
2012-11-14 10:51 PST, Joshua Bell
no flags
Patch (11.00 KB, patch)
2012-11-14 11:41 PST, Joshua Bell
no flags
Patch (10.74 KB, patch)
2012-11-19 11:04 PST, Joshua Bell
no flags
Patch (10.84 KB, patch)
2012-12-06 15:58 PST, Joshua Bell
no flags
Patch (11.12 KB, patch)
2012-12-06 16:41 PST, Joshua Bell
no flags
Joshua Bell
Comment 1 2012-11-14 10:51:12 PST
Joshua Bell
Comment 2 2012-11-14 10:52:04 PST
First stab at this. Needs tests.
Joshua Bell
Comment 3 2012-11-14 10:53:45 PST
Comment on attachment 174203 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174203&action=review > Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:158 > +static const unsigned char MaxGlobalMetaDataSimpleTypeByte = 3; This name sucks, since it looks like a regular piece of metadata - suggestions? > Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:864 > + if (typeByteA <= MaxGlobalMetaDataSimpleTypeByte) Here's where it's used, replacing a magic number. > Source/WebCore/bindings/js/SerializedScriptValue.cpp:1990 > +uint32_t SerializedScriptValue::wireFormatVersion() Alternate naming suggestions appreciated... > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2351 > + return WebCore::wireFormatVersion; ... especially due to this name conflict; would be nice.
Early Warning System Bot
Comment 4 2012-11-14 10:59:18 PST
kov's GTK+ EWS bot
Comment 5 2012-11-14 11:21:52 PST
EFL EWS Bot
Comment 6 2012-11-14 11:22:41 PST
Build Bot
Comment 7 2012-11-14 11:35:48 PST
Joshua Bell
Comment 8 2012-11-14 11:39:23 PST
Better handling of the metadata comparisons (and a bug fix) is introduced in http://webkit.org/b/102255 - that should land first and this should be rebased.
Joshua Bell
Comment 9 2012-11-14 11:41:38 PST
Joshua Bell
Comment 10 2012-11-14 11:42:01 PST
Let's try putting that inside the namespace { }
David Grogan
Comment 11 2012-11-14 14:03:26 PST
Comment on attachment 174213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174213&action=review > Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:864 > + if (typeByteA <= MaxGlobalMetaDataSimpleTypeByte) should be strictly less than? Though I suppose that only changes behavior in debug mode.
Joshua Bell
Comment 12 2012-11-14 14:04:33 PST
(In reply to comment #11) > (From update of attachment 174213 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174213&action=review > > > Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:864 > > + if (typeByteA <= MaxGlobalMetaDataSimpleTypeByte) > > should be strictly less than? Though I suppose that only changes behavior in debug mode. You're right, that's a typo. Over in https://bugs.webkit.org/show_bug.cgi?id=102255 I think I have it correct...
Joshua Bell
Comment 13 2012-11-19 11:04:10 PST
Joshua Bell
Comment 14 2012-11-19 11:10:45 PST
Rebased now that 102255 has landed.
Joshua Bell
Comment 15 2012-11-19 11:11:49 PST
This still needs tests, obviously.
Joshua Bell
Comment 16 2012-12-06 15:58:10 PST
Joshua Bell
Comment 17 2012-12-06 15:59:00 PST
Chromium-side test in review at: https://codereview.chromium.org/11470013/ dgrogan@, alecflett@ - please take a look?
Alec Flett
Comment 18 2012-12-06 16:05:16 PST
Comment on attachment 178098 [details] Patch Minor naming nit, but otherwise LGTM. View in context: https://bugs.webkit.org/attachment.cgi?id=178098&action=review > Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:192 > + if (schemaVersion > latestSchemaVersion) I feel like 'latest' needs to be 'current' or something - or maybe 'schemaVersion' needs to be 'databaseSchemaVersion' or something. (and same for *Data below) In any case I find this confusing knowing which side of the '>' is the one in the database, and which is the constant.
Joshua Bell
Comment 19 2012-12-06 16:22:04 PST
Comment on attachment 178098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178098&action=review >> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:192 >> + if (schemaVersion > latestSchemaVersion) > > I feel like 'latest' needs to be 'current' or something - or maybe 'schemaVersion' needs to be 'databaseSchemaVersion' or something. (and same for *Data below) In any case I find this confusing knowing which side of the '>' is the one in the database, and which is the constant. I agree it's confusing. How about s/latest/latestKnown/ and 'db' as a prefix on the other, so this turns into: if (dbSchemaVersion > latestKnownSchemaVersion)
David Grogan
Comment 20 2012-12-06 16:23:09 PST
Comment on attachment 178098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178098&action=review >>> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:192 >>> + if (schemaVersion > latestSchemaVersion) >> >> I feel like 'latest' needs to be 'current' or something - or maybe 'schemaVersion' needs to be 'databaseSchemaVersion' or something. (and same for *Data below) In any case I find this confusing knowing which side of the '>' is the one in the database, and which is the constant. > > I agree it's confusing. How about s/latest/latestKnown/ and 'db' as a prefix on the other, so this turns into: > > if (dbSchemaVersion > latestKnownSchemaVersion) lgtm
Joshua Bell
Comment 21 2012-12-06 16:41:27 PST
Joshua Bell
Comment 22 2012-12-06 16:42:57 PST
tony@ - r? There's a little more context over in https://bugs.webkit.org/show_bug.cgi?id=102230
Alec Flett
Comment 23 2012-12-07 09:33:54 PST
Comment on attachment 178107 [details] Patch committing this for jsbell because I want to make sure this gets some bake time in canary.
WebKit Review Bot
Comment 24 2012-12-07 09:53:32 PST
Comment on attachment 178107 [details] Patch Clearing flags on attachment: 178107 Committed r136958: <http://trac.webkit.org/changeset/136958>
WebKit Review Bot
Comment 25 2012-12-07 09:53:37 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.