Bug 231970 – RemoteRenderingBackend::CreateImageBuffer should be an async IPC stream message

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Wenson Hsieh]

Revert the workaround in https://bugs.webkit.org/show_bug.cgi?id=231681, and instead add some kind of mechanism to enqueue stream messages that don't yet have a receive queue.

[Wenson Hsieh]

Created attachment 441804 [details]
Patch

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #1)
> Created attachment 441804 [details]
> Patch

This patch is essentially the same as https://bugs.webkit.org/attachment.cgi?id=441755&action=review, except that I've renamed some methods and members to hopefully make these invariants explicit:

1. Incoming stream messages must either be added to a receive queue upon arrival, or added to `m_incomingStreamMessagesWithMissingReceiverQueues` if there is no extant receive queue.

2. All messages in `m_incomingStreamMessagesWithMissingReceiverQueues` are destined for a receive queue that will be added via (the now-renamed) `addStreamMessageReceiver()` method.

As far as I understand, this should address the concerns in https://bugs.webkit.org/show_bug.cgi?id=231681#c27, regarding:

> ...There are normal IPC messages that are sent to MessageReceiveQueues or Connection::Client...

...since we do not expect any "normal" (i.e. non-stream) IPC messages to be sent to MessageReceiveQueues that are used to receive stream messages.

[Wenson Hsieh]

Additionally — I started going down the path of adding new public methods on IPC::Connection to register a list of ReceiverNames as being "expected to add receiver queues" and using this list to add messages on a `m_incomingMessagesWithMissingReceiverQueues`.

However, since I would just be adding the stream message receiver names here anyways, I think it's ultimately more straightforward to treat all stream message receivers in this way (hence, why I'm using this new `isStreamMessageReceiver` function instead of new Connection methods, or a new message receiver attribute in the message file).

[Kimmo Kinnunen]

I'm still not very fond of the solution.
I think there should not be fundamental difference between "stream receiver" and "receiver".

I don't think there should be a feature where you can send a message to unknown receiver and have the receiving connection save that message in perpetuity.

I don't think it's a good idea to add yet another hard to understand deviation of how the messages are delivered.

The same issue, safe-guarding user-created race condition problem, exists in normal IPC and it looks odd if streams are the only place this is solved.

So if we have a scenario where we forget streams for a minute, and consider the same feature for normal IPC messages:
"I want to send messages to unknown receivers."

Should we fix that issue for normal IPC?
I don't think so. 

If you write a race condition in normal IPC, you have the same problem as a race condition in stream-based IPC.

How pressing of an issue is this to fix?

Can we think of a general purpose short-term mechanism that solves this? For long term I think ordered queue per receiver should solve it.

I think best short-term would be to declare:
"Queue q is the queue for RRB messages with my id"
"Queue q is the queue for RDLR messages with any id"

I think that should be done by saying something like:

void RemoteRenderingBackend::startListeningForIPC()
{
    m_gpuConnectionToWebProcess->connection().addWorkQueueMessageReceiver(Messages::RemoteRenderingBackend::messageReceiverName(), m_workQueue, this, m_renderingBackendIdentifier.toUInt64());
    m_gpuConnectionToWebProcess->connection().addWorkQueueMessageReceiver(Messages::RemoteDisplayListRecorder::messageReceiverName(), m_workQueue, this, 0);
}

Then:
- Make receive queues registrable in all threads (I can do this too, but it's simple WorkQueue change IIRC to the degree anything with threads is simple..)
- Change RDLR to register the message listener in the thread instead of posting to main thread
- Add a some sort of rerouting massaging hook for the message processing so that it'd be understandable how the dispatch goes to the RDLR. I can try to sketch something for this 


Then maybe catch the message in the RRB dispatchMessages, inspect the

[Kimmo Kinnunen]

... Had my wrong computer with older checkout.

void RemoteRenderingBackend::startListeningForIPC()
{
    m_streamConnection->startReceivingMessages(*this, Messages::RemoteRenderingBackend::messageReceiverName(), m_renderingBackendIdentifier.toUInt64());
    m_streamConnection->startReceivingMessages(Messages::RemoteDisplayListRecorder::messageReceiverName()); //add
}

...



// add this
void StreamServerConnection::startReceivingMessages(ReceiverName receiverName)
{
    {
        Locker locker { m_receiversLock };
        ASSERT(!m_wildcardReceivers[receiverName]);
        m_wildcardReceivers[receiverName] = true;
    }
    m_connection->addMessageReceiveQueue(*this, receiverName, 0);

}

Maybe with this the messages appear to the RRB thread correctly. Haven't tried it, so it might be completely off.

[Darin Adler]

Comment on attachment 441804 [details]
Patch

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

> Source/WebKit/Platform/IPC/Connection.h:236
> +    void removeStreamMessageReceiver(ReceiverName receiverName, uint64_t destinationID = 0)
> +    {
> +        ASSERT(isStreamMessageReceiver(receiverName));
> +        removeMessageReceiveQueue(receiverName, destinationID);
> +    }

Maybe just a personal preference of mine, but I would like it better if a multi-line inline function was defined outside the class definition, making it a little easier to see an overview of what the class provides rather than having that interspersed with implementation. So I would put the implementation below.

If you do that, I would also suggest we add the keyword "inline" to start of the declaration; not something we did in the past, but a good practice to start going forward. Doing that will let us move the inlines to a separate header in the future if we wish to, and give us a compile time error if we don’t include that header. So it’s a good practice to start with any function that has an inline implementation outside the class definiton. We could wait and add that keyword only if we decide to move the function body to a separate header, but I think that specifying inline in the declarations in such cases may be a good habit to cultivate for future maintainability.

> Source/WebKit/Platform/IPC/Connection.h:424
>      Deque<std::unique_ptr<Decoder>> m_incomingMessages WTF_GUARDED_BY_LOCK(m_incomingMessagesLock);
> +    Deque<std::unique_ptr<Decoder>> m_incomingStreamMessagesWithMissingReceiverQueues WTF_GUARDED_BY_LOCK(m_incomingMessagesLock);

I’m not sure I understand why we use a Deque here instead of a Vector. As I understand it, the primary feature a Deque offers that a Vector does not is efficient removal at the front. I don’t see code here that removes from the front, but maybe this is done on the main m_incomingMessages somewhere I did not spot and somehow indirectly that’s the same for the new new duque.

I’m sure that I’m just missing the place where this occurs, but wanted to call it to your intention in case it’s not so.

[Wenson Hsieh]

Comment on attachment 441804 [details]
Patch

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

Thanks for taking a look, Darin and Kimmo! I'll restructure this patch to account for Kimmo's feedback, and upload a new (different) version soon.

>> Source/WebKit/Platform/IPC/Connection.h:236
>> +    }
> 
> Maybe just a personal preference of mine, but I would like it better if a multi-line inline function was defined outside the class definition, making it a little easier to see an overview of what the class provides rather than having that interspersed with implementation. So I would put the implementation below.
> 
> If you do that, I would also suggest we add the keyword "inline" to start of the declaration; not something we did in the past, but a good practice to start going forward. Doing that will let us move the inlines to a separate header in the future if we wish to, and give us a compile time error if we don’t include that header. So it’s a good practice to start with any function that has an inline implementation outside the class definiton. We could wait and add that keyword only if we decide to move the function body to a separate header, but I think that specifying inline in the declarations in such cases may be a good habit to cultivate for future maintainability.

This piece of code is going away in the new version of the patch, but I will note that moving forward!

>> Source/WebKit/Platform/IPC/Connection.h:424
>> +    Deque<std::unique_ptr<Decoder>> m_incomingStreamMessagesWithMissingReceiverQueues WTF_GUARDED_BY_LOCK(m_incomingMessagesLock);
> 
> I’m not sure I understand why we use a Deque here instead of a Vector. As I understand it, the primary feature a Deque offers that a Vector does not is efficient removal at the front. I don’t see code here that removes from the front, but maybe this is done on the main m_incomingMessages somewhere I did not spot and somehow indirectly that’s the same for the new new duque.
> 
> I’m sure that I’m just missing the place where this occurs, but wanted to call it to your intention in case it’s not so.

That's a good point — we do `m_incomingMessages.takeFirst()` in several places, but (in this patch, at least) we only iterate sequentially through `m_incomingStreamMessagesWithMissingReceiverQueues`, so it should indeed be a Vector.

[Wenson Hsieh]

(In reply to Kimmo Kinnunen from comment #5)
> ... Had my wrong computer with older checkout.
> 
> void RemoteRenderingBackend::startListeningForIPC()
> {
>     m_streamConnection->startReceivingMessages(*this,
> Messages::RemoteRenderingBackend::messageReceiverName(),
> m_renderingBackendIdentifier.toUInt64());
>    
> m_streamConnection->startReceivingMessages(Messages::
> RemoteDisplayListRecorder::messageReceiverName()); //add
> }
> 
> ...
> 
> 
> 
> // add this
> void StreamServerConnection::startReceivingMessages(ReceiverName
> receiverName)
> {
>     {
>         Locker locker { m_receiversLock };
>         ASSERT(!m_wildcardReceivers[receiverName]);
>         m_wildcardReceivers[receiverName] = true;
>     }
>     m_connection->addMessageReceiveQueue(*this, receiverName, 0);
> 
> }
> 
> Maybe with this the messages appear to the RRB thread correctly. Haven't
> tried it, so it might be completely off.

I'm trying to implement your approach now, but it's not clear to me what purpose `m_wildcardReceivers` is intended to serve. It's also not clear to me exactly what this means:

> ..Then maybe catch the message in the RRB dispatchMessages..

Assuming "the message" refers to a queued message that was missing a receiver, how would this message be routed to RRB (since the ReceiverNames would not match up)? Are you proposing that we add some kind of mechanism for an IPC message receiver to receive messages that are intended not for itself, but instead for another receiver that it can forward messages to?

I think I understand your high-level approach (i.e. "install a 0-destination receive queue for RDLR to catch and later enqueue any incoming receiver-less messages"), but I'm confused by some of these details in your previous comments.

I'll attempt to come up with a patch that captures the spirit of your approach, and let's go from there..

[Wenson Hsieh]

Created attachment 441958 [details]
Alternate approach?

[Wenson Hsieh]

Created attachment 441970 [details]
Alternate approach, #2

[Kimmo Kinnunen]

Comment on attachment 441970 [details]
Alternate approach, #2

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

Nice!

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:95
> +    m_streamConnection->startReceivingMessages(Messages::RemoteDisplayListRecorder::messageReceiverName());

So I remember this after two weeks, could you add a comment about the subtleness?
I can come up with something like below, but feel free to word it differently too.
// RemoteDisplayListRecorder messages depend on RemoteRenderingBackend, because RemoteRenderingBackend creates RemoteDisplayListRecorder and
// makes a receive queue for it. In order to guarantee correct ordering, ensure that all RemoteDisplayListRecorder messages are processed in the same sequence
// as RemoteRenderingBackend.

> Source/WebKit/Platform/IPC/StreamServerConnection.cpp:43
> +    // FIXME: Can we avoid synchronous dispatch here by adjusting the assertion in `Connection::enqueueMatchingMessagesToMessageReceiveQueue`?

Yeah, this should be done, along with fixing m_workQueue.addStreamConnection to be thread-safe.

[Wenson Hsieh]

Comment on attachment 441970 [details]
Alternate approach, #2

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

Thanks for taking a look! I'll have a new version out soon with a ChangeLog.

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:95
>> +    m_streamConnection->startReceivingMessages(Messages::RemoteDisplayListRecorder::messageReceiverName());
> 
> So I remember this after two weeks, could you add a comment about the subtleness?
> I can come up with something like below, but feel free to word it differently too.
> // RemoteDisplayListRecorder messages depend on RemoteRenderingBackend, because RemoteRenderingBackend creates RemoteDisplayListRecorder and
> // makes a receive queue for it. In order to guarantee correct ordering, ensure that all RemoteDisplayListRecorder messages are processed in the same sequence
> // as RemoteRenderingBackend.

Added!

>> Source/WebKit/Platform/IPC/StreamServerConnection.cpp:43
>> +    // FIXME: Can we avoid synchronous dispatch here by adjusting the assertion in `Connection::enqueueMatchingMessagesToMessageReceiveQueue`?
> 
> Yeah, this should be done, along with fixing m_workQueue.addStreamConnection to be thread-safe.

As far as I understood, `addStreamConnection()` is going to be invoked from the main runloop in all cases.

Are you suggesting that we guard the call to `wakeUpProcessingThread();` with `StreamConnectionWorkQueue::m_lock` so that we don't end up in a situation where we `wakeUpProcessingThread()` simultaneously from different threads due to `StreamConnectionWorkQueue::dispatch`?

(i.e., something like:)

```
void StreamConnectionWorkQueue::dispatch(WTF::Function<void()>&& function)
{
    Locker locker { m_lock };
    m_functions.append(WTFMove(function));
    ASSERT(!m_shouldQuit); // Re-entering during shutdown not supported.
    wakeUpProcessingThread();
}

void StreamConnectionWorkQueue::addStreamConnection(StreamServerConnectionBase& connection)
{
    Locker locker { m_lock };
    m_connections.add(connection);
    ASSERT(!m_shouldQuit); // Re-entering during shutdown not supported.
    wakeUpProcessingThread();
}
```

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #12)
> Comment on attachment 441970 [details]
> Alternate approach, #2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441970&action=review
> 
> Thanks for taking a look! I'll have a new version out soon with a ChangeLog.
> 
> >> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:95
> >> +    m_streamConnection->startReceivingMessages(Messages::RemoteDisplayListRecorder::messageReceiverName());
> > 
> > So I remember this after two weeks, could you add a comment about the subtleness?
> > I can come up with something like below, but feel free to word it differently too.
> > // RemoteDisplayListRecorder messages depend on RemoteRenderingBackend, because RemoteRenderingBackend creates RemoteDisplayListRecorder and
> > // makes a receive queue for it. In order to guarantee correct ordering, ensure that all RemoteDisplayListRecorder messages are processed in the same sequence
> > // as RemoteRenderingBackend.
> 
> Added!
> 
> >> Source/WebKit/Platform/IPC/StreamServerConnection.cpp:43
> >> +    // FIXME: Can we avoid synchronous dispatch here by adjusting the assertion in `Connection::enqueueMatchingMessagesToMessageReceiveQueue`?
> > 
> > Yeah, this should be done, along with fixing m_workQueue.addStreamConnection to be thread-safe.
> 
> As far as I understood, `addStreamConnection()` is going to be invoked from
> the main runloop in all cases.
> 
> Are you suggesting that we guard the call to `wakeUpProcessingThread();`
> with `StreamConnectionWorkQueue::m_lock` so that we don't end up in a
> situation where we `wakeUpProcessingThread()` simultaneously from different
> threads due to `StreamConnectionWorkQueue::dispatch`?
> 
> (i.e., something like:)
> 
> ```
> void StreamConnectionWorkQueue::dispatch(WTF::Function<void()>&& function)
> {
>     Locker locker { m_lock };
>     m_functions.append(WTFMove(function));
>     ASSERT(!m_shouldQuit); // Re-entering during shutdown not supported.
>     wakeUpProcessingThread();
> }
> 
> void
> StreamConnectionWorkQueue::addStreamConnection(StreamServerConnectionBase&
> connection)
> {
>     Locker locker { m_lock };
>     m_connections.add(connection);
>     ASSERT(!m_shouldQuit); // Re-entering during shutdown not supported.
>     wakeUpProcessingThread();
> }
> ```

Never mind — I see what you mean in https://bugs.webkit.org/show_bug.cgi?id=232068 now.

[Wenson Hsieh]

Created attachment 442040 [details]
Depends on #232068

[EWS]

Committed r284695 (243414@main): <https://commits.webkit.org/243414@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/84556289>