Bug 238740

Created attachment 456575 [details]
WIP patch

Created attachment 457196 [details]
Patch

(In reply to Zan Dobersek (Reviews) from comment #2)
> Created attachment 457196 [details]
> Patch

This works as intended when going through StreamServerConnection::sendSyncReply(), but not for sync messages on a basic Connection.

Created attachment 457204 [details]
Patch

Now includes the same magic for Connection-based reply sending.

Comment on attachment 457204 [details]
Patch

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

good stuff.

I'm not 100% sure if my suggestions all make sense, but maybe some simplification of the templates could be possible still.

Incidentally I'm working on slowly, or dreaming about, supporting rvalue types in the send side, see the other bugs

> Source/WebKit/Platform/IPC/HandleMessage.h:177
> +struct CompletionHandlerTypeDeduction;

If I understand correctly:
This function is used for two purposes
1) extract the message handler function type
2) extract the completion handler type.

Would it make sense to have separate names for these? These seem two different tasks..
See also below..

> Source/WebKit/Platform/IPC/HandleMessage.h:183
> +template<typename Result, typename ClassType, typename... ParameterTypes>

these templates are textually rather dense.
You could check if you can rename the template parameter types according to the existing naming style of the file.
E.g. some ideas:
 Return could be R or Out
 ParameterTypes could be named Args or In
 ClassType could be C

> Source/WebKit/Platform/IPC/HandleMessage.h:200
> +struct CompletionHandlerTypeDeduction<CompletionHandler<Result(ParameterTypes...)>> {

This appears to be an implementation of a CompletionHandler feature:
- What is the return value of the completion handler?
- What is the parameters tuple of the completion handler?

It doesn't really make sense to write this template from scratch every time this is needed..
Would this make sense to be added to CompletionHandler itself?
Eg.
CompletionHandler::In = ..
CompleitonHandler::Out = ..

> Source/WebKit/Platform/IPC/HandleMessage.h:231
> +        return std::is_void_v<HandlerReturnType> && std::is_void_v<ReplyReturnType>

If it simplifies things, I don't think it's especially crucial to assert that the completionhandler has void return value

it is not useful to assert that the ReplyReturnType has void value, as that's part of what is being autogenerated and as such "always correct".

> Source/WebKit/Platform/IPC/HandleMessage.h:280
> +

You might be able to simplify some of the dense templates or remove them by thinking about the problem this way:

static_assert(std::is_same_v<RemoveCVRefsFromTuple<CompletionHandlerFromMF::InTypes>, T::ReplyArguments)

e.g. you figure out a way to go from MF to reply arguments list, and then you check "is the reply arguments list user has compatible with the reply arguments list the message reply has".

> Source/WebKit/Platform/IPC/StreamServerConnection.h:138
> +                if ((messageEncoder << ... << std::forward<Arguments>(arguments)))

this one cannot pass as rvalue...

> Source/WebKit/Platform/IPC/StreamServerConnection.h:146
> +    (encoder.get() << ... << std::forward<Arguments>(arguments));

.. as the same object may be accessed here.

There's no optimization opportunities above, as the objects are by nature of "data" kind, as the stream is just a blob of memory.

<rdar://problem/91567779>

Comment on attachment 457204 [details]
Patch

This is looks very good but marking as r- just for Zan to see if it's possible to simplify the templates.

Another suggestion:
In case it's not useful to add the CompletionHandler::InTuple and a new template is used:
When doing the template for getting the CompletionHandler arguments as tuple, you could see if you can make the template match "something that is invokable" instead of "something that is a CompletionHandler". This way it would be more re-usable.

std::invoke_result can extract the result type out of the "something that's invokable".

Comment on attachment 457204 [details]
Patch

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

>> Source/WebKit/Platform/IPC/HandleMessage.h:177
>> +struct CompletionHandlerTypeDeduction;
> 
> If I understand correctly:
> This function is used for two purposes
> 1) extract the message handler function type
> 2) extract the completion handler type.
> 
> Would it make sense to have separate names for these? These seem two different tasks..
> See also below..

OK.

>> Source/WebKit/Platform/IPC/HandleMessage.h:183
>> +template<typename Result, typename ClassType, typename... ParameterTypes>
> 
> these templates are textually rather dense.
> You could check if you can rename the template parameter types according to the existing naming style of the file.
> E.g. some ideas:
>  Return could be R or Out
>  ParameterTypes could be named Args or In
>  ClassType could be C

OK.

>> Source/WebKit/Platform/IPC/HandleMessage.h:200
>> +struct CompletionHandlerTypeDeduction<CompletionHandler<Result(ParameterTypes...)>> {
> 
> This appears to be an implementation of a CompletionHandler feature:
> - What is the return value of the completion handler?
> - What is the parameters tuple of the completion handler?
> 
> It doesn't really make sense to write this template from scratch every time this is needed..
> Would this make sense to be added to CompletionHandler itself?
> Eg.
> CompletionHandler::In = ..
> CompleitonHandler::Out = ..

OK.

>> Source/WebKit/Platform/IPC/HandleMessage.h:231
>> +        return std::is_void_v<HandlerReturnType> && std::is_void_v<ReplyReturnType>
> 
> If it simplifies things, I don't think it's especially crucial to assert that the completionhandler has void return value
> 
> it is not useful to assert that the ReplyReturnType has void value, as that's part of what is being autogenerated and as such "always correct".

I think I can check/enforce the void return type in the template specialization, i.e. not bother with the Result type either in the specialization or here.

>> Source/WebKit/Platform/IPC/StreamServerConnection.h:146
>> +    (encoder.get() << ... << std::forward<Arguments>(arguments));
> 
> .. as the same object may be accessed here.
> 
> There's no optimization opportunities above, as the objects are by nature of "data" kind, as the stream is just a blob of memory.

Ugh, my bad.

(In reply to Kimmo Kinnunen from comment #7)
> Another suggestion:
> In case it's not useful to add the CompletionHandler::InTuple and a new
> template is used:
> When doing the template for getting the CompletionHandler arguments as
> tuple, you could see if you can make the template match "something that is
> invokable" instead of "something that is a CompletionHandler". This way it
> would be more re-usable.
> 
> std::invoke_result can extract the result type out of the "something that's
> invokable".

I would enforce detecting a CompletionHandler object since the implementation of it tests that the invocation was done exactly once, as would be required for an IPC reply. CompletionHandler itself is a wrapper around any callable object, so I would just leave to it to handle everything, and place type aliases on the class to more easily access the return and parameter types.

Created attachment 457331 [details]
Patch

Comment on attachment 457331 [details]
Patch

Clearing flags on attachment: 457331

Committed r292863 (249634@trunk): <https://commits.webkit.org/249634@trunk>

All reviewed patches have been landed.  Closing bug.

Hmmm, GCC 12 doesn't like it:

In file included from /home/mcatanzaro/Projects/WebKit/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:33,
                 from /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/DerivedSources/WebKit/unified-sources/UnifiedSource-88d1702b-2.cpp:7:
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/HandleMessage.h: In instantiation of ‘static constexpr bool IPC::CompletionHandlerValidation::matchingTypes() [with CHType = WTF::CompletionHandler<void(const WebKit::WebBackForwardListCounts&)>; ReplyType = WTF::CompletionHandler<void(const WebKit::WebBackForwardListCounts&)>]’:
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/HandleMessage.h:275:121:   required from ‘bool IPC::handleMessageSynchronous(Connection&, Decoder&, WTF::UniqueRef<Encoder>&, C*, MF) [with T = Messages::WebPageProxy::BackForwardGoToItem; C = WebKit::ProvisionalPageProxy; MF = void (WebKit::ProvisionalPageProxy::*)(const WebCore::BackForwardItemIdentifier&, WTF::CompletionHandler<void(const WebKit::WebBackForwardListCounts&)>&&)]’
/home/mcatanzaro/Projects/WebKit/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:609:90:   required from here
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/HandleMessage.h:218:15: error: no type named ‘InTypes’ in ‘class WTF::CompletionHandler<void(const WebKit::WebBackForwardListCounts&)>’
  218 |         using CHInTypes = typename CHType::InTypes;
      |               ^~~~~~~~~
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/HandleMessage.h:219:15: error: no type named ‘InTypes’ in ‘class WTF::CompletionHandler<void(const WebKit::WebBackForwardListCounts&)>’
  219 |         using ReplyInTypes = typename ReplyType::InTypes;
      |               ^~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/HandleMessage.h:223:90: error: no type named ‘InTypes’ in ‘class WTF::CompletionHandler<void(const WebKit::WebBackForwardListCounts&)>’
  223 |             && matchingParameters<CHInTypes, ReplyInTypes>(std::make_index_sequence<std::tuple_size_v<CHInTypes>>());
      |                                                                                     ~~~~~^~~~~~~~~~~~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/HandleMessage.h: In instantiation of ‘bool IPC::handleMessageSynchronous(Connection&, Decoder&, WTF::UniqueRef<Encoder>&, C*, MF) [with T = Messages::WebPageProxy::BackForwardGoToItem; C = WebKit::ProvisionalPageProxy; MF = void (WebKit::ProvisionalPageProxy::*)(const WebCore::BackForwardItemIdentifier&, WTF::CompletionHandler<void(const WebKit::WebBackForwardListCounts&)>&&)]’:
/home/mcatanzaro/Projects/WebKit/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:609:90:   required from here
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/HandleMessage.h:275:121: error: non-constant condition for static assertion
  275 |     static_assert(CompletionHandlerValidation::template matchingTypes<CompletionHandlerFromMF, typename T::DelayedReply>());
      |                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/HandleMessage.h:275:121:   in ‘constexpr’ expansion of ‘IPC::CompletionHandlerValidation::matchingTypes<WTF::CompletionHandler<void(const WebKit::WebBackForwardListCounts&)>, WTF::CompletionHandler<void(const WebKit::WebBackForwardListCounts&)> >()’
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/HandleMessage.h:275:121: error: ‘constexpr’ call flows off the end of the function
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/HandleMessage.h: In instantiation of ‘static constexpr bool IPC::CompletionHandlerValidation::matchingTypes() [with CHType = WTF::CompletionHandler<void(const WebKit::PolicyDecision&)>; ReplyType = WTF::CompletionHandler<void(const WebKit::PolicyDecision&)>]’:
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/HandleMessage.h:275:121:   required from ‘bool IPC::handleMessageSynchronous(Connection&, Decoder&, WTF::UniqueRef<Encoder>&, C*, MF) [with T = Messages::WebPageProxy::DecidePolicyForNavigationActionSync; C = WebKit::ProvisionalPageProxy; MF = void (WebKit::ProvisionalPageProxy::*)(WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, bool, WebKit::FrameInfoData&&, WebCore::PolicyCheckIdentifier, long unsigned int, WebKit::NavigationActionData&&, WebKit::FrameInfoData&&, std::optional<WTF::ObjectIdentifier<WebKit::WebPageProxyIdentifierType> >, const WebCore::ResourceRequest&, WebCore::ResourceRequest&&, FormDataReference&&, WebCore::ResourceResponse&&, const WebKit::UserData&, WTF::CompletionHandler<void(const WebKit::PolicyDecision&)>&&)]’
/home/mcatanzaro/Projects/WebKit/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:612:106:   required from here
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/HandleMessage.h:218:15: error: no type named ‘InTypes’ in ‘class WTF::CompletionHandler<void(const WebKit::PolicyDecision&)>’
  218 |         using CHInTypes = typename CHType::InTypes;
      |               ^~~~~~~~~
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/HandleMessage.h:219:15: error: no type named ‘InTypes’ in ‘class WTF::CompletionHandler<void(const WebKit::PolicyDecision&)>’
  219 |         using ReplyInTypes = typename ReplyType::InTypes;
      |               ^~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/HandleMessage.h:223:90: error: no type named ‘InTypes’ in ‘class WTF::CompletionHandler<void(const WebKit::PolicyDecision&)>’
  223 |             && matchingParameters<CHInTypes, ReplyInTypes>(std::make_index_sequence<std::tuple_size_v<CHInTypes>>());
      |                                                                                     ~~~~~^~~~~~~~~~~~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/HandleMessage.h: In instantiation of ‘bool IPC::handleMessageSynchronous(Connection&, Decoder&, WTF::UniqueRef<Encoder>&, C*, MF) [with T = Messages::WebPageProxy::DecidePolicyForNavigationActionSync; C = WebKit::ProvisionalPageProxy; MF = void (WebKit::ProvisionalPageProxy::*)(WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, bool, WebKit::FrameInfoData&&, WebCore::PolicyCheckIdentifier, long unsigned int, WebKit::NavigationActionData&&, WebKit::FrameInfoData&&, std::optional<WTF::ObjectIdentifier<WebKit::WebPageProxyIdentifierType> >, const WebCore::ResourceRequest&, WebCore::ResourceRequest&&, FormDataReference&&, WebCore::ResourceResponse&&, const WebKit::UserData&, WTF::CompletionHandler<void(const WebKit::PolicyDecision&)>&&)]’:
/home/mcatanzaro/Projects/WebKit/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:612:106:   required from here
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/HandleMessage.h:275:121: error: non-constant condition for static assertion
  275 |     static_assert(CompletionHandlerValidation::template matchingTypes<CompletionHandlerFromMF, typename T::DelayedReply>());
      |                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/HandleMessage.h:275:121:   in ‘constexpr’ expansion of ‘IPC::CompletionHandlerValidation::matchingTypes<WTF::CompletionHandler<void(const WebKit::PolicyDecision&)>, WTF::CompletionHandler<void(const WebKit::PolicyDecision&)> >()’
/home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/HandleMessage.h:275:121: error: ‘constexpr’ call flows off the end of the function

Mmmmm, maybe this was already fixed? It's working fine for me now, but I didn't do a clean build, so it's not an incremental build issue. Odd. Whatever.

This patch broke compilation in GCC8.3:

FAILED: Source/WebKit/CMakeFiles/WebKit.dir/__/__/DerivedSources/WebKit/WebFullScreenManagerProxyMessageReceiver.cpp.o
/bin/ccache /usr/lib/ccache/g++-8  -DBUILDING_GTK__=1 -DBUILDING_WEBKIT -DBUILDING_WITH_CMAKE=1 -DBUILDING_WebKit -DBWRAP_EXECUTABLE=\"/bin/bwrap\" -DDATADIR=\"/usr/local/share\" -DDBUS_PROXY_EXECUTABLE=\"/bin/xdg-dbus-proxy\" -DGETTEXT_PACKAGE=\"WebKit2GTK-4.0\" -DHAVE_CONFIG_H=1 -DJSC_GLIB_API_ENABLED -DLIBDIR=\"/usr/local/lib\
In file included from DerivedSources/WebKit/WebFullScreenManagerProxyMessageReceiver.cpp:30:
../../Source/WebKit/Platform/IPC/HandleMessage.h:186:23: error: ‘remove_cvref_t’ in namespace ‘std’ does not name a template type
     using Type = std::remove_cvref_t<typename std::tuple_element_t<N, std::tuple<Args...>>>;
                       ^~~~~~~~~~~~~~
../../Source/WebKit/Platform/IPC/HandleMessage.h:186:18: note: suggested alternative: ‘remove_cv_t’
     using Type = std::remove_cvref_t<typename std::tuple_element_t<N, std::tuple<Args...>>>;
                  ^~~

std::remove_cvref_t is only supported since >=GCC9. See:

  https://en.cppreference.com/w/cpp/compiler_support/20

GCC8.3 is used by Debian Stable which we still need to support until September 2022.

Please find an alternative implementation to std::remove_cvref_t.

(In reply to Diego Pino from comment #15)
> Please find an alternative implementation to std::remove_cvref_t.

Does std::remove_

Try std::remove_cvref<typename std::tuple_element_t<N, std::tuple<Args...>>>::type.

(In reply to Diego Pino from comment #15)
> This patch broke compilation in GCC8.3:
> 
> FAILED:
> Source/WebKit/CMakeFiles/WebKit.dir/__/__/DerivedSources/WebKit/
> WebFullScreenManagerProxyMessageReceiver.cpp.o
> /bin/ccache /usr/lib/ccache/g++-8  -DBUILDING_GTK__=1 -DBUILDING_WEBKIT
> -DBUILDING_WITH_CMAKE=1 -DBUILDING_WebKit -DBWRAP_EXECUTABLE=\"/bin/bwrap\"
> -DDATADIR=\"/usr/local/share\"
> -DDBUS_PROXY_EXECUTABLE=\"/bin/xdg-dbus-proxy\"
> -DGETTEXT_PACKAGE=\"WebKit2GTK-4.0\" -DHAVE_CONFIG_H=1
> -DJSC_GLIB_API_ENABLED -DLIBDIR=\"/usr/local/lib\
> In file included from
> DerivedSources/WebKit/WebFullScreenManagerProxyMessageReceiver.cpp:30:
> ../../Source/WebKit/Platform/IPC/HandleMessage.h:186:23: error:
> ‘remove_cvref_t’ in namespace ‘std’ does not name a template type
>      using Type = std::remove_cvref_t<typename std::tuple_element_t<N,
> std::tuple<Args...>>>;
>                        ^~~~~~~~~~~~~~
> ../../Source/WebKit/Platform/IPC/HandleMessage.h:186:18: note: suggested
> alternative: ‘remove_cv_t’
>      using Type = std::remove_cvref_t<typename std::tuple_element_t<N,
> std::tuple<Args...>>>;
>                   ^~~
> 
> std::remove_cvref_t is only supported since >=GCC9. See:
> 
>   https://en.cppreference.com/w/cpp/compiler_support/20
> 
> GCC8.3 is used by Debian Stable which we still need to support until
> September 2022.
> 
> Please find an alternative implementation to std::remove_cvref_t.

We could do a check for it and then if not present we use the suggested implementation from https://en.cppreference.com/w/cpp/types/remove_cvref and add that to wtf/StdLibExtras.h

This is what we did on PlayStation since we did not have that file.

(In reply to Don Olmstead from comment #17)

> We could do a check for it and then if not present we use the suggested
> implementation from https://en.cppreference.com/w/cpp/types/remove_cvref and
> add that to wtf/StdLibExtras.h
> 
> This is what we did on PlayStation since we did not have that file.

Addressed issue on follow-up bug https://bugs.webkit.org/show_bug.cgi?id=239482