IndexedDB: Crash when storing File objects
Created attachment 123387 [details] Patch
I don't have an end-to-end fix/test for http://crbug.com/108012, so won't land this yet.
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)
See also: https://bugs.webkit.org/show_bug.cgi?id=40835
Created attachment 129969 [details] Patch
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.
Created attachment 131092 [details] Patch
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.)
Created attachment 131099 [details] Patch
New patch just updates ChangeLogs with more comments.
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 {}.
(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.
Created attachment 131120 [details] Patch
haraken@ - can you take a look? Any feedback on the SSV API change would be very appreciated.
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?
(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.
(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.
LGTM for IDB-stuff
Comment on attachment 131120 [details] Patch Thanks for the preliminary looks. Marking r- for now until I update with the JSC version.
Created attachment 131964 [details] Patch
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 on attachment 131964 [details] Patch Attachment 131964 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11953594
Created attachment 132110 [details] Patch
(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.
tony@, can you review?
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.
(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.
Created attachment 132328 [details] Patch for landing
Comment on attachment 132328 [details] Patch for landing Clearing flags on attachment: 132328 Committed r111044: <http://trac.webkit.org/changeset/111044>
All reviewed patches have been landed. Closing bug.
(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
(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