Bug 76746 - [Chromium] IndexedDB: Assertion failure when storing File objects
Summary: [Chromium] IndexedDB: Assertion failure when storing File objects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-20 15:08 PST by Joshua Bell
Modified: 2015-10-12 22:55 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.34 KB, patch)
2012-01-20 15:12 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (1.69 KB, patch)
2012-03-02 15:44 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (17.68 KB, patch)
2012-03-09 13:46 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (17.91 KB, patch)
2012-03-09 13:53 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (17.78 KB, patch)
2012-03-09 15:04 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (25.45 KB, patch)
2012-03-14 17:48 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (23.86 KB, patch)
2012-03-15 13:26 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch for landing (23.79 KB, patch)
2012-03-16 11:28 PDT, 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-01-20 15:08:03 PST
IndexedDB: Crash when storing File objects
Comment 1 Joshua Bell 2012-01-20 15:12:45 PST
Created attachment 123387 [details]
Patch
Comment 2 Joshua Bell 2012-01-20 15:15:41 PST
I don't have an end-to-end fix/test for http://crbug.com/108012, so won't land this yet.
Comment 3 Joshua Bell 2012-02-03 12:14:38 PST
We may want to have SerializedScriptValue support three options:

* Serialize reference if Blob/File/FileList encountered (current behavior, desired for uses other than IDB)
* DataCloneError if Blob/File/FileList encountered
* Serialize data if Blob/File/FileList encountered (ultimately desired behavior for IDB)

This would be selected by an attribute in the IDL which would cause the binding code generator to pass in an enum value.

There is already support for serializing ArrayBuffers; need to look into that further as it could likely be er-used (e.g. via FileReaderLoader::ReadAsArrayBuffer)
Comment 4 Joshua Bell 2012-02-03 13:50:30 PST
See also: https://bugs.webkit.org/show_bug.cgi?id=40835
Comment 5 Joshua Bell 2012-03-02 15:44:11 PST
Created attachment 129969 [details]
Patch
Comment 6 Joshua Bell 2012-03-02 15:46:06 PST
No tests, so I'm still not planning to land this. This would only repro in the Chromium port that uses a WebKit utility process, so can't be tested in DRT. And there are only issues preventing an end-to-end test from succeeding in Chromium. 

Input appreciated.
Comment 7 Joshua Bell 2012-03-09 13:46:02 PST
Created attachment 131092 [details]
Patch
Comment 8 Joshua Bell 2012-03-09 13:51:04 PST
This patch is insufficient for landing (it only implements SSV API changes in V8, not JSC), but I wanted to get early feedback.

This includes:
* SerializedScriptValue exposes blobURLs() accessor
* IndexedDB methods that take SSVs throw DATA_CLONE_ERROR if blobURL list is not empty
* A preemptive fix (from earlier patches) that corrects the assumption that non-Window contexts must be Worker contexts. (In the Chromium port they might be utility process contexts instead.)
Comment 9 Joshua Bell 2012-03-09 13:53:44 PST
Created attachment 131099 [details]
Patch
Comment 10 Joshua Bell 2012-03-09 13:54:14 PST
New patch just updates ChangeLogs with more comments.
Comment 11 David Grogan 2012-03-09 14:44:21 PST
Comment on attachment 131099 [details]
Patch

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

LGTM IDB-wise

> LayoutTests/storage/indexeddb/noblobs.html:103
> +        if (cursor) {

Will this ever not be true?  Either way remove the {}.
Comment 12 Joshua Bell 2012-03-09 15:03:49 PST
(In reply to comment #11)
> > LayoutTests/storage/indexeddb/noblobs.html:103
> > +        if (cursor) {
> 
> Will this ever not be true?  Either way remove the {}.

Good point. No, it will never not be true(!). Simplified.

New patch - I edited the description and forgot to regenerate the expected file anyway.
Comment 13 Joshua Bell 2012-03-09 15:04:08 PST
Created attachment 131120 [details]
Patch
Comment 14 Joshua Bell 2012-03-12 10:48:50 PDT
haraken@ - can you take a look? Any feedback on the SSV API change would be very appreciated.
Comment 15 Kentaro Hara 2012-03-13 04:59:37 PDT
Comment on attachment 131120 [details]
Patch

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

The changes around SerializedScriptValue look good!

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:31
> +#include "ExceptionCode.h"

Nit: not necessary (ExceptionCode is just a typedef and should be already defined in IDBCursor.h)

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:32
> +#include "ExceptionCode.h"

Nit: not necessary

> Source/WebCore/bindings/v8/V8DOMWrapper.cpp:239
> +            else if (isWrapperOfType(globalPrototype, &V8WorkerContext::info))

What is this change for?
Comment 16 Joshua Bell 2012-03-13 08:48:22 PDT
(In reply to comment #15)
> 
> > Source/WebCore/bindings/v8/V8DOMWrapper.cpp:239
> > +            else if (isWrapperOfType(globalPrototype, &V8WorkerContext::info))
> 
> What is this change for?

Ah, context was lost. That's source of the actual crash the bug title references. I added a note to the ChangeList in the latest patch:

WebCore::V8DOMWrapper::instantiateV8Object):  Ensure context is a worker context before treating it as such

The instantiateV8Object code looks like it started off handling two cases: (1) "window" context or (2) no context (i.e. chromium utility process). At some point logic was added to support worker contexts, but that code just assumed "if !window then worker", and thus failed for the chromium utility process.

If you attempt to deserialize a Blob without this patch at all, it hits the above case and crashes if the utility process is involved (e.g. IDB key path calculation). With just the instantiateV8Object change it doesn't crash but (in the the chromium port, at least) the blob's data has likely been released so chromium DCHECKs later in the deserialization process.

Having IDB reject SSVs with blobs would prevent any of the above from occurring, but that's temporary until we do correctly serialize the blob data. I could remove the instantiateV8Object change, but the code right now is definitely incorrect.
Comment 17 Kentaro Hara 2012-03-13 11:01:12 PDT
(In reply to comment #16)
> Ah, context was lost. That's source of the actual crash the bug title references. I added a note to the ChangeList in the latest patch:

Thank you very much for the clarification. r+ for the SSV related code. I am not familiar with IDB and thus would like to delegate the review to another guy.
Comment 18 David Grogan 2012-03-13 12:36:45 PDT
LGTM for IDB-stuff
Comment 19 Joshua Bell 2012-03-13 12:38:23 PDT
Comment on attachment 131120 [details]
Patch

Thanks for the preliminary looks. Marking r- for now until I update with the JSC version.
Comment 20 Joshua Bell 2012-03-14 17:48:20 PDT
Created attachment 131964 [details]
Patch
Comment 21 Kentaro Hara 2012-03-14 17:55:36 PDT
Comment on attachment 131964 [details]
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:-1660
> -SerializedScriptValue::SerializedScriptValue(Vector<uint8_t>& buffer)

Maybe you can keep SerializedScriptValue(Vector<uint8_t>& buffer), which initializes m_blobURLs to an empty string vector.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1733
> -    return adoptRef(new SerializedScriptValue(buffer));
> +    Vector<String> blobURLs;
> +    return adoptRef(new SerializedScriptValue(buffer, blobURLs));

If you keep SerializedScriptValue(Vector<uint8_t>& buffer), this change won't be necessary.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1742
> +    Vector<String> blobURLs;
>      if (!CloneSerializer::serialize(string, buffer))
>          return 0;
> -    return adoptRef(new SerializedScriptValue(buffer));
> +    return adoptRef(new SerializedScriptValue(buffer, blobURLs));

Ditto.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1855
> +    Vector<String> blobURLs;
>      CloneSerializer::serializeUndefined(buffer);
> -    return adoptRef(new SerializedScriptValue(buffer));
> +    return adoptRef(new SerializedScriptValue(buffer, blobURLs));

Ditto.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1863
> +    Vector<String> blobURLs;
>      CloneSerializer::serializeBoolean(value, buffer);
> -    return adoptRef(new SerializedScriptValue(buffer));
> +    return adoptRef(new SerializedScriptValue(buffer, blobURLs));

Ditto.

> Source/WebCore/bindings/js/SerializedScriptValue.h:77
> -        return adoptRef(new SerializedScriptValue(buffer));
> +        Vector<String> blobURLs;
> +        return adoptRef(new SerializedScriptValue(buffer, blobURLs));

Ditto.
Comment 22 Build Bot 2012-03-14 19:02:13 PDT
Comment on attachment 131964 [details]
Patch

Attachment 131964 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11953594
Comment 23 Joshua Bell 2012-03-15 13:26:44 PDT
Created attachment 132110 [details]
Patch
Comment 24 Joshua Bell 2012-03-15 13:28:55 PDT
(In reply to comment #21)
> (From update of attachment 131964 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131964&action=review
> 
> > Source/WebCore/bindings/js/SerializedScriptValue.cpp:-1660
> > -SerializedScriptValue::SerializedScriptValue(Vector<uint8_t>& buffer)
> 
> Maybe you can keep SerializedScriptValue(Vector<uint8_t>& buffer), which initializes m_blobURLs to an empty string vector.

Agreed - Done.
Comment 25 Joshua Bell 2012-03-15 15:37:40 PDT
tony@, can you review?
Comment 26 Tony Chang 2012-03-15 16:18:05 PDT
Comment on attachment 132110 [details]
Patch

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

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1000
> +        uint32_t length = fileList->length();
> +        for (unsigned i = 0; i < length; ++i)

Nit: It looks like the return type is 'unsigned' for FileList::length().

> LayoutTests/storage/indexeddb/noblobs.html:9
> +<div id="console"></div>

Nit: I think js-test-pre.js will auto create the console div for you.
Comment 27 Joshua Bell 2012-03-16 10:43:54 PDT
(In reply to comment #26)
> (From update of attachment 132110 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132110&action=review
> 
> > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1000
> > +        uint32_t length = fileList->length();
> > +        for (unsigned i = 0; i < length; ++i)
> 
> Nit: It looks like the return type is 'unsigned' for FileList::length().

Thanks, will fix.

> > LayoutTests/storage/indexeddb/noblobs.html:9
> > +<div id="console"></div>
> 
> Nit: I think js-test-pre.js will auto create the console div for you.

All of the storage/indexeddb/*.html tests have this. I'll file another bug to clean this up.
Comment 28 Joshua Bell 2012-03-16 11:28:02 PDT
Created attachment 132328 [details]
Patch for landing
Comment 29 WebKit Review Bot 2012-03-16 12:26:16 PDT
Comment on attachment 132328 [details]
Patch for landing

Clearing flags on attachment: 132328

Committed r111044: <http://trac.webkit.org/changeset/111044>
Comment 30 WebKit Review Bot 2012-03-16 12:26:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Peng Xinchao 2015-10-12 22:54:51 PDT
(In reply to comment #0)
> IndexedDB: Crash when storing File objects

 I want to enalbe BLOB/File of the IndexDB, Could you support the testcase of storing File objects ?
i have tested some case , i need more testcase to check my patch .
Thank you
Comment 32 Peng Xinchao 2015-10-12 22:55:50 PDT
(In reply to comment #31)
> (In reply to comment #0)
> > IndexedDB: Crash when storing File objects
> Dear Mr.Bell
>  I want to enalbe BLOB/File of the IndexDB, Could you support the testcase
> of storing File objects ?
> i have tested some case , i need more testcase to check my patch .
> Thank you