Bug 238516 – RemoteRenderingBackend should have dedicated IPC::Connection for out-of-stream messages

• Component: Canvas
• Status: RESOLVED FIXED

[Kimmo Kinnunen]

RemoteRenderingBackend should have dedicated IPC::Connection for out-of-stream messages

[Kimmo Kinnunen]

Created attachment 456046 [details]
wip for wed

[Kimmo Kinnunen]

Created attachment 456348 [details]
Patch

[Kimmo Kinnunen]

Created attachment 456353 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/91306116>

[Kimmo Kinnunen]

Created attachment 456913 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 456913 [details]
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

[Kimmo Kinnunen]

Thanks!

(In reply to Simon Fraser (smfr) from comment #6)
> > 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?

This was just a remark why the change in IPC::Connection was made.

> 
> > 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?

This was just a remark trying why "server" Connection is being given tho Stream*Client*Connection.
Rephrased, to be a bit clearer.

Might be good to rename StreamClientConnection and StreamServerConnection back to *Sender* and *Receiver* connection like they originally were, but maybe not in this commit.



> 
> > 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?

WebGL and WebGPU for now.

[Kimmo Kinnunen]

Created attachment 457030 [details]
For landing

[Kimmo Kinnunen]

Created attachment 457031 [details]
For landing

[Kimmo Kinnunen]

Created attachment 457250 [details]
For landing

[Kimmo Kinnunen]

Created attachment 457323 [details]
[fast-cq] For landing

[EWS]

Committed r292803 (249585@main): <https://commits.webkit.org/249585@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 457323 [details].

[Truitt Savell]

The changed in https://github.com/WebKit/WebKit/commit/df139fb58a6cf833efe7bc3d5e83e9b3e46cd486

broke this API test TestWebKitAPI.IPCTestingAPI.CanReceiveIPCSemaphore

tracking in https://bugs.webkit.org/show_bug.cgi?id=239507

[Fujii Hironori]

One more regression:
  Bug 239488 – WinCairo WebKit2 significant performance regression