Testing blobs in tests storage/indexeddb/structured-clone.html crashes.
Created attachment 404635 [details] Patch
Created attachment 404636 [details] Patch
Comment on attachment 404636 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404636&action=review > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2949 > + if (!m_isDOMGlobalObject || m_globalObject->inherits<JSIDBSerializationGlobalObject>(m_globalObject->vm())) There are some more m_isDOMGlobalObject that might benefit from the same change, for instance in readFile. Would it be possible to just initialise m_isDOMGlobalObject to false if m_globalObject->inherits<JSIDBSerializationGlobalObject>(...) is true? And maybe rename m_isDOMGlobalObject to a more precise name. > Source/WebCore/bindings/js/SerializedScriptValue.cpp:3174 > if (m_isDOMGlobalObject) We could check for JSIDBSerializationGlobalObject as well here.
Comment on attachment 404636 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404636&action=review >> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2949 >> + if (!m_isDOMGlobalObject || m_globalObject->inherits<JSIDBSerializationGlobalObject>(m_globalObject->vm())) > > There are some more m_isDOMGlobalObject that might benefit from the same change, for instance in readFile. > > Would it be possible to just initialise m_isDOMGlobalObject to false if m_globalObject->inherits<JSIDBSerializationGlobalObject>(...) is true? > And maybe rename m_isDOMGlobalObject to a more precise name. I am not sure whether there is other use case of m_isDOMGlobalObject other than IDB so I did not change this check. Will try. >> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3174 >> if (m_isDOMGlobalObject) > > We could check for JSIDBSerializationGlobalObject as well here. Sure. It may be even better to move the check to before the iteration.
Created attachment 404721 [details] Patch
Created attachment 404724 [details] Patch
<rdar://problem/65834875>
Comment on attachment 404724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404724&action=review > Source/WebCore/bindings/js/SerializedScriptValue.cpp:3161 > + return jsNull(); We will not call readFile so will not be able to advance m_ptr. We should probably do this check after readFile. Ditto for FileListTag. > Source/WebCore/bindings/js/SerializedScriptValue.cpp:3198 > + ASSERT_NOT_REACHED(); This change is not explained and is somehow counter intuitive. Why not: "if (m_isJSIDBSerializationGlobalObject) ..." without the ASSERT_NOT_REACHED?
Comment on attachment 404724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404724&action=review >> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3161 >> + return jsNull(); > > We will not call readFile so will not be able to advance m_ptr. > We should probably do this check after readFile. > Ditto for FileListTag. Sure. >> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3198 >> + ASSERT_NOT_REACHED(); > > This change is not explained and is somehow counter intuitive. > Why not: "if (m_isJSIDBSerializationGlobalObject) ..." without the ASSERT_NOT_REACHED? ImageData can be deserialized with JSIDBSerializationGlobalObject according to storage/indexeddb/structured-clone.html I added this assertion to check if any test uses this code path (called with non JSDOMGlobalObject) other than previous IDB; if so then we need to keep this check. It looks like we can remove this.
Created attachment 404826 [details] Patch for landing
Committed r264661: <https://trac.webkit.org/changeset/264661> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404826 [details].