Bug 48606 - Connection::sendSyncMessage needs to dispatch incoming sync messages
Summary: Connection::sendSyncMessage needs to dispatch incoming sync messages
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Anders Carlsson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-28 18:34 PDT by Anders Carlsson
Modified: 2010-10-29 10:25 PDT (History)
0 users

See Also:


Attachments
Patch (6.13 KB, patch)
2010-10-28 18:38 PDT, Anders Carlsson
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anders Carlsson 2010-10-28 18:34:42 PDT
Connection::sendSyncMessage needs to dispatch incoming sync messages
Comment 1 Anders Carlsson 2010-10-28 18:38:51 PDT
Created attachment 72282 [details]
Patch
Comment 2 Adam Roben (:aroben) 2010-10-28 19:12:20 PDT
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.
Comment 3 Anders Carlsson 2010-10-29 10:25:49 PDT
Committed r70893: <http://trac.webkit.org/changeset/70893>