Bug 41690 - [Qt] Close the WebProcess
Summary: [Qt] Close the WebProcess
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-06 06:02 PDT by Balazs Kelemen
Modified: 2010-08-03 22:08 PDT (History)
5 users (show)

See Also:


Attachments
proposed patch (5.84 KB, patch)
2010-07-06 06:13 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
proposed patch (6.45 KB, patch)
2010-07-06 10:45 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
proposed patch - this time created on the trunk (6.50 KB, patch)
2010-07-08 01:15 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
proposed patch (updated) (2.69 KB, patch)
2010-07-26 09:46 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
proposed patch (1.73 KB, patch)
2010-07-27 04:16 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2010-07-06 06:02:30 PDT
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.
Comment 1 Balazs Kelemen 2010-07-06 06:13:04 PDT
Created attachment 60624 [details]
proposed patch

This change contains the process closing mechanism and some final cleanup logic.
Comment 2 Anders Carlsson 2010-07-06 09:21:51 PDT
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.
Comment 3 Kenneth Rohde Christiansen 2010-07-06 09:26:55 PDT
regarding destroySharedMemory(), since we are using mmap, maybe unmapMemory would be a better name.
Comment 4 Balazs Kelemen 2010-07-06 10:45:48 PDT
Created attachment 60640 [details]
proposed patch

Ranamed destroySharedMemory to cleanup, create the temporal files in the temp dir. The changes was suggested by kenneth.
Comment 5 WebKit Commit Bot 2010-07-06 12:23:53 PDT
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
Comment 6 Eric Seidel (no email) 2010-07-07 03:15:57 PDT
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.
Comment 7 Balazs Kelemen 2010-07-08 01:15:24 PDT
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.
Comment 8 Eric Seidel (no email) 2010-07-09 03:16:35 PDT
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.
Comment 9 Balazs Kelemen 2010-07-26 09:46:25 PDT
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.
Comment 10 Anders Carlsson 2010-07-26 10:02:36 PDT
Comment on attachment 62582 [details]
proposed patch (updated)

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

On which thread is this called?
Comment 11 Balazs Kelemen 2010-07-26 11:03:29 PDT
> 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.
Comment 12 Anders Carlsson 2010-07-26 12:41:26 PDT
(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?
Comment 13 Balazs Kelemen 2010-07-27 02:08:25 PDT
> 
> 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.)
Comment 14 Balazs Kelemen 2010-07-27 02:23:57 PDT
Oh, I have just realized that connectionDidClose and dispatchConnectionDidClose fit my needs!
Comment 15 Balazs Kelemen 2010-07-27 04:16:40 PDT
Created attachment 62678 [details]
proposed patch
Comment 16 WebKit Commit Bot 2010-08-03 22:08:11 PDT
Comment on attachment 62678 [details]
proposed patch

Clearing flags on attachment: 62678

Committed r64628: <http://trac.webkit.org/changeset/64628>
Comment 17 WebKit Commit Bot 2010-08-03 22:08:16 PDT
All reviewed patches have been landed.  Closing bug.