Bug 110978

*** Bug 109032 has been marked as a duplicate of this bug. ***

Created attachment 190537 [details] Patch

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 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 on attachment 190537 [details] Patch Attachment 190537 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/16804057

Created attachment 190734 [details] Patch

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

Created attachment 190736 [details] Patch

Created attachment 190737 [details] Patch Yet another buildfix

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 on attachment 190737 [details] Patch Attachment 190737 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/16779705

Created attachment 190746 [details] Patch

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 on attachment 190746 [details] Patch Attachment 190746 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/16842246

Created attachment 190948 [details] Patch

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 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 on attachment 190948 [details] Patch s/Aquire/Acquire

(In reply to comment #16) > Any chance to get reviewed by an owner? Anders needs to review this.

I'd rather we did this once we have move semantics for IPC objects.

(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.

(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.

Created attachment 191959 [details] Patch

(In reply to comment #23) > Created an attachment (id=191959) [details] > Patch Removed the "port" terminology from the common interfaces.

Could someone please review this patch?

(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.

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.

Anders, can you comment on what should be fixed in this patch?

Any progress?

Created attachment 206130 [details] Patch

(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.

(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 on attachment 206130 [details] Patch You should solve the issue with FD_CLOEXEC first.

(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.

(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.

(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.

(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.

Created attachment 207421 [details] Patch

(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.

ping for review?

Created attachment 212706 [details] Patch

WK2 owners, could you review this patch please? ( There weren't any comments, reviews, objections in the last 2 months. :( )

(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 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.

Created attachment 212980 [details] Patch Updated after r156508.

(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 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.

(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.

Created attachment 213361 [details] Patch removed localization change, removed Qt related change, resolved trivial conflicts

(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?

Add more WK2 owner, maybe somebody have a time to review it.

ping for review, please

WK2 owners, could you review this patch please?

ping?

ping?

WK2 owners, ping for review please

ping

ping

(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.

(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?

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.

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.

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. )

Created attachment 217598 [details] Patch updated to ToT

Comment on attachment 217598 [details] Patch Attachment 217598 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/32608047

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 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.

Created attachment 217605 [details] Patch with speculative Mac buildfix after r158890

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 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 on attachment 217605 [details] Patch Attachment 217605 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/31988183

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.

Created attachment 230168 [details] updated patch

(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.

Created attachment 239403 [details] Patch

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 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 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.

Created attachment 240920 [details] Patch

(In reply to comment #79) > Created attachment 240920 [details] > Patch Updated, based on comment #77 and comment #78.

(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 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 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.

(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.