Bug 102243 – IndexedDB: Check SSV version when opening backing store

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joshua Bell]

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.

[Joshua Bell]

Created attachment 174203 [details]
Patch

[Joshua Bell]

First stab at this. Needs tests.

[Joshua Bell]

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 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 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 on attachment 174203 [details]
Patch

Attachment 174203 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14838383

[Build Bot]

Comment on attachment 174203 [details]
Patch

Attachment 174203 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14834428

[Joshua Bell]

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]

Created attachment 174213 [details]
Patch

[Joshua Bell]

Let's try putting that inside the namespace { }

[David Grogan]

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]

(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]

Created attachment 175008 [details]
Patch

[Joshua Bell]

Rebased now that 102255 has landed.

[Joshua Bell]

This still needs tests, obviously.

[Joshua Bell]

Created attachment 178098 [details]
Patch

[Joshua Bell]

Chromium-side test in review at: https://codereview.chromium.org/11470013/

dgrogan@, alecflett@ - please take a look?

[Alec Flett]

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 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 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]

Created attachment 178107 [details]
Patch

[Joshua Bell]

tony@ - r?

There's a little more context over in https://bugs.webkit.org/show_bug.cgi?id=102230

[Alec Flett]

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 on attachment 178107 [details]
Patch

Clearing flags on attachment: 178107

Committed r136958: <http://trac.webkit.org/changeset/136958>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.