Created attachment 201291 [details] Test case. When stressing the WebProcess creation/destruction, WebKitGTK can often run into socket issues like bad file descriptor errors or polling a socket indefinitely. Currently WebKitGTK's has three places where a socket can be closed. - childFinishedFunction (in ProcessLauncherGtk.cpp) - Connection::platformInvalidate (in ConnectionUnix.cpp) - WorkQueue EventSource destruction (in WorkQueueGtk.cpp) After leaving only the latter, the test case works fine although there are potential issues like when the WebProcess crashes early, and the event sources aren't registered properly.
Created attachment 201292 [details] Tentative patch. Patch leaving the EventSource in charge of closing the socket.
Comment on attachment 201292 [details] Tentative patch. View in context: https://bugs.webkit.org/attachment.cgi?id=201292&action=review I think this patch is correct. It needs a ChangeLog entry, though. > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:136 > +// WorkQueueGtk.cpp EventSource is in charge of closing the socket. I would explain it a bit more here, because it's not obvious. The socket event source is going to be removed below, which will cancel the source. when the source is cancelled, the GSocket is destroyed and the socket is closed. > Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:-115 > - // Monitor the child process, it calls waitpid to prevent the child process from becomming a zombie, > - // and it allows us to close the socket when the child process crashes. > - g_child_watch_add(m_processIdentifier, childFinishedFunction, GINT_TO_POINTER(sockets[1])); > - Right, we added this when we used DGRAM sockets, ti shouldn't bee needed with STREAM sockets, since we are already notified when the other end closes the connection.
(In reply to comment #2) > (From update of attachment 201292 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=201292&action=review > > I think this patch is correct. It needs a ChangeLog entry, though. > > > I would explain it a bit more here, because it's not obvious. The socket event source is going to be removed below, which will cancel the source. when the source is cancelled, the GSocket is destroyed and the socket is closed. > I'm going to update the patch with the comments and ChangeLog entries. Should I add the test (using WK2 API) to the patch? The current version (50 iterations...) takes a while to run on WebKitGTK+ (several seconds) and Nix (3s), though. About the problem when the WebProcess crashes before the connection is setup, it'll setup the register handlers and trigger connectionDidClose when trying to read.
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 201292 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=201292&action=review > > > > I think this patch is correct. It needs a ChangeLog entry, though. > > > > > > I would explain it a bit more here, because it's not obvious. The socket event source is going to be removed below, which will cancel the source. when the source is cancelled, the GSocket is destroyed and the socket is closed. > > > > I'm going to update the patch with the comments and ChangeLog entries. Thanks! > Should I add the test (using WK2 API) to the patch? The current version (50 iterations...) takes a while to run on WebKitGTK+ (several seconds) and Nix (3s), though. Yes, I think it's too much for a unit test, and the problem is very specific to the GTK+ implementation. There are already tests to check the process respawn so I don't think it's needed. > About the problem when the WebProcess crashes before the connection is setup, it'll setup the register handlers and trigger connectionDidClose when trying to read. Exactly.
Created attachment 203037 [details] Updated patch including ChangeLog entry Updated patch to add a ChangeLog entry so that it can be marked for review, since I would like to include this patch in the next release planned for this week.
Comment on attachment 203037 [details] Updated patch including ChangeLog entry Attachment 203037 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/666089 New failing tests: editing/pasteboard/4641033.html
Created attachment 203050 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Comment on attachment 203037 [details] Updated patch including ChangeLog entry View in context: https://bugs.webkit.org/attachment.cgi?id=203037&action=review Looks good to me. This just need an owner to sign off on it. > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:140 > + // In GTK+ platform the socket is closed by the work queue. > +#if !PLATFORM(GTK) > if (m_socketDescriptor != -1) > while (close(m_socketDescriptor) == -1 && errno == EINTR) { } > +#endif Ideally everyone would close the socket in the same way. I think we should put a TODO here for other ports and open bugs.
Comment on attachment 203037 [details] Updated patch including ChangeLog entry View in context: https://bugs.webkit.org/attachment.cgi?id=203037&action=review >> Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:140 >> +#endif > > Ideally everyone would close the socket in the same way. I think we should put a TODO here for other ports and open bugs. I agree, having the work queue take ownership of the file descriptor is necessary to avoid race conditions.
(In reply to comment #9) > (From update of attachment 203037 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=203037&action=review > > >> Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:140 > >> +#endif > > > > Ideally everyone would close the socket in the same way. I think we should put a TODO here for other ports and open bugs. > > I agree, having the work queue take ownership of the file descriptor is necessary to avoid race conditions. Thank you both for the review, opened bugs for EFL and Qt: https://bugs.webkit.org/show_bug.cgi?id=116872 https://bugs.webkit.org/show_bug.cgi?id=116873
Comment on attachment 203037 [details] Updated patch including ChangeLog entry Clearing flags on attachment: 203037 Committed r150808: <http://trac.webkit.org/changeset/150808>
All reviewed patches have been landed. Closing bug.