Bug 201746 – AuxiliaryProcessProxy::sendWithAsyncReply should queue up messages if sent while the process is starting like it does messages without replies

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

AuxiliaryProcessProxy::sendWithAsyncReply should queue up messages if sent while the process is starting like it does messages without replies

[Alex Christensen]

Created attachment 378696 [details]
Patch

[Chris Dumez]

Comment on attachment 378696 [details]
Patch

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

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:152
> +        if (auto&& info = WTFMove(asyncReplyInfo))

Not sure why this isn't simply:
if (asyncReplyInfo)

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:222
> +        if (auto&& info = WTFMove(pendingMessage.asyncReplyInfo))

ditto.

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.h:175
> +    sendMessage(WTFMove(encoder), sendOptions, {{ [completionHandler = WTFMove(completionHandler)] (IPC::Decoder* decoder) mutable {

If sendMessage() returns false (because the process has been terminated, it looks to me that the completion handler will never be called. We should probably T::cancelReply.

[Chris Dumez]

Comment on attachment 378696 [details]
Patch

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

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:153
> +            IPC::addAsyncReplyHandler(*connection(), info->second, WTFMove(info->first));

and then WTFMove(asyncReplyInfo->first) here.

[Alex Christensen]

Created attachment 378716 [details]
Patch

[youenn fablet]

Comment on attachment 378716 [details]
Patch

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

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:213
> +        OptionSet<IPC::SendOption> sendOptions = pendingMessage.sendOptions;

both can be auto.

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:230
>          m_connection->sendMessage(WTFMove(message), sendOptions);

I guess sendMessage can only return true here.
I would assert it though. In theory, we could add the async reply handler that would only complete once the connection is destroyed.

[Chris Dumez]

Comment on attachment 378716 [details]
Patch

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

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:211
> +    for (auto&& pendingMessage : std::exchange(m_pendingMessages, { })) {

Why auto&& and not auto? You're doing a std::exchange().

[Alex Christensen]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 378716 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378716&action=review
> 
> > Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:211
> > +    for (auto&& pendingMessage : std::exchange(m_pendingMessages, { })) {
> 
> Why auto&& and not auto? You're doing a std::exchange().

Otherwise I get this:
error: call to implicitly-deleted copy constructor of 'WebKit::AuxiliaryProcessProxy::PendingMessage'

[Alex Christensen]

http://trac.webkit.org/r249835

[Radar WebKit Bug Importer]

<rdar://problem/55340747>

[Chris Dumez]

Comment on attachment 378716 [details]
Patch

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

>>> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:211
>>> +    for (auto&& pendingMessage : std::exchange(m_pendingMessages, { })) {
>> 
>> Why auto&& and not auto? You're doing a std::exchange().
> 
> Otherwise I get this:
> error: call to implicitly-deleted copy constructor of 'WebKit::AuxiliaryProcessProxy::PendingMessage'

Ah, should probably be auto& then. I missed that it was a for() and not an if().