Bug 201359

Summary: Abstract out WebSocketChannel message queue
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebRTCAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, cgarcia, commit-queue, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 201377    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description youenn fablet 2019-08-30 15:27:16 PDT
This can be used by RTCDataChannel as well when implementing send(Blob)
Comment 1 youenn fablet 2019-08-30 15:45:13 PDT
Created attachment 377757 [details]
Patch
Comment 2 Alex Christensen 2019-08-30 16:52:36 PDT
Comment on attachment 377757 [details]
Patch

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

> Source/WebCore/ChangeLog:25
> +        (WebCore::MessageQueueWithBlobs::MessageQueueWithBlobs):

I think this needs a better name, like NetworkSendQueue or something.  This currently looks related to WTF::MessageQueue, but it's not.

> Source/WebCore/fileapi/MessageQueueWithBlobs.h:46
> +    MessageQueueWithBlobs(Document&, Function<void(const String&)>&&, Function<void(const char*, size_t)>&&, Function<bool(int)>&&);

I think these functions need names in the header.  Why does notifyError return a bool?

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:56
> +    }, [&channel](const char* data, size_t byteLength) {
> +        channel.sendMessage(Messages::NetworkSocketChannel::SendData { IPC::DataReference { reinterpret_cast<const uint8_t*>(data), byteLength } }, byteLength);

This captures a raw pointer and uses it later.  I think we should capture a strong or weak reference instead.
Comment 3 youenn fablet 2019-08-30 23:38:49 PDT
(In reply to Alex Christensen from comment #2)
> Comment on attachment 377757 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377757&action=review
> 
> > Source/WebCore/ChangeLog:25
> > +        (WebCore::MessageQueueWithBlobs::MessageQueueWithBlobs):
> 
> I think this needs a better name, like NetworkSendQueue or something.  This
> currently looks related to WTF::MessageQueue, but it's not.

OK.
 
> > Source/WebCore/fileapi/MessageQueueWithBlobs.h:46
> > +    MessageQueueWithBlobs(Document&, Function<void(const String&)>&&, Function<void(const char*, size_t)>&&, Function<bool(int)>&&);
> 
> I think these functions need names in the header.  Why does notifyError
> return a bool?

Will add names, notifyError returns a boolean to determine whether processing messages should stop after an error or not.
I will change it to processError.

> > Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:56
> > +    }, [&channel](const char* data, size_t byteLength) {
> > +        channel.sendMessage(Messages::NetworkSocketChannel::SendData { IPC::DataReference { reinterpret_cast<const uint8_t*>(data), byteLength } }, byteLength);
> 
> This captures a raw pointer and uses it later.  I think we should capture a
> strong or weak reference instead.

channel is owning the MessageQueueWithBlobs so the lambdas will be destroyed at channel destruction time.
The only tricky part is the case of the lambdas being called during the destruction of channel.
I believe this is handled by the ABORT_ERROR check.
Comment 4 youenn fablet 2019-08-31 00:04:31 PDT
Created attachment 377788 [details]
Patch
Comment 5 youenn fablet 2019-08-31 04:05:23 PDT
Created attachment 377796 [details]
Patch
Comment 6 WebKit Commit Bot 2019-09-04 09:47:01 PDT
Comment on attachment 377796 [details]
Patch

Clearing flags on attachment: 377796

Committed r249481: <https://trac.webkit.org/changeset/249481>
Comment 7 WebKit Commit Bot 2019-09-04 09:47:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2019-09-04 09:48:19 PDT
<rdar://problem/55023788>