Bug 227740 – Allow custom IPC::Attachment messaging in ConnectionUnix.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Imanol Fernandez]

We need to pass AHardwareBuffer instance from the UIProcess to the WebProcess, which requires calling functions such as AHardwareBuffer_sendHandleToUnixSocket(). By adding custom IPC::Attachmentmessaging we can call the AHardwareBuffer helper functions to pass and get AHardwareBuffers via IPC

[Imanol Fernandez]

Created attachment 433021 [details]
Patch

[Michael Catanzaro]

Comment on attachment 433021 [details]
Patch

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

> Source/WebKit/Platform/IPC/unix/ConnectionUnix.cpp:59
> -#endif // SOCK_SEQPACKET
> +#endif // SOCK_SEQPACKET 
>  
> -namespace IPC {
> +namespace IPC { 

I assume there was some accident with this patch? :)

[Imanol Fernandez]

yes, it was just a compilation test to discard what it looked like a intermittent compilation error bug 226919

I'll upload the real patch soon :)

[Imanol Fernandez]

Created attachment 434836 [details]
Patch

Implement CustomWriterType Attachment

[Imanol Fernandez]

This is an example of how the API can be used to send aHardwareBuffer via IPC:

void ArgumentCoder<PlatformXR::AHardwareBufferWrapper>::encode(Encoder& encoder, const PlatformXR::AHardwareBufferWrapper& wrapper)
{
    auto hardwarebuffer = wrapper.hardwarebuffer;
    Attachment attachment(Attachment::CustomWriter { [hardwarebuffer](int socketDescriptor) {
        while (true) {
            int ret = AHardwareBuffer_sendHandleToUnixSocket(hardwarebuffer, socketDescriptor);
            if (!ret || ret != -EAGAIN)
                break;
        }
    }});
    encoder << attachment;
}

bool ArgumentCoder<PlatformXR::AHardwareBufferWrapper>::decode(Decoder& decoder, PlatformXR::AHardwareBufferWrapper& wrapper)
{
    Attachment attachment;
    if (!decoder.decode(attachment))
        return false;
    auto socketDescriptor = WTF::get<Attachment::SocketDescriptor>(attachment.customWriter());
    while (true) {
        auto ret = AHardwareBuffer_recvHandleFromUnixSocket(socketDescriptor, &wrapper.hardwarebuffer);
        if (!ret || ret != -EAGAIN)
            break;
    }

    return true;
}

[Imanol Fernandez]

Created attachment 435068 [details]
Patch

Use WTF::function instead of std::function

[Zan Dobersek (Reviews)]

Comment on attachment 435068 [details]
Patch

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

> Source/WebKit/Platform/IPC/unix/ConnectionUnix.cpp:593
> +        for (size_t i = 0; i < attachments.size(); ++i) {

for (auto& attachment : attachments)

[Imanol Fernandez]

Created attachment 435176 [details]
Patch for landing

[Imanol Fernandez]

Created attachment 435177 [details]
Patch for landing

[EWS]

Committed r280769 (240353@main): <https://commits.webkit.org/240353@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 435177 [details].

[Kimmo Kinnunen]

Did the client for this ever land?
I think this is a fair deviation of the intended purpose of Attachment, and maybe forms undue cognitive overhead while maintenance.

Hanging arbitrary function closure to the IPC message attachment is cumbersome as it's unclear what is the contract to call the function. What thread is it ok to call? Is it ok to call many times? What's the lifetime of the closed variables?

In the example in the comments, there's a potential to try to access a already freed ANB. Message send can be delayed, and as such it's not guaranteed that the ANB pointer closed by the function is alive at the time of actual message send.

Some time in the future the non-Cocoa and Cocoa parts could/should/must share more of the implementation. In these cases, it's not ok to run arbitrary code, as the send may happen in other thread.

The intended use of IPC::Attachment is that all objects that cannot be transferred as pure data are stored in IPC::Attachment.
In this case, it'd be better to store an owning reference to a ANB in the Attachment, and then explicitly say
 "Connection knows how to transfer UnixFileDescriptor" (e.g this is already there)
 "Connection knows how to transfer ANB" (e.g. you make ConnectionUnix.cpp call AHardwareBuffer_sendHandleToUnixSocket et al)


So Unix variant of Connection would have 
class Attachment {

    std::variant<UnixFileDescriptor
#if USE(ANDROID_NATIVE_BUFFER)
        , Ref<AHardwareBuffer>
#endif
    >
};


(Ref<AHardwareBuffer> uses AHardwareBuffer_acquire, AHardwareBuffer_release to implement ref(), unref())