Bug 201746 - AuxiliaryProcessProxy::sendWithAsyncReply should queue up messages if sent while the process is starting like it does messages without replies
Summary: AuxiliaryProcessProxy::sendWithAsyncReply should queue up messages if sent wh...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-12 18:00 PDT by Alex Christensen
Modified: 2019-09-13 09:07 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.71 KB, patch)
2019-09-12 18:01 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (7.00 KB, patch)
2019-09-12 22:42 PDT, Alex Christensen
youennf: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2019-09-12 18:00:56 PDT
AuxiliaryProcessProxy::sendWithAsyncReply should queue up messages if sent while the process is starting like it does messages without replies
Comment 1 Alex Christensen 2019-09-12 18:01:07 PDT
Created attachment 378696 [details]
Patch
Comment 2 Chris Dumez 2019-09-12 18:11:28 PDT
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.
Comment 3 Chris Dumez 2019-09-12 18:17:40 PDT
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.
Comment 4 Alex Christensen 2019-09-12 22:42:42 PDT
Created attachment 378716 [details]
Patch
Comment 5 youenn fablet 2019-09-13 05:26:44 PDT
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.
Comment 6 Chris Dumez 2019-09-13 08:28:29 PDT
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().
Comment 7 Alex Christensen 2019-09-13 09:01:25 PDT
(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'
Comment 8 Alex Christensen 2019-09-13 09:03:29 PDT
http://trac.webkit.org/r249835
Comment 9 Radar WebKit Bug Importer 2019-09-13 09:04:15 PDT
<rdar://problem/55340747>
Comment 10 Chris Dumez 2019-09-13 09:07:07 PDT
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().