Bug 190746 – Introduce CompletionHandler-based Async IPC messages with replies

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

[Alex Christensen]

Introduce CompletionHandler-based Async IPC messages with replies

[Alex Christensen]

Created attachment 352773 [details]
Patch

[Alex Christensen]

Created attachment 352774 [details]
Patch

[Alex Christensen]

Created attachment 352783 [details]
Patch

[Alex Christensen]

Tim wants to know from Anders why this wasn't already done.

[Alex Christensen]

(like if there's a good reason to not do it or if it just hasn't been done yet)

[Tim Horton]

Comment on attachment 352783 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        What a mess.  No wonder people take shortcuts and make strange design decisions.

Hah. Ouch.

> Source/WebKit/ChangeLog:15
> +        layout test and many others.  I intent to refine and further adopt this incrementally.

You intent!

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.messages.in:59
> +    GetSandboxExtensionsForBlobFiles(Vector<String> paths) -> (WebKit::SandboxExtension::HandleArray extensions) Async

I feel like we should either invert the sync-by-default or make you always write one or the other. But that can be later.

[Alex Christensen]

Comment on attachment 352783 [details]
Patch

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

http://trac.webkit.org/r237294

> Source/WebKit/Platform/IPC/Connection.cpp:1137
> +CompletionHandler<void(Decoder*)> takeAsyncReplyHandler(Connection& connection, uint64_t identifier)

As suggested by Anders, I added the equivalent of MESSAGE_CHECK(map.isValidKey(identifier)) here because it's coming from an untrusted source.

[Alex Christensen]

http://trac.webkit.org/r237294

[Radar WebKit Bug Importer]

<rdar://problem/45409444>

[Alex Christensen]

http://trac.webkit.org/r237298

[Simon Fraser (smfr)]

Do we expect all callers to be on the main thread?

[Chris Dumez]

(In reply to Simon Fraser (smfr) from comment #11)
> Do we expect all callers to be on the main thread?

This definitely has not been a requirement so far for async IPC. I do not think we should add such requirement either.

[Simon Fraser (smfr)]

Do you need to protect asyncReplyHandlerMap() with a lock then?

[Chris Dumez]

(In reply to Simon Fraser (smfr) from comment #13)
> Do you need to protect asyncReplyHandlerMap() with a lock then?

I will look into the patch more but there might be other issues also:
- ID generation is not thread safe
- CompletionHandlers may not get called on the thread that scheduled them.

[Chris Dumez]

Comment on attachment 352783 [details]
Patch

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

> Source/WebKit/Platform/IPC/Connection.cpp:284
> +            handler(nullptr);

The Connection destructor is always called on the main thread so those completion handlers will be called on the main thread no matter on which thread they were scheduled.

> Source/WebKit/Platform/IPC/Connection.cpp:970
> +        auto handler = takeAsyncReplyHandler(*this, *listenerID);

Looks like the map is only used from the main thread here.

> Source/WebKit/Platform/IPC/Connection.h:419
> +void Connection::sendWithAsyncReply(T&& message, CompletionHandler<void(Args...)>&& completionHandler, uint64_t destinationID)

I see that you added a new method. We may want to add ASSERT(RunLoop::isMain()); in this method and in nextAsyncReplyHandlerID() and in asyncReplyHandlerMap() since those are not thread safe.

In the future we have have some use cases for calling this on background threads though as the regular send() allows this.

[Alex Christensen]

Adding assertions in https://bugs.webkit.org/show_bug.cgi?id=199324