REGRESSION(r264486): ASSERTION FAILED: ASSERT_NOT_REACHED() in NetworkProcessPlatformStrategies::createBlobRegistry

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

Testing blobs in tests storage/indexeddb/structured-clone.html crashes.

[Sihui Liu]

Created attachment 404635 [details]
Patch

[Sihui Liu]

Created attachment 404636 [details]
Patch

[youenn fablet]

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.

[Sihui Liu]

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.

[Sihui Liu]

Created attachment 404721 [details]
Patch

[Sihui Liu]

Created attachment 404724 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/65834875>

[youenn fablet]

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?

[Sihui Liu]

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.

[Sihui Liu]

Created attachment 404826 [details]
Patch for landing

[EWS]

Committed r264661: <https://trac.webkit.org/changeset/264661>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404826 [details].