Bug 48606 – Connection::sendSyncMessage needs to dispatch incoming sync messages

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Anders Carlsson]

Connection::sendSyncMessage needs to dispatch incoming sync messages

[Anders Carlsson]

Created attachment 72282 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 72282 [details]
Patch

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(syncMessagesReceivedWhileWaitingForSyncReply);
> +
> +            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.

[Anders Carlsson]

Committed r70893: <http://trac.webkit.org/changeset/70893>