|Summary:||[GTK] WebProcess leaked when closing pages with network process enabled|
|Product:||WebKit||Reporter:||Carlos Garcia Campos <cgarcia>|
|Severity:||Normal||CC:||andersca, csaavedra, gns, svillar|
|Version:||528+ (Nightly build)|
Description Carlos Garcia Campos 2014-03-04 08:25:30 PST
Comment 1 Carlos Garcia Campos 2014-03-04 08:36:02 PST
This happens sometimes when closing the last page of a web process: 1.- WebPageProxy::close() sends the Close message to the WebProcess 2.- WebProcessProxy::removeWebPage() checks that it's the last page and that the process can be terminated and closes the connection (this only happens when using the network process) 3.- The WebProcess receives the Close message and enables the process termination 4.- The WebProcess io thread notices the connection has been closed and invalidates it 5.- Connection::connectionDidClose() calls platformInvalidate and schedules dispatchConnectionDidClose on the main loop 6.- WebProcess::shouldTerminate() tries to send the sync message ShouldTerminate to the UI process At this point the connection is still valid (it has a client because dispatchConnectionDidClose is scheduled but hasn't been handled yet) but the connection has been invalidated in plaformInvalidate. So we fail to send the message, of course, but the connection keeps waiting for the reply because it still has a client. This only happens sometimes because in other cases where the dispatchConnectionDidClose is run before the sendSync, the connection ends up in didFailToSendSyncMessage() and the processes exists.
Comment 2 Carlos Garcia Campos 2014-03-04 09:14:33 PST
Created attachment 225784 [details] Patch I think this patch fixes the race condition, by checking if the connection is still open before trying to send the sync message
Comment 3 Carlos Garcia Campos 2014-03-05 06:29:18 PST
I've noticed that is not enough. I think the connection can be closed between Connection::sendMessage() that schedules sendOutgoingMessages() and when sendOutgoingMessages() is called. In that case we actually check is connected in canSendOutgoingMessages() and we don't try to send the message, but Connection::sendSyncMessage has already called waitForSyncReply and keeps waiting for the reply of a sync message that hasn't been sent.
Comment 4 Carlos Garcia Campos 2014-03-05 08:30:58 PST
Maybe the only problem is that we are using the default timeout (std::chrono::milliseconds::max()) maybe we could use a 1 second timeout for the shouldTerminate message?
Comment 5 Carlos Garcia Campos 2014-03-06 02:18:36 PST
Created attachment 225973 [details] Use a 1 second timeout for ShouldTerminate message
Comment 6 Carlos Garcia Campos 2014-05-26 05:27:09 PDT
I've finally found the problem here. It's not actually a race condition in the end, the problem is that the web process is not notified when the UI process closes the connection. It happens because the UI process uses close() on the socket fd, which only shuts down the connection when the socket is not shared by another process. We are already closing the sockets in ProcessLauncherGtk right after the spawn and in the child process callback, but that's not enough in multiprocess mode. We need to use set the cloexec flag to make sure the socket is not exposed to other future children.