Bug 52890

Summary: IndexedDB corrupts data on disk
Product: WebKit Reporter: Andrei Popescu <andreip>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: hans, jorlow, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch jorlow: review+

Description Andrei Popescu 2011-01-21 07:03:16 PST
IndexedDB corrupts data on disk
Comment 1 Andrei Popescu 2011-01-21 07:11:13 PST
SerializedScriptValue values should be stored in a BLOB column rather than TEXT.
Comment 2 Andrei Popescu 2011-01-21 07:20:38 PST
Created attachment 79732 [details]
Patch
Comment 3 WebKit Review Bot 2011-01-21 07:23:47 PST
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.
Comment 4 Hans Wennborg 2011-01-21 09:10:25 PST
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.
Comment 5 Hans Wennborg 2011-01-21 09:14:35 PST
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...
Comment 6 Andrei Popescu 2011-01-21 09:39:57 PST
Created attachment 79751 [details]
Patch
Comment 7 Andrei Popescu 2011-01-21 09:41:39 PST
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.
Comment 8 WebKit Review Bot 2011-01-21 09:43:13 PST
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.
Comment 9 Jeremy Orlow 2011-01-21 10:14:56 PST
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.
Comment 10 Andrei Popescu 2011-01-22 07:00:54 PST
Committed r76448: <http://trac.webkit.org/changeset/76448>