Bug 199189 - WebSockets: add support for sending blob messages when using web sockets platform APIs
Summary: WebSockets: add support for sending blob messages when using web sockets plat...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk, InRadar
Depends on:
Blocks:
 
Reported: 2019-06-25 04:24 PDT by Carlos Garcia Campos
Modified: 2019-07-01 01:10 PDT (History)
3 users (show)

See Also:


Attachments
Patch (6.34 KB, patch)
2019-06-25 04:28 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (11.33 KB, patch)
2019-06-26 04:50 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (13.99 KB, patch)
2019-06-27 04:19 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix apple builds (15.65 KB, patch)
2019-06-27 04:57 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix apple builds (16.71 KB, patch)
2019-06-27 06:42 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix apple builds (17.94 KB, patch)
2019-06-27 06:51 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix apple builds (19.26 KB, patch)
2019-06-28 00:55 PDT, Carlos Garcia Campos
youennf: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2019-06-25 04:24:38 PDT
It's currently unimplemented.
Comment 1 Carlos Garcia Campos 2019-06-25 04:28:46 PDT
Created attachment 372834 [details]
Patch
Comment 2 youenn fablet 2019-06-25 09:45:47 PDT
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.
Comment 3 Carlos Garcia Campos 2019-06-26 04:38:55 PDT
(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.
Comment 4 Carlos Garcia Campos 2019-06-26 04:50:28 PDT
Created attachment 372915 [details]
Patch
Comment 5 youenn fablet 2019-06-26 22:45:52 PDT
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.
Comment 6 Carlos Garcia Campos 2019-06-27 02:24:12 PDT
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.
Comment 7 Carlos Garcia Campos 2019-06-27 04:19:54 PDT
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.
Comment 8 Carlos Garcia Campos 2019-06-27 04:57:55 PDT
Created attachment 373019 [details]
Try to fix apple builds

Just guessing what the problem is...
Comment 9 Carlos Garcia Campos 2019-06-27 06:42:35 PDT
Created attachment 373029 [details]
Try to fix apple builds
Comment 10 Carlos Garcia Campos 2019-06-27 06:51:22 PDT
Created attachment 373031 [details]
Try to fix apple builds
Comment 11 Carlos Garcia Campos 2019-06-28 00:55:54 PDT
Created attachment 373095 [details]
Try to fix apple builds
Comment 12 youenn fablet 2019-06-28 11:35:02 PDT
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.
Comment 13 Carlos Garcia Campos 2019-07-01 01:07:19 PDT
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.
Comment 14 Carlos Garcia Campos 2019-07-01 01:08:44 PDT
(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.
Comment 15 Carlos Garcia Campos 2019-07-01 01:09:58 PDT
Committed r246964: <https://trac.webkit.org/changeset/246964>
Comment 16 Radar WebKit Bug Importer 2019-07-01 01:10:21 PDT
<rdar://problem/52445572>