Bug 106708 – [WK2] Make it possible to send sync messages from secondary threads

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Alexey Proskuryakov]

It's much easier to send sync messages from background threads, because one doesn't need to be concerned about blocking incoming messages. But it's not currently possible.

[Alexey Proskuryakov]

Created attachment 182434 [details]
proposed patch

[WebKit Review Bot]

Attachment 182434 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/Platform/CoreIPC/Connection.h:185:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 182434 [details]
proposed patch

Attachment 182434 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15832068

[Anders Carlsson]

Comment on attachment 182434 [details]
proposed patch

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

> Source/WebKit2/Platform/CoreIPC/Connection.cpp:98
> +    // The reply decoder, will be null if there was an error processing the sync
> +    // message on the other side.

No need to break this comment.

> Source/WebKit2/Platform/CoreIPC/Connection.cpp:435
> +        didFailToSendSyncMessage();

I think's weird that didFailToSendSyncMessage can now be called from any thread. I think just returning null is better.

> Source/WebKit2/Platform/CoreIPC/Connection.cpp:469
> +    if (!pendingReply.replyDecoder)
> +        didFailToSendSyncMessage();

Ditto.

> Source/WebKit2/Platform/CoreIPC/Connection.cpp:557
> +    // If it's not a reply to any primary thread message, check if it is a reply to a secondary thread one.
> +    SecondaryThreadPendingSyncReplyMap::iterator secondaryThreadReplyMapItem = m_secondaryThreadPendingSyncReplyMap.find(decoder->destinationID());
> +    if (secondaryThreadReplyMapItem != m_secondaryThreadPendingSyncReplyMap.end()) {
> +        SecondaryThreadPendingSyncReply* reply = secondaryThreadReplyMapItem->value;
> +        ASSERT(!reply->replyDecoder);
> +        reply->replyDecoder = decoder.leakPtr();
> +        reply->semaphore.signal();
> +    }

I think this needs to be protected by the mutex m_syncReplyState mutex.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/r139514> and <http://trac.webkit.org/r139515>.

> I think's weird that didFailToSendSyncMessage can now be called from any thread. I think just returning null is better.

OK. Also, this will be used by new code that will hopefully be prepared for connection failures.

> I think this needs to be protected by the mutex m_syncReplyState mutex.

Actually, the whole function is protected with this mutex.

[Csaba Osztrogonác]

(In reply to comment #5)
> Committed <http://trac.webkit.org/r139514> and <http://trac.webkit.org/r139515>.

It broke all GCC WK2 builds, please see https://bugs.webkit.org/show_bug.cgi?id=106729. I uploaded the proposed fix into it.

[Csaba Osztrogonác]

MIPS GCC still needs fix: https://bugs.webkit.org/show_bug.cgi?id=106739
Windows still needs fix: https://bugs.webkit.org/show_bug.cgi?id=106740