Bug 76746 – [Chromium] IndexedDB: Assertion failure when storing File objects

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: Crash when storing File objects

[Joshua Bell]

Created attachment 123387 [details]
Patch

[Joshua Bell]

I don't have an end-to-end fix/test for http://crbug.com/108012, so won't land this yet.

[Joshua Bell]

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)

[Joshua Bell]

See also: https://bugs.webkit.org/show_bug.cgi?id=40835

[Joshua Bell]

Created attachment 129969 [details]
Patch

[Joshua Bell]

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.

[Joshua Bell]

Created attachment 131092 [details]
Patch

[Joshua Bell]

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.)

[Joshua Bell]

Created attachment 131099 [details]
Patch

[Joshua Bell]

New patch just updates ChangeLogs with more comments.

[David Grogan]

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 {}.

[Joshua Bell]

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

[Joshua Bell]

Created attachment 131120 [details]
Patch

[Joshua Bell]

haraken@ - can you take a look? Any feedback on the SSV API change would be very appreciated.

[Kentaro Hara]

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?

[Joshua Bell]

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

[Kentaro Hara]

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

[David Grogan]

LGTM for IDB-stuff

[Joshua Bell]

Comment on attachment 131120 [details]
Patch

Thanks for the preliminary looks. Marking r- for now until I update with the JSC version.

[Joshua Bell]

Created attachment 131964 [details]
Patch

[Kentaro Hara]

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.

[Build Bot]

Comment on attachment 131964 [details]
Patch

Attachment 131964 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11953594

[Joshua Bell]

Created attachment 132110 [details]
Patch

[Joshua Bell]

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

[Joshua Bell]

tony@, can you review?

[Tony Chang]

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.

[Joshua Bell]

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

[Joshua Bell]

Created attachment 132328 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 132328 [details]
Patch for landing

Clearing flags on attachment: 132328

Committed r111044: <http://trac.webkit.org/changeset/111044>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Peng Xinchao]

(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

[Peng Xinchao]

(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