Bug 43636 - Add a type-safe IPC mechanism to WebKit2
Summary: Add a type-safe IPC mechanism to WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 46297
  Show dependency treegraph
 
Reported: 2010-08-06 12:52 PDT by Adam Roben (:aroben)
Modified: 2010-09-22 22:21 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.96 KB, patch)
2010-08-06 14:38 PDT, Adam Roben (:aroben)
no flags Details | Formatted Diff | Diff
Super-ugly work-in-progress patch (with autogeneration!) (52.96 KB, patch)
2010-09-16 07:29 PDT, Adam Roben (:aroben)
no flags Details | Formatted Diff | Diff
Autogenerate WebPage's message-receiving code and message types (92.11 KB, patch)
2010-09-22 12:47 PDT, Adam Roben (:aroben)
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2010-08-06 12:52:23 PDT
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.
Comment 1 Adam Roben (:aroben) 2010-08-06 12:54:25 PDT
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);
Comment 2 Adam Roben (:aroben) 2010-08-06 12:55:58 PDT
We could presumably generate these message classes from IDL files, though that's not a requirement.
Comment 3 Adam Roben (:aroben) 2010-08-06 12:56:55 PDT
<rdar://problem/8282462>
Comment 4 Adam Roben (:aroben) 2010-08-06 13:55:50 PDT
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)
            {
            }
    }
}
Comment 5 Adam Roben (:aroben) 2010-08-06 14:05:24 PDT
I think the destination and source in the above example are backwards. It should be:

messages WebPageProxy -> WebPage {
Comment 6 Adam Roben (:aroben) 2010-08-06 14:38:18 PDT
Created attachment 63762 [details]
Patch
Comment 7 Sam Weinig 2010-08-06 21:30:22 PDT
Perhaps the MouseEvent constructor should be explicit.
Comment 8 Anders Carlsson 2010-08-07 14:58:23 PDT
(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>
Comment 9 Adam Roben (:aroben) 2010-08-16 07:13:33 PDT
(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.
Comment 10 Adam Roben (:aroben) 2010-09-15 08:42:56 PDT
(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&>?
Comment 11 Anders Carlsson 2010-09-15 08:45:51 PDT
(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.
Comment 12 Adam Roben (:aroben) 2010-09-16 07:29:23 PDT
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!
Comment 13 Adam Roben (:aroben) 2010-09-16 15:06:04 PDT
Anders suggested adding a type blacklist to prevent certain types (e.g., enums, unsigned long, etc.) from being used as message parameters.
Comment 14 Adam Roben (:aroben) 2010-09-22 12:24:05 PDT
(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.
Comment 15 Anders Carlsson 2010-09-22 12:36:43 PDT
(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>
Comment 16 Adam Roben (:aroben) 2010-09-22 12:47:01 PDT
Created attachment 68420 [details]
Autogenerate WebPage's message-receiving code and message types
Comment 17 WebKit Review Bot 2010-09-22 12:48:52 PDT
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.
Comment 18 Early Warning System Bot 2010-09-22 12:56:20 PDT
Attachment 68420 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4071101
Comment 19 Adam Roben (:aroben) 2010-09-22 13:50:57 PDT
Committed r68079: <http://trac.webkit.org/changeset/68079>
Comment 20 Csaba Osztrogonác 2010-09-22 22:21:48 PDT
(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. :)