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.
Created attachment 174203 [details] Patch
First stab at this. Needs tests.
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.
Comment on attachment 174203 [details] Patch Attachment 174203 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14828544
Comment on attachment 174203 [details] Patch Attachment 174203 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14845081
Comment on attachment 174203 [details] Patch Attachment 174203 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14838383
Comment on attachment 174203 [details] Patch Attachment 174203 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14834428
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.
Created attachment 174213 [details] Patch
Let's try putting that inside the namespace { }
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.
(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...
Created attachment 175008 [details] Patch
Rebased now that 102255 has landed.
This still needs tests, obviously.
Created attachment 178098 [details] Patch
Chromium-side test in review at: https://codereview.chromium.org/11470013/ dgrogan@, alecflett@ - please take a look?
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.
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)
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
Created attachment 178107 [details] Patch
tony@ - r? There's a little more context over in https://bugs.webkit.org/show_bug.cgi?id=102230
Comment on attachment 178107 [details] Patch committing this for jsbell because I want to make sure this gets some bake time in canary.
Comment on attachment 178107 [details] Patch Clearing flags on attachment: 178107 Committed r136958: <http://trac.webkit.org/changeset/136958>
All reviewed patches have been landed. Closing bug.