[webkit-reviews] review granted: [Bug 238516] RemoteRenderingBackend should have dedicated IPC::Connection for out-of-stream messages : [Attachment 456913] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Kimmo Kinnunen
<kkinnunen at apple.com>'s request for review:
Bug 238516: RemoteRenderingBackend should have dedicated IPC::Connection for
out-of-stream messages
https://bugs.webkit.org/show_bug.cgi?id=238516

Attachment 456913: Patch

https://bugs.webkit.org/attachment.cgi?id=456913&action=review




--- Comment #6 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 456913
  --> https://bugs.webkit.org/attachment.cgi?id=456913
Patch

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

> Source/WebKit/ChangeLog:9
> +	   of using the WP-GPUP main connection. RemoteDisplayListRecorder
starts listenting to

"listenting"

> Source/WebKit/ChangeLog:14
> +	   is a "message queue". This way when each out-of-stream message
marker is processed, the

"in a"?

> Source/WebKit/ChangeLog:36
> +	   After, untrusted WP instantiates these IPC stream dedicated
connections.
> +	   This is accounted in the case where IPC::Connection ensures that
IPC::MessageNames::InitializeConnection
> +	   message is not acted on twice.

Is this a problem?

> Source/WebKit/Platform/IPC/StreamClientConnection.cpp:56
> +    // For Connection, "server" means the connection which was created
first, the connection which is not sent through IPC to other party.
> +    // For Connection, "client" means the connection which was established
by receiving it through IPC and creating IPC::Connection out from the
identifier.
> +    // For StreamClientConnection, "Client" means the party that mostly does
sending, e.g. untrusted party.
> +    // For StreamServerConnection, "Server" means the party that mostly does
receiving, e.g. the trusted party which holds the destination object to
communicate with.

This is confusing. For StreamClientConnection can we call the "sender" and
"receiver" instead?

> Source/WebKit/Platform/IPC/StreamClientConnection.cpp:63
> +StreamClientConnection::StreamClientConnection(Ref<Connection>&& connection,
size_t size, std::unique_ptr<DedicatedConnectionClient>&&
dedicatedConnectionClient)

size -> bufferSize

> Source/WebKit/Platform/IPC/StreamClientConnection.h:66
> +    // Creates StreamClientConnection where the out of stream messages and
server replies are
> +    // sent through the passed IPC::Connection. The messages from the server
are delivered to
> +    // the caller through the passed IPC::Connection.
> +    // Note: This function should be used only in cases where the
> +    // stream server starts listening to messages with new identifiers on
the same thread as
> +    // in which the server IPC::Connection dispatch messages. At the time of
writing,
> +    // IPC::Connection dispatches messages only in main thread.

Which code paths use this non-dedicated connection setup?

> Source/WebKit/Platform/IPC/StreamServerConnection.cpp:36
> +// Part 2. of the workaround for not having non-dispatching Connection. We
have a dummy Connection::Client which is expected to
> +// never get any messages.

This would be better as // FIXME: Need to have non-dispatching Connections
webkit.org/b/NNNN

> Source/WebKit/Platform/IPC/StreamServerConnection.cpp:95
> +	   // Part 1. of the workaround for not having non-dispatching
Connection. Divert all messages to the receive queue.

This would be better as // FIXME: Need to have non-dispatching Connections
webkit.org/b/NNNN

> Source/WebKit/Platform/IPC/StreamServerConnection.h:68
> +    // Creates StreamClientConnection where the out of stream messages and
server replies are
> +    // received through the passed IPC::Connection. The messages from the
server are sent to
> +    // the passed IPC::Connection.
> +    // Note: This function should be used only in cases where the
> +    // stream server starts listening to messages with new identifiers on
the same thread as
> +    // in which the server IPC::Connection dispatch messages. At the time of
writing,
> +    // IPC::Connection dispatches messages only in main thread.
> +    static Ref<StreamServerConnection> create(Connection&,
StreamConnectionBuffer&&, StreamConnectionWorkQueue&);
> +
> +    // Creates StreamServerConnection where the out of stream messages and
server replies are
> +    // received through a dedidcated, new IPC::Connection. The messages from
the server are sent to
> +    // the dedicated conneciton.
> +    static Ref<StreamServerConnection>
createWithDedicatedConnection(Attachment&& connectionIdentifier,
StreamConnectionBuffer&&, StreamConnectionWorkQueue&);

Do we need the repeated comments?

> Source/WebKit/Platform/IPC/StreamServerConnection.h:90
> +    enum class HasDedicatedConnection { No, Yes };

: bool