[webkit-reviews] review canceled: [Bug 231681] REGRESSION (r284079): fast/canvas/gradient-with-clip.html and fast/canvas/gradient-text-with-shadow.html are flaky failures : [Attachment 441755] Async approach, v2

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has canceled Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 231681: REGRESSION (r284079): fast/canvas/gradient-with-clip.html and
fast/canvas/gradient-text-with-shadow.html are flaky failures
https://bugs.webkit.org/show_bug.cgi?id=231681

Attachment 441755: Async approach, v2

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




--- Comment #24 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 441755
  --> https://bugs.webkit.org/attachment.cgi?id=441755
Async approach, v2

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

>> Source/WebKit/Platform/IPC/Connection.cpp:362
>> +	enqueueMatchingMessagesToMessageReceiveQueue(receiveQueue,
m_incomingReceiveQueueMessages, receiverName, destinationID);
> 
> Receive queue is not a stream-specific thing.
> Everything that receives messages off main thread is a receive queue.
> This now dispatches only the pending stream messages to the newly added
receive queues.

I think it's true that stream messages should only ever be enqueued on receive
queues, though? (In other words, I don't think there should ever be
`m_incomingMessages` that are destined for a `MessageReceiveQueue`).

>> Source/WebKit/Platform/IPC/Connection.cpp:772
>> +	if (isStreamMessageReceiver(message->messageReceiverName())) {
> 
> I don't think generally this sort of special casing of "is it stream or is it
not" is warranted.
> 
> The root of the problem is that there are two different message receivers.
> These message receivers are not supposed to be sequential, nothing guarantees
it.
> These are not guaranteed to be sequential, so we shouldn't  try to find
stop-gap measures to make them so.
> 
> If we want two message receivers be sequential, we need to define the
mechanism as an API guarantee which makes it happen. Then we need to implement
that.

I agree that it's a bit fragile, but IIUC `isStreamMessageReceiver()` implies
that the message should end up in a receive queue, currently.

Instead of checking for `isStreamMessageReceiver`, we could instead either:
1. Add a mechanism for the owner of the Connection (e.g.
`GPUConnectionToWebProcess`) to indicate that certain ReceiverNames are
expected to always end up in receive queues, or
2. Bake this into the grammar of `*.messages.in` as a new receiver attribute

In lieu of these alternatives, could we temporarily address the flaky test
failures by landing the other mitigation (making CreateImageBuffer sync) first?