Bug 110978 - [WK2] Refactoring: unify IPC connection creation - get rid of ifdef hell
Summary: [WK2] Refactoring: unify IPC connection creation - get rid of ifdef hell
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Balazs Kelemen
URL:
Keywords:
: 109032 (view as bug list)
Depends on:
Blocks: 108832 111310 110093 111543
  Show dependency treegraph
 
Reported: 2013-02-27 07:10 PST by Balazs Kelemen
Modified: 2016-03-09 13:45 PST (History)
41 users (show)

See Also:


Attachments
Patch (41.69 KB, patch)
2013-02-27 08:57 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (42.62 KB, patch)
2013-02-28 09:02 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (42.66 KB, patch)
2013-02-28 09:13 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (42.67 KB, patch)
2013-02-28 09:22 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (42.73 KB, patch)
2013-02-28 10:14 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (42.76 KB, patch)
2013-03-01 05:51 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (42.87 KB, patch)
2013-03-07 02:39 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (41.76 KB, patch)
2013-07-05 02:52 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (41.75 KB, patch)
2013-07-24 16:59 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (38.68 KB, patch)
2013-09-26 07:47 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (36.59 KB, patch)
2013-09-30 04:48 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (33.24 KB, patch)
2013-10-04 07:03 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (33.18 KB, patch)
2013-11-21 12:34 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (33.17 KB, patch)
2013-11-21 13:16 PST, Csaba Osztrogonác
buildbot: commit-queue-
Details | Formatted Diff | Diff
updated patch (34.05 KB, patch)
2014-04-25 04:29 PDT, Peter Molnar
no flags Details | Formatted Diff | Diff
updated patch (34.13 KB, patch)
2014-04-25 05:02 PDT, Peter Molnar
no flags Details | Formatted Diff | Diff
Patch (38.13 KB, patch)
2014-10-07 03:59 PDT, Éva Balázsfalvi
no flags Details | Formatted Diff | Diff
Patch (37.79 KB, patch)
2014-11-04 04:17 PST, Éva Balázsfalvi
andersca: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2013-02-27 07:10:16 PST
This has been raised in https://bugs.webkit.org/show_bug.cgi?id=110093#c6
Comment 1 Balazs Kelemen 2013-02-27 07:10:49 PST
*** Bug 109032 has been marked as a duplicate of this bug. ***
Comment 2 Balazs Kelemen 2013-02-27 08:57:32 PST
Created attachment 190537 [details]
Patch
Comment 3 EFL EWS Bot 2013-02-27 09:09:58 PST
Comment on attachment 190537 [details]
Patch

Attachment 190537 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/16845001
Comment 4 Build Bot 2013-02-27 09:25:52 PST
Comment on attachment 190537 [details]
Patch

Attachment 190537 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16826071
Comment 5 Build Bot 2013-02-27 10:04:30 PST
Comment on attachment 190537 [details]
Patch

Attachment 190537 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16804057
Comment 6 Balazs Kelemen 2013-02-28 09:02:33 PST
Created attachment 190734 [details]
Patch
Comment 7 Build Bot 2013-02-28 09:09:09 PST
Comment on attachment 190734 [details]
Patch

Attachment 190734 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16787132
Comment 8 Balazs Kelemen 2013-02-28 09:13:30 PST
Created attachment 190736 [details]
Patch
Comment 9 Balazs Kelemen 2013-02-28 09:22:06 PST
Created attachment 190737 [details]
Patch

Yet another buildfix
Comment 10 Build Bot 2013-02-28 09:46:40 PST
Comment on attachment 190737 [details]
Patch

Attachment 190737 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16803283
Comment 11 Build Bot 2013-02-28 10:06:08 PST
Comment on attachment 190737 [details]
Patch

Attachment 190737 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16779705
Comment 12 Balazs Kelemen 2013-02-28 10:14:53 PST
Created attachment 190746 [details]
Patch
Comment 13 Build Bot 2013-02-28 10:25:55 PST
Comment on attachment 190746 [details]
Patch

Attachment 190746 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16798153
Comment 14 Build Bot 2013-02-28 10:27:53 PST
Comment on attachment 190746 [details]
Patch

Attachment 190746 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16842246
Comment 15 Balazs Kelemen 2013-03-01 05:51:20 PST
Created attachment 190948 [details]
Patch
Comment 16 Balazs Kelemen 2013-03-06 00:55:29 PST
Any chance to get reviewed by an owner? Actually, I would need somebody showing some love for making the network process cross-platform and nursing my patches blocking bug 108832.
Comment 17 Thiago Marcos P. Santos 2013-03-06 01:12:32 PST
Comment on attachment 190948 [details]
Patch

The patch looks good to me, but I would like to suggest renaming all the references of "port" to "identifier".
Comment 18 Thiago Marcos P. Santos 2013-03-06 01:50:09 PST
Comment on attachment 190948 [details]
Patch

s/Aquire/Acquire
Comment 19 Brady Eidson 2013-03-06 11:19:05 PST
(In reply to comment #16)
> Any chance to get reviewed by an owner? 

Anders needs to review this.
Comment 20 Anders Carlsson 2013-03-06 11:27:07 PST
I'd rather we did this once we have move semantics for IPC objects.
Comment 21 Kenneth Rohde Christiansen 2013-03-06 11:28:35 PST
(In reply to comment #20)
> I'd rather we did this once we have move semantics for IPC objects.

Any ETA on that? Could you shortly explain what you are planning. Thiago Santos expressed some interest in helping out with CoreIPC.
Comment 22 Balazs Kelemen 2013-03-07 02:37:23 PST
(In reply to comment #17)
> (From update of attachment 190948 [details])
> The patch looks good to me, but I would like to suggest renaming all the references of "port" to "identifier".

Makes sense.
Comment 23 Balazs Kelemen 2013-03-07 02:39:43 PST
Created attachment 191959 [details]
Patch
Comment 24 Balazs Kelemen 2013-03-07 04:10:52 PST
(In reply to comment #23)
> Created an attachment (id=191959) [details]
> Patch

Removed the "port" terminology from the common interfaces.
Comment 25 Thiago Marcos P. Santos 2013-03-22 06:42:50 PDT
Could someone please review this patch?
Comment 26 Brady Eidson 2013-03-22 08:45:45 PDT
(In reply to comment #25)
> Could someone please review this patch?

I believe Anders already expressed that we should wait on this.  Perhaps he can give more details.
Comment 27 Balazs Kelemen 2013-03-22 08:56:00 PDT
He mentioned we should have move semantics for this, but I wonder how should we use it. I guess he think about these parts:
+    enum ConnectionRequirement {
+        AcquireSendRight,
+        ForwardSendRight
+    };
+
+    Attachment(Connection::Identifier, ConnectionRequirement);

and where it is used, for example

+    Attachment clientPort(descriptor.clientIdentifier(), Attachment::AcquireSendRight);
     parentProcessConnection()->send(Messages::PluginProcessProxy::DidCreateWebProcessConnection(clientPort, m_supportsAsynchronousPluginInitialization), 0);
or
+    reply->send(CoreIPC::Attachment::nullConnection(CoreIPC::Attachment::ForwardSendRight));

However, since there are 2 different ways we want to "move" an Attachment, I don't think move constructors really help here.
But I'm just guessing this, probably he was thinking about something different.
In any case we can refactor the code later, so I don't really understand why does it have to block the patch.
Comment 28 Lamarque V. Souza 2013-06-04 14:59:38 PDT
Anders, can you comment on what should be fixed in this patch?
Comment 29 Kwang Yul Seo 2013-06-28 01:10:52 PDT
Any progress?
Comment 30 Kwang Yul Seo 2013-07-05 02:52:47 PDT
Created attachment 206130 [details]
Patch
Comment 31 Kwang Yul Seo 2013-07-05 03:06:02 PDT
(In reply to comment #30)
> Created an attachment (id=206130) [details]
> Patch

I rebased the original patch on top of the HEAD.

In the mean time, I found a bug in the original patch.

Connection::ConnectionDescriptor Connection::createPlatformConnection()
{
    int sockets[2];
    RELEASE_ASSERT(socketpair(AF_UNIX, SOCKET_TYPE, 0, sockets) != -1);

    // Avoid exposing the server socket to the child process or the client socket to any future child.
    for (unsigned i = 0; i < 2; ++i) {
        while (fcntl(sockets[i], F_SETFD, FD_CLOEXEC) == -1)
            RELEASE_ASSERT(errno == EINTR);
    }

    return ConnectionDescriptor(sockets[0], sockets[1]);
}

"or the client socket to any future child." applies only to NetworkProcess::createNetworkConnectionToWebProcess and SharedWorkerProcess::createNetworkConnectionToWebProcess. ProcessLauncher should not set FD_CLOEXEC on the client socket. 

I changed the method to set FD_CLOEXEC only on the server socket. But now I am not sure how to handle NetworkProcess::createNetworkConnectionToWebProcess and SharedWorkerProcess::createNetworkConnectionToWebProcess. 

I will investigate this issue more.
Comment 32 Balazs Kelemen 2013-07-05 06:32:18 PDT
(In reply to comment #31)
> (In reply to comment #30)
> > Created an attachment (id=206130) [details] [details]
> > Patch
> 
> I rebased the original patch on top of the HEAD.
> 
> In the mean time, I found a bug in the original patch.
> 
> Connection::ConnectionDescriptor Connection::createPlatformConnection()
> {
>     int sockets[2];
>     RELEASE_ASSERT(socketpair(AF_UNIX, SOCKET_TYPE, 0, sockets) != -1);
> 
>     // Avoid exposing the server socket to the child process or the client socket to any future child.
>     for (unsigned i = 0; i < 2; ++i) {
>         while (fcntl(sockets[i], F_SETFD, FD_CLOEXEC) == -1)
>             RELEASE_ASSERT(errno == EINTR);
>     }
> 
>     return ConnectionDescriptor(sockets[0], sockets[1]);
> }
> 
> "or the client socket to any future child." applies only to NetworkProcess::createNetworkConnectionToWebProcess and SharedWorkerProcess::createNetworkConnectionToWebProcess. ProcessLauncher should not set FD_CLOEXEC on the client socket. 
> 
> I changed the method to set FD_CLOEXEC only on the server socket. But now I am not sure how to handle NetworkProcess::createNetworkConnectionToWebProcess and SharedWorkerProcess::createNetworkConnectionToWebProcess. 
> 
> I will investigate this issue more.

You are right. It's good to set FD_CLOEXEC on the server socket (the one the creator of the connection use) but only after the connection with the child has been set up. I am not sure how it was not breaking things for me (with my local branch where I pushed all my network process patches together), maybe I just did not test this version of the patch.
You don't need to do any magic, just explicitly express the difference of these situations. I would add functions that calls fcntl on the client and server sockets. These will be noops on Mac. But maybe you can get up with a nicer approach.
Comment 33 Balazs Kelemen 2013-07-05 06:34:45 PDT
Comment on attachment 206130 [details]
Patch

You should solve the issue with FD_CLOEXEC first.
Comment 34 Kwang Yul Seo 2013-07-05 20:15:12 PDT
(In reply to comment #32)
> You are right. It's good to set FD_CLOEXEC on the server socket (the one the creator of the connection use) but only after the connection with the child has been set up. I am not sure how it was not breaking things for me (with my local branch where I pushed all my network process patches together), maybe I just did not test this version of the patch.

Why "only after the connection with the child has been set up"? We should set FD_CLOEXEC on the server socket (the one the creator of the connection use) immediately after creating a socketpair because we want to prevent future child processes from accessing the server socket. This is the current behavior and I didn't change it.

Setting FD_CLOEXEC on the client socket seems to have a different requirement. Here's my understanding.

PluginProcess passes its client socket to the UIProcess because UIProcess relays the socket to the requesting WebProcess. This means the UIProcess also gets the client socket and the future descendants processes of the UIProess implicitly inherits all these client sockets. To prevent this problem, PluginProcess sets the FD_CLOEXEC on the client socket before sending it to the UIProcess. 

(The Mac port prevents this by sending the port to the WebProcess with MACH_MSG_TYPE_MOVE_SEND option though it seems more strict.)

Bug this is redundant because we already set FD_CLOEXEC on every file descriptor we pass over UNIX domain sockets in readBytesFromSocket. That means when the client socket is passed to the UIProcess, we set FD_CLOEXEC on the socket. So the future child processes of the UIProcess can't access to this socket.

static ssize_t readBytesFromSocket(int socketDescriptor, uint8_t* buffer, int count, int* fileDescriptors, size_t* fileDescriptorsCount)
{
...
        bool found = false;
        struct cmsghdr* controlMessage;
        for (controlMessage = CMSG_FIRSTHDR(&message); controlMessage; controlMessage = CMSG_NXTHDR(&message, controlMessage)) {
            if (controlMessage->cmsg_level == SOL_SOCKET && controlMessage->cmsg_type == SCM_RIGHTS) {
                *fileDescriptorsCount = (controlMessage->cmsg_len - CMSG_LEN(0)) / sizeof(int);
                memcpy(fileDescriptors, CMSG_DATA(controlMessage), sizeof(int) * *fileDescriptorsCount);

                for (size_t i = 0; i < *fileDescriptorsCount; ++i) {
                    while (fcntl(fileDescriptors[i], F_SETFL, FD_CLOEXEC) == -1) {
                        if (errno != EINTR) {
                            ASSERT_NOT_REACHED();
                            break;
                        }   
                    }   
                }   

                found = true;
                break;
            }   
        }   

So I think that this patch has no behavior change. But I am not an expert on UNIX domain sockets. So please correct me if I'm wrong or misunderstood something.
Comment 35 Kwang Yul Seo 2013-07-05 23:37:23 PDT
(In reply to comment #34)
> (The Mac port prevents this by sending the port to the WebProcess with MACH_MSG_TYPE_MOVE_SEND option though it seems more strict.)

Correction: Mach ports are not inherited. So there is no such problem in the Mac port.

This prevents UIProcess from possessing the port used for communication between PluginProcess and WebProcess. We don't enforce this on Unix domain sockets yet.
Comment 36 Balazs Kelemen 2013-07-07 10:25:57 PDT
(In reply to comment #34)
> (In reply to comment #32)
> > You are right. It's good to set FD_CLOEXEC on the server socket (the one the creator of the connection use) but only after the connection with the child has been set up. I am not sure how it was not breaking things for me (with my local branch where I pushed all my network process patches together), maybe I just did not test this version of the patch.
> 
> Why "only after the connection with the child has been set up"? We should set FD_CLOEXEC on the server socket (the one the creator of the connection use) immediately after creating a socketpair because we want to prevent future child processes from accessing the server socket. This is the current behavior and I didn't change it.

You was mentioning that ProcessLauncher should not set FD_CLOEXEC on the client socket, I was reacting on this. I think this is true because want the child (which we are launching) to inherit the socket. What I was saying is that we can still set the flag after the child has been started (it has finished calling exec).

The situation is different when we are creating the socket at the time when both processes already exist (like at NetworkProcess::createNetworkConnectionToWebProcess). In this case we can set the FD_CLOEXEC flag immediately.
Comment 37 Kwang Yul Seo 2013-07-08 02:53:22 PDT
(In reply to comment #36)
> You was mentioning that ProcessLauncher should not set FD_CLOEXEC on the client socket, I was reacting on this. I think this is true because want the child (which we are launching) to inherit the socket. What I was saying is that we can still set the flag after the child has been started (it has finished calling exec).

Yeah, you and I speak the same language.
 
> The situation is different when we are creating the socket at the time when both processes already exist (like at NetworkProcess::createNetworkConnectionToWebProcess). In this case we can set the FD_CLOEXEC flag immediately.

Yes, I agree. But the comment in SharedWorkerProcess::createWebProcessConnection is misleading.

// Don't expose the shared worker process socket to possible future web processes.
while (fcntl(sockets[0], F_SETFD, FD_CLOEXEC) == -1) {
...
}

This code sets the FD_CLOEXEC flag on the client socket immediately to prevent future child processes of SharedWorkerProcess from inheriting the client socket. But the comment says that "Don't expose the shared worker process socket to possible future web processes". 

As I mentioned in Comment #34, ConnectionUnix sets the FD_CLOEXEC flag on every file descriptor passed over unix domain sockets. So shared worker process sockets are never exposed to possible future web processes anyway without this code.

So if our intention is actually what the comment says, then I don't think we need to set the FD_CLOEXEC flag on the client socket here because SharedWorkerProcess never forks off web processes.
Comment 38 Kwang Yul Seo 2013-07-24 16:59:45 PDT
Created attachment 207421 [details]
Patch
Comment 39 Kwang Yul Seo 2013-07-24 17:03:01 PDT
(In reply to comment #38)
> Created an attachment (id=207421) [details]
> Patch

Rebased to the HEAD.

Anders, would you review the patch? This patch has been in dust for a few months hoping for love.
Comment 40 Csaba Osztrogonác 2013-08-26 05:03:20 PDT
ping for review?
Comment 41 Csaba Osztrogonác 2013-09-26 07:47:19 PDT
Created attachment 212706 [details]
Patch
Comment 42 Csaba Osztrogonác 2013-09-26 07:50:31 PDT
WK2 owners, could you review this patch please? 
( There weren't any comments, reviews, objections in the last 2 months. :( )
Comment 43 Csaba Osztrogonác 2013-09-26 07:51:44 PDT
(In reply to comment #41)
> Created an attachment (id=212706) [details]
> Patch

I updated the previous patch to ToT:
 - Removed the SharedWorkerProcess change, because it doesn't exist at all after r156277.
 - Resolved a trivial conflict in ProcessLauncherEfl.cpp after r156063.
Comment 44 Darin Adler 2013-09-26 09:34:46 PDT
Comment on attachment 212706 [details]
Patch

I don’t know when he’ll have time, but I think Anders i the best person to review this.
Comment 45 Csaba Osztrogonác 2013-09-30 04:48:41 PDT
Created attachment 212980 [details]
Patch

Updated after r156508.
Comment 46 Csaba Osztrogonác 2013-09-30 04:51:16 PDT
(In reply to comment #44)
> (From update of attachment 212706 [details])
> I don’t know when he’ll have time, but I think Anders i the best person to review this.

(In reply to comment #45)
> Created an attachment (id=212980) [details]
> Patch
> 
> Updated after r156508.

Anders, could you review this patch please?
Comment 47 Carlos Garcia Campos 2013-10-01 00:28:43 PDT
Comment on attachment 212980 [details]
Patch

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

> Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:57
> +    // Make child process inherit parent's locale.
> +    g_setenv("LC_ALL", setlocale(LC_ALL, 0), TRUE);

Why are you bringing this back? this was removed in r145081, see bug #111398.
Comment 48 Csaba Osztrogonác 2013-10-01 02:34:36 PDT
(In reply to comment #47)
> (From update of attachment 212980 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212980&action=review
> 
> > Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:57
> > +    // Make child process inherit parent's locale.
> > +    g_setenv("LC_ALL", setlocale(LC_ALL, 0), TRUE);
> 
> Why are you bringing this back? this was removed in r145081, see bug #111398.

I think accidentally, because the original patch in 
this bug is older than bug111398. I'll remove it.
Comment 49 Csaba Osztrogonác 2013-10-04 07:03:28 PDT
Created attachment 213361 [details]
Patch

removed localization change, removed Qt related change, resolved trivial conflicts
Comment 50 Csaba Osztrogonác 2013-10-10 06:21:26 PDT
(In reply to comment #49)
> Created an attachment (id=213361) [details]
> Patch
> 
> removed localization change, removed Qt related change, resolved trivial conflicts


Anders, could you review this patch please?
Comment 51 Csaba Osztrogonác 2013-10-10 07:27:41 PDT
Add more WK2 owner, maybe somebody have a time to review it.
Comment 52 Csaba Osztrogonác 2013-10-11 04:03:21 PDT
ping for review, please
Comment 53 Csaba Osztrogonác 2013-10-15 03:27:21 PDT
WK2 owners, could you review this patch please?
Comment 54 Csaba Osztrogonác 2013-10-16 23:17:44 PDT
ping?
Comment 55 Csaba Osztrogonác 2013-10-18 06:48:40 PDT
ping?
Comment 56 Csaba Osztrogonác 2013-10-21 02:14:57 PDT
WK2 owners, ping for review please
Comment 57 Csaba Osztrogonác 2013-10-21 22:59:22 PDT
ping
Comment 58 Csaba Osztrogonác 2013-10-22 23:22:52 PDT
ping
Comment 59 Anders Carlsson 2013-10-23 11:09:54 PDT
(In reply to comment #58)
> ping

Stop!

Please! There are 39 people cced on this bug who will get an e-mail every time. 

I understand that you want this patch to be reviewed, but with every ping you're pissing more and more people of which will be less and less willing to do the review. This patch is on my review list but I have more important things to attend to right now.
Comment 60 Csaba Osztrogonác 2013-10-23 13:51:29 PDT
(In reply to comment #59)
> (In reply to comment #58)
> > ping
> Stop!

> Please! There are 39 people cced on this bug who will get an e-mail every time. 

If somebody isn't interested / doesn't have expertise in this topic, feel
free to remove himself/herself from the cc list. I won't be angry, I promise
;)

> I understand that you want this patch to be reviewed, but with every ping you're pissing more and more people of which will be less and less willing to do the review. This patch is on my review list but I have more important things to attend to right now.

Of course, I can understand that you have more important things. But there
are 18 WK2 owner, you aren't only one. The origial patch was prepared 9
months before, the lateset patch is 19 days old. And we didn't get _any_
review from _any_ WK2 owner. It is very very disappointing. :(((( 

It would be great if you could give us an ETA for the review time. How much
should we wait, and how many times should we update the patch to ToT again
and again during waiting for review? Is there any chance if you will let other
platforms to enable NetworkProcess building in the near future (in the
following month, year, decade). It is so exhausting to wait, update, wait,
update, wait, update, ask for review again and again. And then getting an r-
because of a minor naming issue and then update and wait again for months ...

It is not about only this bug ... We had to wait months for other bugs too. :(
For example we waited 3 months for https://bugs.webkit.org/show_bug.cgi?id=118520 (which could have been a oneliner, but a WK2 owner
asked this big refactoring ...) and then you rolled it out without any
notification and comment about what was the problem and didn't give any 
hint how can we fix it.

So is there any chance that WK2 owners will be a little bit 
more responsive in the near future, please?
Comment 61 Brady Eidson 2013-10-23 14:31:14 PDT
Just to point out what may not be obvious:  While WK2 patches need a WK2 owner to review, that's not *all* they need.

There's *way* fewer CoreIPC experts than there are WK2 owners, and this patch needs a close eye from such an expert.
Comment 62 Balazs Kelemen 2013-10-23 18:10:31 PDT
This patch is a simple refactoring, no deep CoreIPC knowledge needed to understand it and decide whether it makes sense. It does not change any behavior or internal mechanisms.
Comment 63 Csaba Osztrogonác 2013-10-24 06:40:27 PDT
In my opinion it isn't correct that you asked this refactoring in
https://bugs.webkit.org/show_bug.cgi?id=110093#c6 and then Balázs
did it in few days, but you hadn't any time in this ~8 months to
review the patch you asked.

If you don't have any time for reviewing refactoring you explicitly asked,
please revise your original opinion and let the much more simpler patch to
the trunk - https://bugs.webkit.org/attachment.cgi?id=205781 .

( I reopened https://bugs.webkit.org/show_bug.cgi?id=110093 and asked review again. )
Comment 64 Csaba Osztrogonác 2013-11-21 12:34:54 PST
Created attachment 217598 [details]
Patch

updated to ToT
Comment 65 Build Bot 2013-11-21 12:50:13 PST
Comment on attachment 217598 [details]
Patch

Attachment 217598 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/32608047
Comment 66 Build Bot 2013-11-21 13:04:20 PST
Comment on attachment 217598 [details]
Patch

Attachment 217598 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/32528052
Comment 67 Csaba Osztrogonác 2013-11-21 13:06:23 PST
Comment on attachment 217598 [details]
Patch

It seems it needs a fix after https://trac.webkit.org/changeset/158890

I'm going to fix it soon.
Comment 68 Csaba Osztrogonác 2013-11-21 13:16:42 PST
Created attachment 217605 [details]
Patch

with speculative Mac buildfix after r158890
Comment 69 Darin Adler 2013-11-21 13:56:01 PST
Comment on attachment 217605 [details]
Patch

I think this should go in as smaller patches. There’s no need for it to all be done at once. In particular, it seems that refactoring the Mac to use this new cross-platform API could be done separately from moving over other platforms. Each of those two patches would be considerably easier to review.
Comment 70 Build Bot 2013-11-21 14:03:50 PST
Comment on attachment 217605 [details]
Patch

Attachment 217605 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/32948005
Comment 71 Build Bot 2013-11-21 14:32:48 PST
Comment on attachment 217605 [details]
Patch

Attachment 217605 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/31988183
Comment 72 Peter Molnar 2014-04-25 04:29:12 PDT
Created attachment 230165 [details]
updated patch

Updated the patch to the current ToT.

I think is makes much more sense to submit this patch as a whole, rather than splitting it into multiple parts.
Comment 73 Peter Molnar 2014-04-25 05:02:12 PDT
Created attachment 230168 [details]
updated patch
Comment 74 Csaba Osztrogonác 2014-05-16 03:28:33 PDT
(In reply to comment #73)
> Created an attachment (id=230168) [details]
> updated patch

Anders, could you possibly review this patch in the near future?
The original patch is more than a year old and was updated to ToT
many many times, because none of the WK2 owner had enough time /
were interested in reviewing it. :-(

I still think that WebKit2 would profit much if we could get rid this
copy/paste ifdef hell once instead of adding more copy/paste code to
DatabaseProcess, <new-fancy-feature>Process again and again.
Comment 75 Éva Balázsfalvi 2014-10-07 03:59:52 PDT
Created attachment 239403 [details]
Patch
Comment 76 Balazs Kelemen 2014-10-07 08:25:46 PDT
Comment on attachment 239403 [details]
Patch

I hope there exists a potential reviewer for this. If it doesn't go this time I don't think we should continue this story.

Some comments.

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

> Source/WebKit2/Platform/IPC/Connection.h:131
>      struct SocketPair {

Do we still need this struct?

> Source/WebKit2/Platform/IPC/Connection.h:165
> +    static ConnectionDescriptor createPlatformConnection(unsigned options = SetCloexecOnClient | SetCloexecOnServer);

Can we use WebKitish names like SetCloseOnExec? Maybe an enum class would be nice also :)

> Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp:-53
> -#define SOCKET_TYPE SOCK_DGRAM

I wonder if it's actually correct. Maybe QNX wants to use DGRAM? Or maybe we don't care (in which case you should not introduce the check for OS(QNX))? I think it would be better to leave this alone for now, as this patch supposed to be only a refactor.
Comment 77 Csaba Osztrogonác 2014-11-04 02:43:47 PST
Comment on attachment 239403 [details]
Patch

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

>> Source/WebKit2/Platform/IPC/Connection.h:131
>>      struct SocketPair {
> 
> Do we still need this struct?

Good point, it became unused, we can safely remove it.

>> Source/WebKit2/Platform/IPC/Connection.h:165
>> +    static ConnectionDescriptor createPlatformConnection(unsigned options = SetCloexecOnClient | SetCloexecOnServer);
> 
> Can we use WebKitish names like SetCloseOnExec? Maybe an enum class would be nice also :)

It can be reasonable. But I prefer doing it in a separated patch.
Comment 78 Csaba Osztrogonác 2014-11-04 03:07:49 PST
Comment on attachment 239403 [details]
Patch

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

>> Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp:-53
>> -#define SOCKET_TYPE SOCK_DGRAM
> 
> I wonder if it's actually correct. Maybe QNX wants to use DGRAM? Or maybe we don't care (in which case you should not introduce the check for OS(QNX))? I think it would be better to leave this alone for now, as this patch supposed to be only a refactor.

It seems QNX is Qt cruft, we shouldn't modify this code path at all here.
Comment 79 Éva Balázsfalvi 2014-11-04 04:17:06 PST
Created attachment 240920 [details]
Patch
Comment 80 Éva Balázsfalvi 2014-11-04 05:35:47 PST
(In reply to comment #79)
> Created attachment 240920 [details]
> Patch

Updated, based on comment #77 and comment #78.
Comment 81 Csaba Osztrogonác 2014-11-04 05:47:37 PST
(In reply to comment #80)
> (In reply to comment #79)
> > Created attachment 240920 [details]
> > Patch
> 
> Updated, based on comment #77 and comment #78.

Dear WebKit2 owners, 

is there any chance to get review for this patch?

You explicitly asked this before letting Linux ports use NetworkProcess - 
https://bugs.webkit.org/show_bug.cgi?id=110093#c6 . But Linux ports can't
wait _20 months_ for review and added more ifdefs a year before:
http://trac.webkit.org/changeset/160308

I still think we should remove these ifdef hell, but it is impossible
if WK2 owners don't want to review. (nor negative / nor positive review)

Please review it in the near future. Thanks.
Comment 82 Darin Adler 2014-11-04 14:10:52 PST
Comment on attachment 240920 [details]
Patch

I can’t answer for the other WebKit2 owners, but for myself the problem is that this patch is just a little to big and broad for me to review. Is there some intermediate step we could take that would be a smaller patch, or is this the smallest first step?

For example, if I was doing this I might do it one platform at a time and then remove the #if as the platforms become identical.
Comment 83 Anders Carlsson 2016-03-09 13:25:39 PST
Comment on attachment 240920 [details]
Patch

We have a different, more ambitious refactoring of the IPC layer planned. I'll make sure to send an e-mail to webkit-dev once this is closer to a reality.
Comment 84 Csaba Osztrogonác 2016-03-09 13:45:26 PST
(In reply to comment #83)
> Comment on attachment 240920 [details]
> Patch
> 
> We have a different, more ambitious refactoring of the IPC layer planned.
> I'll make sure to send an e-mail to webkit-dev once this is closer to a
> reality.

Wow, a real review after 3 years waiting. :)

In this case let's close it as wontfix.