Bug 52890 – IndexedDB corrupts data on disk

• Component: New Bugs
• Status: RESOLVED FIXED

[Andrei Popescu]

IndexedDB corrupts data on disk

[Andrei Popescu]

SerializedScriptValue values should be stored in a BLOB column rather than TEXT.

[Andrei Popescu]

Created attachment 79732 [details]
Patch

[WebKit Review Bot]

Attachment 79732 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/stor..." exit_code: 1

Source/WebCore/platform/sql/SQLiteStatement.cpp:388:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hans Wennborg]

Comment on attachment 79732 [details]
Patch

Looks great. Some nits:


> Source/WebCore/platform/sql/SQLiteStatement.cpp:190
> +    return bindBlob(index, reinterpret_cast<const void*>(characters), text.length() * sizeof(UChar));

I believe static_cast is preferred. Sorry for not saying that earlier.

> Source/WebCore/platform/sql/SQLiteStatement.cpp:391
> +    return String(reinterpret_cast<const UChar*>(blob), size / sizeof(UChar));

Ditto.

> Source/WebCore/platform/sql/SQLiteStatement.cpp:418
>  }

Ditto.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:134
> +    return runCommands(sqliteDatabase, commands, 12);

Using '12' here seems a bit dangerous.. sizeof commands / sizeof commands[0] ?

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:144
> +    return runCommands(sqliteDatabase, commands, 2);

Ditto for 2.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:187
> +        if (!runCommands(sqliteDatabase, commands, 15))

Ditto.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:205
> +        if (!runCommands(sqliteDatabase, commands, 7))

Ditto.

[Hans Wennborg]

Comment on attachment 79732 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79732&action=review

>> Source/WebCore/platform/sql/SQLiteStatement.cpp:190
>> +    return bindBlob(index, reinterpret_cast<const void*>(characters), text.length() * sizeof(UChar));
> 
> I believe static_cast is preferred. Sorry for not saying that earlier.

In fact, I don't even think you need a cast here...

[Andrei Popescu]

Created attachment 79751 [details]
Patch

[Andrei Popescu]

Thanks Hans!

(In reply to comment #4)
> (From update of attachment 79732 [details])
> Looks great. Some nits:
> 
> 
> > Source/WebCore/platform/sql/SQLiteStatement.cpp:190
> > +    return bindBlob(index, reinterpret_cast<const void*>(characters), text.length() * sizeof(UChar));
> 
> I believe static_cast is preferred. Sorry for not saying that earlier.
> 

Removed cast altogether.

> > Source/WebCore/platform/sql/SQLiteStatement.cpp:391
> > +    return String(reinterpret_cast<const UChar*>(blob), size / sizeof(UChar));
> 
> Ditto.
> 

Done.

> > Source/WebCore/platform/sql/SQLiteStatement.cpp:418
> >  }
> 
> Ditto.
> 

Done.

> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:134
> > +    return runCommands(sqliteDatabase, commands, 12);
> 
> Using '12' here seems a bit dangerous.. sizeof commands / sizeof commands[0] ?
> 

Done.

> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:144
> > +    return runCommands(sqliteDatabase, commands, 2);
> 
> Ditto for 2.
> 

Done.

> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:187
> > +        if (!runCommands(sqliteDatabase, commands, 15))
> 
> Ditto.
> 

Done.

> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:205
> > +        if (!runCommands(sqliteDatabase, commands, 7))
> 
> Ditto.

Done.

[WebKit Review Bot]

Attachment 79751 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/stor..." exit_code: 1

Source/WebCore/platform/sql/SQLiteStatement.cpp:388:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jeremy Orlow]

Comment on attachment 79751 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79751&action=review

r=me

>> Source/WebCore/platform/sql/SQLiteStatement.cpp:388
>> +    if (size < 0 || size % sizeof(UChar) != 0)
> 
> Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]

The latter seems more like a sanity check...should we assert it's not true?  The following / is safe in that we won't read invalid memory.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:189
>  

get rid of second newline

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:198
> +            "INSERT INTO ObjectStoreData2 SELECT * FROM ObjectStoreData",

I worry about this....and think we should probably just blow away the data.  No one should have anything they really care about yet.

[Andrei Popescu]

Committed r76448: <http://trac.webkit.org/changeset/76448>