Calling waitForAndDispatchImmediately<M> on a loop fails when multiple M messages arrive simultaneously

• Component: WebKit2
• Status: RESOLVED FIXED

[Wenson Hsieh]

Fix a race condition in IPC logic, wherein a client that calls waitForAndDispatchImmediately<M>() n times may time out when receiving n messages of type M.

...more concretely:

RemoteImageBufferProxy sync-waits for consecutive `RemoteRenderingBackendProxy::DidFlush` messages, and finishes sync-waiting when the DidFlush message containing the expected flush identifier arrives. The following may occur:

(Main thread): waitForAndDispatchImmediately<RemoteRenderingBackendProxy::DidFlush>()

(IPC thread): receives the first DidFlush message
(IPC thread): sees that the message name matches that of m_waitingForMessage, and signals the main thread

(Main thread): sees that m_waitingForMessage now has a decoder, and nulls out m_waitingForMessage
(Main thread): calls into RemoteRenderingBackendProxy::didFlush() and returns from waitForAndDispatchImmediately

(IPC thread): receives the second DidFlush message
(IPC thread): sees that m_waitingForMessage is null

(Main thread): calls into RemoteRenderingBackendProxy::didFlush() and returns from waitForAndDispatchImmediately
(Main thread): sees that the flush is still pending, so we call waitForAndDispatchImmediately<RemoteRenderingBackendProxy::DidFlush>() again
(Main thread): sees that the incoming messages queue doesn't contain DidFlush, so we wait

(IPC thread): enqueues the incoming DidFlush message onto the main thread (via enqueueIncomingMessage)

...and now the main thread will time out waiting for the RemoteRenderingBackendProxy::DidFlush which is currently sitting in the incoming messages queue :|

[Wenson Hsieh]

Created attachment 414812 [details]
Patch

[Chris Dumez]

Comment on attachment 414812 [details]
Patch

r=me assuming the bots are happy.

[Wenson Hsieh]

Comment on attachment 414812 [details]
Patch

Thanks for the review!

[EWS]

Committed r270180: <https://trac.webkit.org/changeset/270180>

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

[Radar WebKit Bug Importer]

<rdar://problem/71757243>

[Geoffrey Garen]

Comment on attachment 414812 [details]
Patch

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

> Source/WebKit/Platform/IPC/Connection.cpp:553
> +        if (hasIncomingSynchronousMessage && waitForOptions.contains(WaitForOption::InterruptWaitingIfSyncMessageArrives)) {

Since we needed to hold a lock while computing hasIncomingSynchronousMessage, it seems like it must be a bug to check hasIncomingSynchronousMessage after dropping the lock. Between dropping the lock and checking hasIncomingSynchronousMessage, the true value of hasIncomingSynchronousMessage may have changed (because a new incoming message may have arrived).

[Geoffrey Garen]

Comment on attachment 414812 [details]
Patch

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

>> Source/WebKit/Platform/IPC/Connection.cpp:553
>> +        if (hasIncomingSynchronousMessage && waitForOptions.contains(WaitForOption::InterruptWaitingIfSyncMessageArrives)) {
> 
> Since we needed to hold a lock while computing hasIncomingSynchronousMessage, it seems like it must be a bug to check hasIncomingSynchronousMessage after dropping the lock. Between dropping the lock and checking hasIncomingSynchronousMessage, the true value of hasIncomingSynchronousMessage may have changed (because a new incoming message may have arrived).

But I guess this is OK because there is only one IPC thread, and we are running on it, and we know that only the IPC thread adds to m_incomingMessages?

[Chris Dumez]

(In reply to Geoffrey Garen from comment #7)
> Comment on attachment 414812 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414812&action=review
> 
> >> Source/WebKit/Platform/IPC/Connection.cpp:553
> >> +        if (hasIncomingSynchronousMessage && waitForOptions.contains(WaitForOption::InterruptWaitingIfSyncMessageArrives)) {
> > 
> > Since we needed to hold a lock while computing hasIncomingSynchronousMessage, it seems like it must be a bug to check hasIncomingSynchronousMessage after dropping the lock. Between dropping the lock and checking hasIncomingSynchronousMessage, the true value of hasIncomingSynchronousMessage may have changed (because a new incoming message may have arrived).
> 
> But I guess this is OK because there is only one IPC thread, and we are
> running on it, and we know that only the IPC thread adds to
> m_incomingMessages?

2 things:
a. This is not new behavior in this patch
b. Yes, it is my understanding that incoming messages only get enqueued by the IPC thread and we are on the IPC thread here. The reason we have a lock is because the main thread dequeues those messages.