WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(11.00 KB, patch)
2012-11-14 11:41 PST
,
Joshua Bell
no flags
Details
Formatted Diff
Diff
Patch
(10.74 KB, patch)
2012-11-19 11:04 PST
,
Joshua Bell
no flags
Details
Formatted Diff
Diff
Patch
(10.84 KB, patch)
2012-12-06 15:58 PST
,
Joshua Bell
no flags
Details
Formatted Diff
Diff
Patch
(11.12 KB, patch)
2012-12-06 16:41 PST
,
Joshua Bell
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Joshua Bell
Comment 1
2012-11-14 10:51:12 PST
Created
attachment 174203
[details]
Patch
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
Comment on
attachment 174203
[details]
Patch
Attachment 174203
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/14828544
kov's GTK+ EWS bot
Comment 5
2012-11-14 11:21:52 PST
Comment on
attachment 174203
[details]
Patch
Attachment 174203
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/14845081
EFL EWS Bot
Comment 6
2012-11-14 11:22:41 PST
Comment on
attachment 174203
[details]
Patch
Attachment 174203
[details]
did not pass efl-ews (efl): Output:
http://queues.webkit.org/results/14838383
Build Bot
Comment 7
2012-11-14 11:35:48 PST
Comment on
attachment 174203
[details]
Patch
Attachment 174203
[details]
did not pass win-ews (win): Output:
http://queues.webkit.org/results/14834428
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
Created
attachment 174213
[details]
Patch
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
Created
attachment 175008
[details]
Patch
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
Created
attachment 178098
[details]
Patch
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
Created
attachment 178107
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug