Bug 200572

Summary: Blob should store its session ID
Product: WebKit Reporter: youenn fablet <youennf>
Component: Page LoadingAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, cdumez, commit-queue, darin, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 200567    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description youenn fablet 2019-08-09 06:04:17 PDT
Blob should store its session ID
Comment 1 youenn fablet 2019-08-09 06:13:16 PDT
Created attachment 375910 [details]
Patch
Comment 2 youenn fablet 2019-08-09 06:36:36 PDT
Created attachment 375913 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2019-08-09 07:35:58 PDT
<rdar://problem/54125466>
Comment 4 Darin Adler 2019-08-09 08:29:57 PDT
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?
Comment 5 youenn fablet 2019-08-09 14:47:30 PDT
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.
Comment 6 Darin Adler 2019-08-09 15:50:10 PDT
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.
Comment 7 youenn fablet 2019-08-10 14:47:55 PDT
Created attachment 376016 [details]
Patch for landing
Comment 8 youenn fablet 2019-08-10 14:49:28 PDT
> > 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.
Comment 9 WebKit Commit Bot 2019-08-10 15:50:26 PDT
Comment on attachment 376016 [details]
Patch for landing

Clearing flags on attachment: 376016

Committed r248503: <https://trac.webkit.org/changeset/248503>
Comment 10 WebKit Commit Bot 2019-08-10 15:50:28 PDT
All reviewed patches have been landed.  Closing bug.