Bug 202847 – [GTK][WPE] Add user messages API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

We have never exposed an API to send/receive message to/from Web extensions to allow applications use their own IPC. Now with PSON enabled it's a lot more difficult to implement the custom IPC on the application side, because applications need to keep track of all the web processes launched, and the changes of web process in the web view to send the messages to the right extension. That's already done internally by WebKit, so it would be easier to provide a simple API so that apps don't need to worry about the web process being used.

[Milan Crha]

The way I did it recently in Evolution was that I use JSC for the communication on both sides. If the script wants to say something to the client side, it does so through a custom signal of

> window.webkit.messageHandlers.MYSIGNAL.postMessage(data);

to which the client side listens. And when the client side wants to say something to the extension, (well, I do not talk to the extension, I only call javascript routines loaded in the extension), I just use javascript calls too.

I mean, there are ways to rely on WebKit's IPC already.

[Carlos Garcia Campos]

Created attachment 380746 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

For now, I've only used the unit tests to check the API. I'll port ephy to use it too.

[Carlos Garcia Campos]

(In reply to Milan Crha from comment #1)
> The way I did it recently in Evolution was that I use JSC for the
> communication on both sides. If the script wants to say something to the
> client side, it does so through a custom signal of
> 
> > window.webkit.messageHandlers.MYSIGNAL.postMessage(data);
> 
> to which the client side listens. And when the client side wants to say
> something to the extension, (well, I do not talk to the extension, I only
> call javascript routines loaded in the extension), I just use javascript
> calls too.
> 
> I mean, there are ways to rely on WebKit's IPC already.

Yes, that's what we have suggested to do for simple cases to avoid having to implement your own IPC. But that's more limited, and doesn't cover all the cases. Epiphany does the same, but it still needs to use DBus for other cases. The new API will make both cases a lot easier in any case.

[Adrian Perez]

Comment on attachment 380746 [details]
Patch

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

This looks really great, thanks for working on it, Carlos! I have
some use-cases in mind where this will come in very handy and allow
removing custom-written IPC code from applications. Kudos for reusing
GVariant to carry data around, and for future-proofing the feature by
allowing passing file descriptors around.

I just have one small nit/doubt/suggestion below :)

> Source/WebKit/Shared/API/glib/WebKitUserMessage.cpp:162
> +     * after the message has been sent.

Maybe we want to explicitly have a note indicating that...

  “If the sender needs to keep the file descriptor, `dup(2)`
   can be used to send a new descriptor referring to the same
   data.”

I suggest this because my first thought was “what if I need to keep
referring to some shared data in both processes” and it took me a
minute to realize how to do it. Maybe for many people it's obvious,
but it won't hurt pointing it out, I think.

> Source/WebKit/Shared/glib/UserMessage.cpp:50
> +        GRefPtr<GDBusMessage> message = adoptGRef(g_dbus_message_new_method_call(nullptr, "/", nullptr, "_"));

Is there any advantage here in abusing GDBusMessage over sending
directly a blob of bytes with the GVariant type and the payload?
Roughly, I am thinking of something like this:

  const GvariantType *payloadType = g_variant_get_type(parameters.get());
  encoder << IPC::DataReference(g_variant_type_peek_string(payloadType),
                                g_variant_type_get_string_length(payloadType));
  encoder << IPC::DataReference(g_variant_get_data(parameters.get(),
                                g_variant_get_size(parameters.get());

This doesn't incur in any allocations in the best case, and only one in the
worst case for the buffer where the GVariant gets serialized on demand if
needed. Using GDBusMessage involves an additional GObject (and its allocations
and deallocations), for no added benefit (I think).

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2105
> +     * and the message has been replied, the operation in the #WebKitWebPage will

From the context I suppose that this is supposed to be

  “…and the message has not been replied…”
                        ^^^

[Michael Catanzaro]

Comment on attachment 380746 [details]
Patch

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

>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2105
>> +     * and the message has been replied, the operation in the #WebKitWebPage will
> 
> From the context I suppose that this is supposed to be
> 
>   “…and the message has not been replied…”
>                         ^^^

"not been replied to"

[Carlos Garcia Campos]

Comment on attachment 380746 [details]
Patch

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

>> Source/WebKit/Shared/API/glib/WebKitUserMessage.cpp:162
>> +     * after the message has been sent.
> 
> Maybe we want to explicitly have a note indicating that...
> 
>   “If the sender needs to keep the file descriptor, `dup(2)`
>    can be used to send a new descriptor referring to the same
>    data.”
> 
> I suggest this because my first thought was “what if I need to keep
> referring to some shared data in both processes” and it took me a
> minute to realize how to do it. Maybe for many people it's obvious,
> but it won't hurt pointing it out, I think.

Or we could always dup and avoid confusions. I decided to steal mainly because I think of WebKitUserMessage as a temp object (that's also why it's initially unowned), that you don't care about after being sent.

>> Source/WebKit/Shared/glib/UserMessage.cpp:50
>> +        GRefPtr<GDBusMessage> message = adoptGRef(g_dbus_message_new_method_call(nullptr, "/", nullptr, "_"));
> 
> Is there any advantage here in abusing GDBusMessage over sending
> directly a blob of bytes with the GVariant type and the payload?
> Roughly, I am thinking of something like this:
> 
>   const GvariantType *payloadType = g_variant_get_type(parameters.get());
>   encoder << IPC::DataReference(g_variant_type_peek_string(payloadType),
>                                 g_variant_type_get_string_length(payloadType));
>   encoder << IPC::DataReference(g_variant_get_data(parameters.get(),
>                                 g_variant_get_size(parameters.get());
> 
> This doesn't incur in any allocations in the best case, and only one in the
> worst case for the buffer where the GVariant gets serialized on demand if
> needed. Using GDBusMessage involves an additional GObject (and its allocations
> and deallocations), for no added benefit (I think).

I decided to follow the same rules as a DBus message, and using GDBusMessage we avoided copying the serialization code. But there's actually no good reason for that, we could use GVariant data directly and remove all the restrictions of a DBus message (it must be a tuple with only complete types).

>>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2105
>>> +     * and the message has been replied, the operation in the #WebKitWebPage will
>> 
>> From the context I suppose that this is supposed to be
>> 
>>   “…and the message has not been replied…”
>>                         ^^^
> 
> "not been replied to"

Right!

[Carlos Garcia Campos]

Created attachment 380864 [details]
Patch

[Carlos Garcia Campos]

(In reply to Carlos Garcia Campos from comment #8)
> Comment on attachment 380746 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380746&action=review
> 
> >> Source/WebKit/Shared/API/glib/WebKitUserMessage.cpp:162
> >> +     * after the message has been sent.
> > 
> > Maybe we want to explicitly have a note indicating that...
> > 
> >   “If the sender needs to keep the file descriptor, `dup(2)`
> >    can be used to send a new descriptor referring to the same
> >    data.”
> > 
> > I suggest this because my first thought was “what if I need to keep
> > referring to some shared data in both processes” and it took me a
> > minute to realize how to do it. Maybe for many people it's obvious,
> > but it won't hurt pointing it out, I think.
> 
> Or we could always dup and avoid confusions. I decided to steal mainly
> because I think of WebKitUserMessage as a temp object (that's also why it's
> initially unowned), that you don't care about after being sent.

I've realized that we should always dup, otherwise it would be impossible to send a message with fds using webkit_web_context_send_message_to_all_extensions(). I'll update the patch.

[Carlos Garcia Campos]

Created attachment 380871 [details]
Patch

[Carlos Garcia Campos]

This is how ephy code would be using this API:

https://gitlab.gnome.org/GNOME/epiphany/commit/87578a124becb345b0079879562203a2674f243b

 10 files changed, 380 insertions(+), 999 deletions(-)
 delete mode 100644 embed/ephy-web-process-extension-proxy.c
 delete mode 100644 embed/ephy-web-process-extension-proxy.h

[Carlos Garcia Campos]

Created attachment 380972 [details]
Patch

[Michael Catanzaro]

Comment on attachment 380972 [details]
Patch

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

LGTM. I'll leave r+ for Adrian, though.

> Source/WebKit/Shared/API/glib/WebKitUserMessage.cpp:46
> + * and Web extensions. A WebKitUserMessage always have a name, and it can also include parameters and

We've never capitalized web before, at least not in web process or web extension. I wouldn't start now.

Also: have -> has

> Source/WebKit/Shared/API/glib/WebKitUserMessage.cpp:49
> + * corresponding #WebKitWebPage (and vice versa). One to one messages can be replied directly with

"replied to"

> Source/WebKit/Shared/API/glib/WebKitUserMessage.cpp:299
> +    GRefPtr<WebKitUserMessage> adoptedReply = reply;

Probably a good idea to add your one-line comment about sinking the message here, since you did everywhere else, and it's not obvious.

> Source/WebKit/Shared/glib/ArgumentCodersGLib.cpp:46
> +bool decode(Decoder& decoder, GRefPtr<GVariant>& variant)

Shouldn't it return Optional<GRefPtr<GVariant>&>, using Optional return instead of an out parameter, as you did for the UserMessage class? I think this is an old-style decoder that we're supposed to be moving away from, when you used the modern style in UserMessage.

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp:162
> +     * replied. Calling webkit_user_message_send_reply() will do nothing.

replied to.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:586
> +    void sendMedssageToAllExtensions(WebKitUserMessage* message)

Medssage -> Message

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:641
> +static bool readFileDescritpor(int fd, char** contents, gsize* length)

Descritpor -> Descriptor

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #14)
> Comment on attachment 380972 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380972&action=review
> 
> LGTM. I'll leave r+ for Adrian, though.

Thanks!

> > Source/WebKit/Shared/API/glib/WebKitUserMessage.cpp:46
> > + * and Web extensions. A WebKitUserMessage always have a name, and it can also include parameters and
> 
> We've never capitalized web before, at least not in web process or web
> extension. I wouldn't start now.

I don't know who corrected me recently changing web to Web somewhere, anyway let's be consistent in our docs, I'll change it.

> Also: have -> has

Oops.

> > Source/WebKit/Shared/API/glib/WebKitUserMessage.cpp:49
> > + * corresponding #WebKitWebPage (and vice versa). One to one messages can be replied directly with
> 
> "replied to"

Ok.

> > Source/WebKit/Shared/API/glib/WebKitUserMessage.cpp:299
> > +    GRefPtr<WebKitUserMessage> adoptedReply = reply;
> 
> Probably a good idea to add your one-line comment about sinking the message
> here, since you did everywhere else, and it's not obvious.

Didn't add it in this case because I thought the adopted prefix in the name was self-explanatory. I'll add the comment in any case, it won't hurt.

> > Source/WebKit/Shared/glib/ArgumentCodersGLib.cpp:46
> > +bool decode(Decoder& decoder, GRefPtr<GVariant>& variant)
> 
> Shouldn't it return Optional<GRefPtr<GVariant>&>, using Optional return
> instead of an out parameter, as you did for the UserMessage class? I think
> this is an old-style decoder that we're supposed to be moving away from,
> when you used the modern style in UserMessage.

Right, I'll use Optional instead.

> > Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp:162
> > +     * replied. Calling webkit_user_message_send_reply() will do nothing.
> 
> replied to.

Ok.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:586
> > +    void sendMedssageToAllExtensions(WebKitUserMessage* message)
> 
> Medssage -> Message

Oops.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:641
> > +static bool readFileDescritpor(int fd, char** contents, gsize* length)
> 
> Descritpor -> Descriptor

You are good catching my typos :-D

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #15) 
> I don't know who corrected me recently changing web to Web somewhere, anyway
> let's be consistent in our docs, I'll change it.

That was Adrian... we had a little argument about that afterwards. I hope I've successfully argued my case for lowercase web. :P

> > > Source/WebKit/Shared/API/glib/WebKitUserMessage.cpp:299
> > > +    GRefPtr<WebKitUserMessage> adoptedReply = reply;
> > 
> > Probably a good idea to add your one-line comment about sinking the message
> > here, since you did everywhere else, and it's not obvious.
> 
> Didn't add it in this case because I thought the adopted prefix in the name
> was self-explanatory. I'll add the comment in any case, it won't hurt.

I didn't notice, but that arguably makes it even worse because people might assume from the variable name that you forgot to adopt the ref.

> You are good catching my typos :-D

I think you use copy/paste more than I do. It's easy to make a typo once, when declaring or calling a function, but it's rare that I make the same typo in both places. So normally the compiler yells when I mess up. But if you use Ctrl+V to paste the function name, typos can go unnoticed.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #16)
> (In reply to Carlos Garcia Campos from comment #15) 
> > I don't know who corrected me recently changing web to Web somewhere, anyway
> > let's be consistent in our docs, I'll change it.
> 
> That was Adrian... we had a little argument about that afterwards. I hope
> I've successfully argued my case for lowercase web :P

It was me, indeed. In the end we agreed that it's okay to use
lowercase, in 2019 The Web™ is not such a special thing anymore
after all :-)

That way we also avoid the ugly looking “GNOME Web Web browser”
expression (“GNOME Web web browser” looks a bit better), and we
don't have to change all the lowercase occurrences that were
already in the code.

[Adrian Perez]

Comment on attachment 380972 [details]
Patch

I had already a good feeling about this patch, and after reading
the MR uses the API in Epiphany I am truly convinced that having
this in WebKit{GTK,WPE} will be beneficial for most (if not all)
applications which are using WebExtensions. Really nice work :D

One last thing, could you please add also a definition to allow
usage of g_autoptr() for WebKitUserMessage in
Source/WebKit/UIProcess/API/{gtk,wpe}/WebKitAutocleanups.h and
Source/WebKit/WebProcess/InjectedBundle/API/{gtk,wpe}/WebKitWebExtensionAutocleanups.h?

With that, and the changes suggested by Michael, I think this will
be ready for landing 👍

[Carlos Garcia Campos]

(In reply to Adrian Perez from comment #18)
> Comment on attachment 380972 [details]
> Patch
> 
> I had already a good feeling about this patch, and after reading
> the MR uses the API in Epiphany I am truly convinced that having
> this in WebKit{GTK,WPE} will be beneficial for most (if not all)
> applications which are using WebExtensions. Really nice work :D
> 
> One last thing, could you please add also a definition to allow
> usage of g_autoptr() for WebKitUserMessage in
> Source/WebKit/UIProcess/API/{gtk,wpe}/WebKitAutocleanups.h and
> Source/WebKit/WebProcess/InjectedBundle/API/{gtk,wpe}/
> WebKitWebExtensionAutocleanups.h?

Sure, I always forget the auto cleanups thing.

> With that, and the changes suggested by Michael, I think this will
> be ready for landing 👍

\o/

[Carlos Garcia Campos]

Created attachment 380987 [details]
Patch

[Adrian Perez]

Comment on attachment 380987 [details]
Patch

Just one minor nit: the autocleanup definitions are still missing from
Source/WebKit/WebProcess/InjectedBundle/API/{gtk,wpe}/WebKitWebExtensionAutocleanups.h
Please add them before landing :)

[Carlos Garcia Campos]

(In reply to Adrian Perez from comment #21)
> Comment on attachment 380987 [details]
> Patch
> 
> Just one minor nit: the autocleanup definitions are still missing from
> Source/WebKit/WebProcess/InjectedBundle/API/{gtk,wpe}/
> WebKitWebExtensionAutocleanups.h
> Please add them before landing :)

Are you sure we have to duplicate them? We don't do that for all other shared objects. Wouldn't we end up with duplicated symbols? it's the same library in the end.

[Carlos Garcia Campos]

Committed r251181: <https://trac.webkit.org/changeset/251181>