Bug 238516 - RemoteRenderingBackend should have dedicated IPC::Connection for out-of-stream messages
Summary: RemoteRenderingBackend should have dedicated IPC::Connection for out-of-strea...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kimmo Kinnunen
URL:
Keywords: InRadar
Depends on: 238608 238618 238622
Blocks: 237674
  Show dependency treegraph
 
Reported: 2022-03-29 11:29 PDT by Kimmo Kinnunen
Modified: 2022-04-19 13:22 PDT (History)
5 users (show)

See Also:


Attachments
wip for wed (21.35 KB, patch)
2022-03-29 11:31 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (48.35 KB, patch)
2022-04-01 04:49 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (52.20 KB, patch)
2022-04-01 05:38 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (50.79 KB, patch)
2022-04-07 05:34 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
For landing (52.32 KB, patch)
2022-04-08 01:40 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
For landing (52.31 KB, patch)
2022-04-08 02:02 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
For landing (52.01 KB, patch)
2022-04-11 05:56 PDT, Kimmo Kinnunen
kkinnunen: commit-queue+
Details | Formatted Diff | Diff
[fast-cq] For landing (52.02 KB, patch)
2022-04-12 02:48 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kimmo Kinnunen 2022-03-29 11:29:34 PDT
RemoteRenderingBackend should have dedicated IPC::Connection for out-of-stream messages
Comment 1 Kimmo Kinnunen 2022-03-29 11:31:30 PDT
Created attachment 456046 [details]
wip for wed
Comment 2 Kimmo Kinnunen 2022-04-01 04:49:05 PDT
Created attachment 456348 [details]
Patch
Comment 3 Kimmo Kinnunen 2022-04-01 05:38:35 PDT
Created attachment 456353 [details]
Patch
Comment 4 Radar WebKit Bug Importer 2022-04-05 11:30:17 PDT
<rdar://problem/91306116>
Comment 5 Kimmo Kinnunen 2022-04-07 05:34:15 PDT
Created attachment 456913 [details]
Patch
Comment 6 Simon Fraser (smfr) 2022-04-07 10:47:23 PDT
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
Comment 7 Kimmo Kinnunen 2022-04-08 01:38:53 PDT
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.
Comment 8 Kimmo Kinnunen 2022-04-08 01:40:04 PDT
Created attachment 457030 [details]
For landing
Comment 9 Kimmo Kinnunen 2022-04-08 02:02:55 PDT
Created attachment 457031 [details]
For landing
Comment 10 Kimmo Kinnunen 2022-04-11 05:56:16 PDT
Created attachment 457250 [details]
For landing
Comment 11 Kimmo Kinnunen 2022-04-12 02:48:05 PDT
Created attachment 457323 [details]
[fast-cq] For landing
Comment 12 EWS 2022-04-13 00:30:26 PDT
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].
Comment 13 Truitt Savell 2022-04-19 10:52:59 PDT
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
Comment 14 Fujii Hironori 2022-04-19 13:22:59 PDT
One more regression:
  Bug 239488 – WinCairo WebKit2 significant performance regression