Bug 199189 – WebSockets: add support for sending blob messages when using web sockets platform APIs

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

It's currently unimplemented.

[Carlos Garcia Campos]

Created attachment 372834 [details]
Patch

[youenn fablet]

Comment on attachment 372834 [details]
Patch

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

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:180
> +    m_pendingBlobMessages.append(std::make_unique<BlobLoader>(m_document.get(), blob, [this, protectedThis = makeRef(*this)] {

Whenever we have a pending blob message, we should enqueue all other messages until sending the blob.
Otherwise send(blob);send(arrayBuffer); will end up sending the arrayBuffer first and the blob second.
Since blobs are stored in NetworkProcess, it might be more efficient to do this enqueuing/reading in Network Process as well.

[Carlos Garcia Campos]

(In reply to youenn fablet from comment #2)
> Comment on attachment 372834 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372834&action=review
> 
> > Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:180
> > +    m_pendingBlobMessages.append(std::make_unique<BlobLoader>(m_document.get(), blob, [this, protectedThis = makeRef(*this)] {
> 
> Whenever we have a pending blob message, we should enqueue all other
> messages until sending the blob.
> Otherwise send(blob);send(arrayBuffer); will end up sending the arrayBuffer
> first and the blob second.

Oops, indeed.

> Since blobs are stored in NetworkProcess, it might be more efficient to do
> this enqueuing/reading in Network Process as well.

hmm, that would be more complicated, I'm afraid, we need to register the blob, send a network request to the network process socket channel, start the network load, and reaply back to unregister the blob. The part of starting the network connection in the network process is what I think it's more complicated. Should we use NetworkREsourceLoder? or NetworkLoad directly? For now I'm going to leave it in the web process using FileReadLoader, I'm not sure it's worth the effort of optimizing that.

[Carlos Garcia Campos]

Created attachment 372915 [details]
Patch

[youenn fablet]

Comment on attachment 372915 [details]
Patch

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

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:159
> +    const RefPtr<JSC::ArrayBuffer>& result() const { return m_buffer; }

Preexisting issue in WebSocket code so I guess this is fine.
But it does not seem super great to get a JSC::ArrayBuffer.
Maybe we should do some refactoring or use FetchLoader like in FetchBodyOwner.

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:195
> +    PendingMessage(const String& message, CompletionHandler<void()>&& completionHandler)

String&&

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:229
> +    CompletionHandler<void()> m_completionHandler;

Since these are completion handlers, we should call them when clearing the pending messages, otherwise there will be debug asserts.
Maybe we should change the implementation so that keeping these completion handlers is not needed.
That would mean slight adjustements to how we compute m_bufferedAmount.

Also this patch is doing multi blob loading in parallel.
While this is better for performance, we might be able to simplify things by keeping the current behavior, which means having a Deque<Variant<String, ArrayBuffer, Blob>> and a bool m_loadingBlob.

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:236
> +        return send(JSC::ArrayBuffer::create(blob.size(), 1), 0, 0);

If size is greater than zero, we need to +/- m_bufferedAmount

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:238
> +    m_pendingMessages.append(std::make_unique<PendingMessage>(m_document.get(), blob, [this, protectedThis = makeRef(*this)] {

We might want to check whether the socket is closed there.
A protectedThis is not needed here since m_pendingMessages is owned.

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:258
> +                    send(*result, 0, result->byteLength());

We should probably do a sendWithAsyncReply here instead of a send.
That might be needed for m_bufferedAmount and for readability.
Initially I was puzzled of m_sendingPendingBlobMessage check in send(ArrayBuffer) but not send(String)

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:260
> +                    fail(makeString("Failed to load Blob: error code = ", errorCode.value()));

I wonder whether something more is needed, like sending an onerror message.
Might be somehow testable with an Internals API to create a 'fake' blob.

[Carlos Garcia Campos]

Comment on attachment 372915 [details]
Patch

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

>> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:229
>> +    CompletionHandler<void()> m_completionHandler;
> 
> Since these are completion handlers, we should call them when clearing the pending messages, otherwise there will be debug asserts.
> Maybe we should change the implementation so that keeping these completion handlers is not needed.
> That would mean slight adjustements to how we compute m_bufferedAmount.
> 
> Also this patch is doing multi blob loading in parallel.
> While this is better for performance, we might be able to simplify things by keeping the current behavior, which means having a Deque<Variant<String, ArrayBuffer, Blob>> and a bool m_loadingBlob.

There's a FIXME in current code, so I thought this was a good moment to fix that in the new code path.

// FIXME: Load two or more Blobs simultaneously for better performance.

>> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:236
>> +        return send(JSC::ArrayBuffer::create(blob.size(), 1), 0, 0);
> 
> If size is greater than zero, we need to +/- m_bufferedAmount

Size is always 0, I'm using blob.size() instead of 0 directly to disambiguate from the different ::create() methods. This is calling send() anyway that already takes care of m_bufferedAmount, even though it will do nothing in this case.

>> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:238
>> +    m_pendingMessages.append(std::make_unique<PendingMessage>(m_document.get(), blob, [this, protectedThis = makeRef(*this)] {
> 
> We might want to check whether the socket is closed there.
> A protectedThis is not needed here since m_pendingMessages is owned.

You mean checking it in the completion handler after the blob is loaded?

>> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:258
>> +                    send(*result, 0, result->byteLength());
> 
> We should probably do a sendWithAsyncReply here instead of a send.
> That might be needed for m_bufferedAmount and for readability.
> Initially I was puzzled of m_sendingPendingBlobMessage check in send(ArrayBuffer) but not send(String)

send() is needed for m_bufferedAmount, using sendWithAsyncReply here would require to define the completion handler here and handle buffered amount, that would duplicate code and make readability here worse for sure. I agree the m_sendingPendingBlobMessage is confusing though.

>> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:260
>> +                    fail(makeString("Failed to load Blob: error code = ", errorCode.value()));
> 
> I wonder whether something more is needed, like sending an onerror message.
> Might be somehow testable with an Internals API to create a 'fake' blob.

Right, this is not correct, old code called WebSocketChannel::fail() which sends an onerror message, but in the new code path it sends Close message to the network process. What we want here is actually didReceiveMessageError(), or make fail() call m_client->didReceiveMessageError(); before sending the Close message.

[Carlos Garcia Campos]

Created attachment 373016 [details]
Patch

Updated patch. It simplifies the way buffered amount is handled, so that we don't need to keep completion handlers around when queuing messages.

[Carlos Garcia Campos]

Created attachment 373019 [details]
Try to fix apple builds

Just guessing what the problem is...

[Carlos Garcia Campos]

Created attachment 373029 [details]
Try to fix apple builds

[Carlos Garcia Campos]

Created attachment 373031 [details]
Try to fix apple builds

[Carlos Garcia Campos]

Created attachment 373095 [details]
Try to fix apple builds

[youenn fablet]

Comment on attachment 373095 [details]
Try to fix apple builds

r=me.

Since we might want the same mechanism for DataChannel, I wonder though whether we should not have a more generic mechanism.
Something like a Queue where we can push String/ArrayBuffer/Blob.
The queue could be query whether empty or not.
It would take a Function callback to notify that a Variant<String, ArrayBuffer> is ready to be sent.

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

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:253
> +

As per spec, I think we should increase the buffered amount by blob.size() synchronously.
And decrement it when receiving the IPC message.
This does not seem to be like this in the previous implementation though.
It might be nice to have a test for that and check what other browsers are doing.

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:264
> +                sendMessage(Messages::NetworkSocketChannel::SendData { IPC::DataReference { reinterpret_cast<const uint8_t*>(binaryData.data()), binaryData.size() } }, binaryData.size());

Maybe we should have routines for these sendMessage(text) sendMessage(binary).

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:277
> +                    fail(makeString("Failed to load Blob: error code = ", errorCode.value()));

I am not sure this error handling is good enough.
If there is no test coverage for that case, we should file a bug.

[Carlos Garcia Campos]

Comment on attachment 373095 [details]
Try to fix apple builds

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

>> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:253
>> +
> 
> As per spec, I think we should increase the buffered amount by blob.size() synchronously.
> And decrement it when receiving the IPC message.
> This does not seem to be like this in the previous implementation though.
> It might be nice to have a test for that and check what other browsers are doing.

I thought about that, but decided to keep the current behaviour. Before the blob is loaded we haven't buffered any data yet, so I think it makes sense to increase it after the blob loaded.

>> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:264
>> +                sendMessage(Messages::NetworkSocketChannel::SendData { IPC::DataReference { reinterpret_cast<const uint8_t*>(binaryData.data()), binaryData.size() } }, binaryData.size());
> 
> Maybe we should have routines for these sendMessage(text) sendMessage(binary).

Instead of the template or in addition to?

>> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:277
>> +                    fail(makeString("Failed to load Blob: error code = ", errorCode.value()));
> 
> I am not sure this error handling is good enough.
> If there is no test coverage for that case, we should file a bug.

It's not easy to work with the tests when most of them are failing and we this code path is not used in the bots. If I add a new test for this, we might need to change the old code too. In any case, we are not generating console messages yet.

[Carlos Garcia Campos]

(In reply to youenn fablet from comment #12)
> Comment on attachment 373095 [details]
> Try to fix apple builds
> 
> r=me.
> 
> Since we might want the same mechanism for DataChannel, I wonder though
> whether we should not have a more generic mechanism.
> Something like a Queue where we can push String/ArrayBuffer/Blob.
> The queue could be query whether empty or not.
> It would take a Function callback to notify that a Variant<String,
> ArrayBuffer> is ready to be sent.
> 

What's DataChannel? I'll check if we can refactor to share the same code then.

[Carlos Garcia Campos]

Committed r246964: <https://trac.webkit.org/changeset/246964>

[Radar WebKit Bug Importer]

<rdar://problem/52445572>