Bug 43636 – Add a type-safe IPC mechanism to WebKit2

• Component: WebKit2
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

Consider this code from WebPageProxy::mouseEvent:

    process()->send(WebPageMessage::MouseEvent, m_pageID, CoreIPC::In(event));

The compiler does not enforce that a WebMouseEvent is passed to CoreIPC::In when the message ID is WebPageMessage::MouseEvent. It's easy to make mistakes which will result in crashes on the other end of the connection.

We should make our IPC type-safe.

[Adam Roben (:aroben)]

Anders' current thinking is that messages should be classes whose constructors take arguments of the right types. Each class will have a static const member that is the message ID. So the above code will instead look like:

    process()->send(MouseEvent(event), m_pageID);

[Adam Roben (:aroben)]

We could presumably generate these message classes from IDL files, though that's not a requirement.

[Adam Roben (:aroben)]

<rdar://problem/8282462>

[Adam Roben (:aroben)]

Here's some proposed syntax from Anders, if we generate these classes:

messages WebPage -> WebPageProxy {
    MouseEvent(WebMouseEvent)
    RunJavaScriptInMainFrame(WebCore::String, uint64_t)
}

generates:

namespace Message {
    namespace WebPage {
        enum {
            MouseEventID,
            RunJavaScriptInMainFrameID,
        }

        struct MouseEvent : Arguments1<WebMouseEvent> {
            static const unsigned messageID = Message::WebPage::MouseEventID;
            explicit MouseEvent(const WebMouseEvent& argument1)
                : Arguments1(argument1)
            }
            {
        };

        struct RunJavaScriptInMainFrame : Arguments2<WebCore::String, uint64_t> {
            static const unsigned messageID = Message::WebPage::RunJavaScriptInMainFrameID;
            RunJavaScriptInMainFrame(const WebCore::String& argument1, uint64_t argument2)
                : Arguments2(argument1, argument2)
            {
            }
    }
}

[Adam Roben (:aroben)]

I think the destination and source in the above example are backwards. It should be:

messages WebPageProxy -> WebPage {

[Adam Roben (:aroben)]

Created attachment 63762 [details]
Patch

[Sam Weinig]

Perhaps the MouseEvent constructor should be explicit.

[Anders Carlsson]

(In reply to comment #4)

>         struct MouseEvent : Arguments1<WebMouseEvent> {

This should be Arguments1<const MouseEvent&> I think.

>         struct RunJavaScriptInMainFrame : Arguments2<WebCore::String, uint64_t> {

And this should be Arguments2<const WebCore::String&, uint64_t>

[Adam Roben (:aroben)]

(In reply to comment #7)
> Perhaps the MouseEvent constructor should be explicit.

Definitely.

(In reply to comment #8)
> (In reply to comment #4)
> 
> >         struct MouseEvent : Arguments1<WebMouseEvent> {
> 
> This should be Arguments1<const MouseEvent&> I think.
> 
> >         struct RunJavaScriptInMainFrame : Arguments2<WebCore::String, uint64_t> {
> 
> And this should be Arguments2<const WebCore::String&, uint64_t>

Hm, OK.

[Adam Roben (:aroben)]

(In reply to comment #8)
> (In reply to comment #4)
> 
> >         struct MouseEvent : Arguments1<WebMouseEvent> {
> 
> This should be Arguments1<const MouseEvent&> I think.

Do you mean Arguments1<const WebMouseEvent&>?

[Anders Carlsson]

(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #4)
> > 
> > >         struct MouseEvent : Arguments1<WebMouseEvent> {
> > 
> > This should be Arguments1<const MouseEvent&> I think.
> 
> Do you mean Arguments1<const WebMouseEvent&>?

Yes.

[Adam Roben (:aroben)]

Created attachment 67795 [details]
Super-ugly work-in-progress patch (with autogeneration!)

Here's another ugly work-in-progress patch. WebPage.messages.in (ugly!) declares all the messages that WebPage receives. generate-messages-header.py generates WebPageMessages.h and generate-message-handler.py generates WebPageMessageHandler.cpp. You have to invoke the scripts manually for now, and I've placed the generated source files in the source tree for convenience. But you can get an idea of where I'm headed.

Tips for cleaning up the Python would be much appreciated!

[Adam Roben (:aroben)]

Anders suggested adding a type blacklist to prevent certain types (e.g., enums, unsigned long, etc.) from being used as message parameters.

[Adam Roben (:aroben)]

(In reply to comment #13)
> Anders suggested adding a type blacklist to prevent certain types (e.g., enums, unsigned long, etc.) from being used as message parameters.

I'm going to handle this separately.

[Anders Carlsson]

(In reply to comment #14)
> (In reply to comment #13)
> > Anders suggested adding a type blacklist to prevent certain types (e.g., enums, unsigned long, etc.) from being used as message parameters.
> 
> I'm going to handle this separately.

Maybe we should use a whitelist instead, unless your blacklist can handle things like Vector<unsigned long>

[Adam Roben (:aroben)]

Created attachment 68420 [details]
Autogenerate WebPage's message-receiving code and message types

[WebKit Review Bot]

Attachment 68420 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Platform/CoreIPC/HandleMessage.h:6:  More than one command on the same line  [whitespace/newline] [4]
WebKit2/Platform/CoreIPC/HandleMessage.h:7:  More than one command on the same line  [whitespace/newline] [4]
WebKit2/Scripts/webkit2/messages.py:52:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKit2/Scripts/webkit2/messages_unittest.py:127:  expected 2 blank lines, found 1  [pep8/E302] [5]
Total errors found: 4 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 68420 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4071101

[Adam Roben (:aroben)]

Committed r68079: <http://trac.webkit.org/changeset/68079>

[Csaba Osztrogonác]

(In reply to comment #19)
> Committed r68079: <http://trac.webkit.org/changeset/68079>

Andras fixed the Qt build: http://trac.webkit.org/changeset/68097
Thx bbandix, you rock. :)