Bug 41690 – [Qt] Close the WebProcess

• Component: WebKit2
• Status: RESOLVED FIXED

[Balazs Kelemen]

The WebProcess should close itself when the client disconnected. We need to do this because the close message can not be delivered to the WebProcess when the client exited - at least I discovered it in Qt. In regard of this change the termination mechanism should be changed, for example wait for the WebProcess to exit instead of immediately killing it. I would implement it in a follow up patch.

[Balazs Kelemen]

Created attachment 60624 [details]
proposed patch

This change contains the process closing mechanism and some final cleanup logic.

[Anders Carlsson]

Comment on attachment 60624 [details]
proposed patch

Looks goood.

In the future I plan to have a special didClose handler that is executed directly on the work queue. This way we can still exit even if the web process is spinning.

[Kenneth Rohde Christiansen]

regarding destroySharedMemory(), since we are using mmap, maybe unmapMemory would be a better name.

[Balazs Kelemen]

Created attachment 60640 [details]
proposed patch

Ranamed destroySharedMemory to cleanup, create the temporal files in the temp dir. The changes was suggested by kenneth.

[WebKit Commit Bot]

Comment on attachment 60640 [details]
proposed patch

Rejecting patch 60640 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Kenneth Rohde Christiansen', u'--force']" exit_code: 1
Last 500 characters of output:
pp
patching file WebKit2/Shared/qt/UpdateChunk.h
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: WebKit2/UIProcess/Launcher/qt/WebProcessLauncherQt.cpp
|index 5e1e110..306eea3 100644
|--- WebKit2/UIProcess/Launcher/qt/WebProcessLauncherQt.cpp
|+++ WebKit2/UIProcess/Launcher/qt/WebProcessLauncherQt.cpp
--------------------------
No file to patch.  Skipping patch.
3 out of 3 hunks ignored

Full output: http://webkit-commit-queue.appspot.com/results/3381344

[Eric Seidel (no email)]

Comment on attachment 60624 [details]
proposed patch

Cleared Anders Carlsson's review+ from obsolete attachment 60624 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Balazs Kelemen]

Created attachment 60848 [details]
proposed patch - this time created on the trunk

The last one was created in the rebase-after-upstream branch, that's why it did not apply on trunk.

[Eric Seidel (no email)]

Comment on attachment 60640 [details]
proposed patch

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 60640 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Balazs Kelemen]

Created attachment 62582 [details]
proposed patch (updated)

This time I kept only those changes that are related to the bug title - handling of the disconnection.
I will push the rest of the previous patch into a new bug.

[Anders Carlsson]

Comment on attachment 62582 [details]
proposed patch (updated)

>  
> +void Connection::disconnectHandler()
> +{
> +    m_client->didClose(this);
> +}
> +

On which thread is this called?

[Balazs Kelemen]

> On which thread is this called?

Oops, on the WorkQueue's thread. It is strange that the patch worked for me (at least it did not crashed). I will fix it in the next patch.

[Anders Carlsson]

(In reply to comment #11)
> > On which thread is this called?
> 
> Oops, on the WorkQueue's thread. It is strange that the patch worked for me (at least it did not crashed). I will fix it in the next patch.

We actually quit the run loop when the last web page has been closed. Is there a reason why the web process doesn't exit when that happens?

[Balazs Kelemen]

> 
> We actually quit the run loop when the last web page has been closed. Is there a reason why the web process doesn't exit when that happens?

Huh, seems like I get confused while working on this. There was various problems with our implementation of Connection but it seems like these have been fixed. So, to be able to exit from the web process when it gets the close message, we should just implement RunLoop::exit. The other parts of the patch assures that the web process would exit even when the client did not tear down correctly (killed or crashed). Don't you think this is something we should implement? Actually, I would introduce a common method of Connection what would look like this:
void didDisconnect()
{
    m_client->didClose();
}
Maybe didClose would be a better name. This way all port can schedule to call this on the main run loop when the connection became broken.
Do you like the idea? (I mean the whole concept not just the new function.)

[Balazs Kelemen]

Oh, I have just realized that connectionDidClose and dispatchConnectionDidClose fit my needs!

[Balazs Kelemen]

Created attachment 62678 [details]
proposed patch

[WebKit Commit Bot]

Comment on attachment 62678 [details]
proposed patch

Clearing flags on attachment: 62678

Committed r64628: <http://trac.webkit.org/changeset/64628>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.