Bug 115880 – [GTK] Connection issues in repeated WebProcess crash/reloads.

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Lauro Moura Maranhao Neto]

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.

[Lauro Moura Maranhao Neto]

Created attachment 201292 [details]
Tentative patch.

Patch leaving the EventSource in charge of closing the socket.

[Carlos Garcia Campos]

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.

[Lauro Moura Maranhao Neto]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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.

[Build Bot]

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

[Build Bot]

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

[Martin Robinson]

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.

[Anders Carlsson]

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.

[Carlos Garcia Campos]

(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

[WebKit Commit Bot]

Comment on attachment 203037 [details]
Updated patch including ChangeLog entry

Clearing flags on attachment: 203037

Committed r150808: <http://trac.webkit.org/changeset/150808>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.