[webkit-reviews] review granted: [Bug 48606] Connection::sendSyncMessage needs to dispatch incoming sync messages : [Attachment 72282] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Anders Carlsson
<andersca at apple.com>'s request for review:
Bug 48606: Connection::sendSyncMessage needs to dispatch incoming sync messages
https://bugs.webkit.org/show_bug.cgi?id=48606

Attachment 72282: Patch
https://bugs.webkit.org/attachment.cgi?id=72282&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=72282&action=review

> WebKit2/Platform/CoreIPC/Connection.cpp:235
> -	       // First, check if there is a sync reply at the top of the
stack.
> +	       // First, check if we have any incoming sync messages that we
need to process.
> +	       Vector<IncomingMessage>
syncMessagesReceivedWhileWaitingForSyncReply;
> +	      
m_syncMessagesReceivedWhileWaitingForSyncReply.swap(syncMessagesReceivedWhileWa
itingForSyncReply);
> +
> +	       if (!syncMessagesReceivedWhileWaitingForSyncReply.isEmpty()) {
> +		   // Make sure to unlock the mutex here because we're calling
out to client code which could in turn send
> +		   // another sync message and we don't want that to deadlock.
> +		   m_syncReplyStateMutex.unlock();
> +		   
> +		   for (size_t i = 0; i <
syncMessagesReceivedWhileWaitingForSyncReply.size(); ++i) {
> +		       IncomingMessage& message =
syncMessagesReceivedWhileWaitingForSyncReply[i];
> +		       OwnPtr<ArgumentDecoder> arguments =
message.releaseArguments();
> +
> +		       dispatchSyncMessage(message.messageID(),
arguments.get());
> +		   }
> +		   m_syncReplyStateMutex.lock();
> +	       }
> +
> +	       // Second, check if there is a sync reply at the top of the
stack.

I think it would be clearer to use two separately-scoped MutexLockers instead
of manually unlocking/locking. I guess you save an unlock/lock in the case
where we haven't received any sync messages; maybe that's important?

> WebKit2/Platform/CoreIPC/Connection.cpp:281
> +	       // Add this message and wake up the client thread.
> +	       m_waitForSyncReplySemaphore.signal();
> +	       return;

The "Add this message" part of this comment seems misplaced, since you already
added it.