Bug 200572 – Blob should store its session ID

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[youenn fablet]

Blob should store its session ID

[youenn fablet]

Created attachment 375910 [details]
Patch

[youenn fablet]

Created attachment 375913 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/54125466>

[Darin Adler]

Comment on attachment 375913 [details]
Patch

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

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:161
> +        return Blob::create(scriptExecutionContext()->sessionID());
> +    return Blob::create(scriptExecutionContext()->sessionID(), *data, m_private->mimeType());

What guarantees that the script execution context is not null?

> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:205
> +        scheduleDispatchEvent(MessageEvent::create(Blob::create(scriptExecutionContext()->sessionID(), SharedBuffer::create(data, dataLength), emptyString()), { }));

What guarantees that the script execution context is not null?

> Source/WebCore/Modules/websockets/WebSocket.cpp:587
> +        dispatchEvent(MessageEvent::create(Blob::create(scriptExecutionContext()->sessionID(), WTFMove(binaryData), emptyString()), SecurityOrigin::create(m_url)->toString()));

What guarantees that the script execution context is not null?

> Source/WebCore/bindings/js/JSDOMPromiseDeferred.h:141
> +    PAL::SessionID sessionID() const { return globalObject()->scriptExecutionContext()->sessionID(); }

A little annoying that we have to add includes so this can be inlined.

What guarantees that neither the global object nor the script execution context is null?

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2098
> +            file = File::deserialize(jsCast<JSDOMGlobalObject*>(m_globalObject)->scriptExecutionContext()->sessionID(), filePath, URL(URL(), url->string()), type->string(), name->string(), optionalLastModified);

What guarantees that neither the global object nor the script execution context is null?

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2864
> +            return getJSValue(Blob::deserialize(jsCast<JSDOMGlobalObject*>(m_globalObject)->scriptExecutionContext()->sessionID(), URL(URL(), url->string()), type->string(), size, blobFilePathForBlobURL(url->string())).get());

What guarantees that neither the global object nor the script execution context is null?

> Source/WebCore/fileapi/Blob.cpp:92
>      , m_size(-1)

Seems peculiar to use -1 instead of Optional or something clearer. Might want to come back and revisit this.

> Source/WebCore/html/FileInputType.cpp:417
> +    m_fileListCreator = FileListCreator::create(element()->document().sessionID(), paths, shouldResolveDirectories, [this, shouldRequestIcon](Ref<FileList>&& fileList) {

Does this function ever get called when element() can be nullptr? Some of the other functions in InputType classes seem to guard against this.

> Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp:152
> +        ScriptExecutionContext& context = globalScope;

I don’t understand why this local variable is needed. Can’t we call globalScope.sessionID()?

> Source/WebCore/xml/XMLHttpRequest.cpp:215
> +    return Blob::create(scriptExecutionContext()->sessionID(), WTFMove(data), normalizedContentType);

What guarantees that script execution context is non-null?

[youenn fablet]

Thanks for the review.

(In reply to Darin Adler from comment #4)
> Comment on attachment 375913 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375913&action=review
> 
> > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:161
> > +        return Blob::create(scriptExecutionContext()->sessionID());
> > +    return Blob::create(scriptExecutionContext()->sessionID(), *data, m_private->mimeType());
> 
> What guarantees that the script execution context is not null?

We create a blob to dispatch an event so the MediaRecorder is not stopped and its context is not null.

> > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:205
> > +        scheduleDispatchEvent(MessageEvent::create(Blob::create(scriptExecutionContext()->sessionID(), SharedBuffer::create(data, dataLength), emptyString()), { }));
> 
> What guarantees that the script execution context is not null?

Ditto.

> > Source/WebCore/Modules/websockets/WebSocket.cpp:587
> > +        dispatchEvent(MessageEvent::create(Blob::create(scriptExecutionContext()->sessionID(), WTFMove(binaryData), emptyString()), SecurityOrigin::create(m_url)->toString()));
> 
> What guarantees that the script execution context is not null?

Ditto.

> > Source/WebCore/bindings/js/JSDOMPromiseDeferred.h:141
> > +    PAL::SessionID sessionID() const { return globalObject()->scriptExecutionContext()->sessionID(); }
> 
> A little annoying that we have to add includes so this can be inlined.

Agreed.

> What guarantees that neither the global object nor the script execution
> context is null?

The caller has to make sure the promise is basically canInvokeCallback.
I'll revert this change since it might be a tempting but risky API and pass the sessionID through FetchBody.

> > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2098
> > +            file = File::deserialize(jsCast<JSDOMGlobalObject*>(m_globalObject)->scriptExecutionContext()->sessionID(), filePath, URL(URL(), url->string()), type->string(), name->string(), optionalLastModified);
> 
> What guarantees that neither the global object nor the script execution
> context is null?

There is a check before for m_globalObject not being null.
As of scriptExecutionContext, it only returns null after release_assert.

> > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2864
> > +            return getJSValue(Blob::deserialize(jsCast<JSDOMGlobalObject*>(m_globalObject)->scriptExecutionContext()->sessionID(), URL(URL(), url->string()), type->string(), size, blobFilePathForBlobURL(url->string())).get());
> 
> What guarantees that neither the global object nor the script execution
> context is null?

Ditto.

> > Source/WebCore/fileapi/Blob.cpp:92
> >      , m_size(-1)
> 
> Seems peculiar to use -1 instead of Optional or something clearer. Might
> want to come back and revisit this.

True, this seems only used to lazily initialize m_size which can cost a sync IPC.

> > Source/WebCore/html/FileInputType.cpp:417
> > +    m_fileListCreator = FileListCreator::create(element()->document().sessionID(), paths, shouldResolveDirectories, [this, shouldRequestIcon](Ref<FileList>&& fileList) {
> 
> Does this function ever get called when element() can be nullptr? Some of
> the other functions in InputType classes seem to guard against this.

From the call sites I checked, this seems fine, 2 of them are calling element() and the last is from the input element itself.

> > Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp:152
> > +        ScriptExecutionContext& context = globalScope;
> 
> I don’t understand why this local variable is needed. Can’t we call
> globalScope.sessionID()?

sessionID is a private method of WorkerGlobalScope.
We could make it public though.
I'll change that.

> > Source/WebCore/xml/XMLHttpRequest.cpp:215
> > +    return Blob::create(scriptExecutionContext()->sessionID(), WTFMove(data), normalizedContentType);
> 
> What guarantees that script execution context is non-null?

This is called from JS binding custom code which checks that there are no errors before calling responseBlob.
When the script execution context is stopped, the XHR object will be errored and createResponseBlob will not be created.

Thinking about it more, this behaviour does not seem to align well with the spec although Safari, Chrome and Firefox do seem to return null after context navigation. 
'response' attribute value caching is different in the various browsers.

[Darin Adler]

Comment on attachment 375913 [details]
Patch

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

>>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2098
>>> +            file = File::deserialize(jsCast<JSDOMGlobalObject*>(m_globalObject)->scriptExecutionContext()->sessionID(), filePath, URL(URL(), url->string()), type->string(), name->string(), optionalLastModified);
>> 
>> What guarantees that neither the global object nor the script execution context is null?
> 
> There is a check before for m_globalObject not being null.
> As of scriptExecutionContext, it only returns null after release_assert.

Cleanup for the future: If scriptExecutionContext only returns nullptr after a RELEASE_ASSERT, it should be changed to return a reference instead of a pointer.

[youenn fablet]

Created attachment 376016 [details]
Patch for landing

[youenn fablet]

> > What guarantees that neither the global object nor the script execution
> > context is null?
> 
> The caller has to make sure the promise is basically canInvokeCallback.
> I'll revert this change since it might be a tempting but risky API and pass
> the sessionID through FetchBody.

I'll fix this in bug 200616.

[WebKit Commit Bot]

Comment on attachment 376016 [details]
Patch for landing

Clearing flags on attachment: 376016

Committed r248503: <https://trac.webkit.org/changeset/248503>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.