Bug 102243 - IndexedDB: Check SSV version when opening backing store
Summary: IndexedDB: Check SSV version when opening backing store
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on: 102255
Blocks: 102230
  Show dependency treegraph
 
Reported: 2012-11-14 08:53 PST by Joshua Bell
Modified: 2012-12-07 09:53 PST (History)
11 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 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.
Comment 1 Joshua Bell 2012-11-14 10:51:12 PST
Created attachment 174203 [details]
Patch
Comment 2 Joshua Bell 2012-11-14 10:52:04 PST
First stab at this. Needs tests.
Comment 3 Joshua Bell 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.
Comment 4 Early Warning System Bot 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
Comment 5 kov's GTK+ EWS bot 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
Comment 6 EFL EWS Bot 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
Comment 7 Build Bot 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
Comment 8 Joshua Bell 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.
Comment 9 Joshua Bell 2012-11-14 11:41:38 PST
Created attachment 174213 [details]
Patch
Comment 10 Joshua Bell 2012-11-14 11:42:01 PST
Let's try putting that inside the namespace { }
Comment 11 David Grogan 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.
Comment 12 Joshua Bell 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...
Comment 13 Joshua Bell 2012-11-19 11:04:10 PST
Created attachment 175008 [details]
Patch
Comment 14 Joshua Bell 2012-11-19 11:10:45 PST
Rebased now that 102255 has landed.
Comment 15 Joshua Bell 2012-11-19 11:11:49 PST
This still needs tests, obviously.
Comment 16 Joshua Bell 2012-12-06 15:58:10 PST
Created attachment 178098 [details]
Patch
Comment 17 Joshua Bell 2012-12-06 15:59:00 PST
Chromium-side test in review at: https://codereview.chromium.org/11470013/

dgrogan@, alecflett@ - please take a look?
Comment 18 Alec Flett 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.
Comment 19 Joshua Bell 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)
Comment 20 David Grogan 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
Comment 21 Joshua Bell 2012-12-06 16:41:27 PST
Created attachment 178107 [details]
Patch
Comment 22 Joshua Bell 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
Comment 23 Alec Flett 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.
Comment 24 WebKit Review Bot 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>
Comment 25 WebKit Review Bot 2012-12-07 09:53:37 PST
All reviewed patches have been landed.  Closing bug.