WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
115964
When possible, terminate web processes immediately when closing their last page.
https://bugs.webkit.org/show_bug.cgi?id=115964
Summary
When possible, terminate web processes immediately when closing their last page.
Andreas Kling
Reported
2013-05-11 11:12:40 PDT
For <
rdar://problem/10103795
>.
Attachments
Patch
(3.80 KB, patch)
2013-05-11 11:13 PDT
,
Andreas Kling
ggaren
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Andreas Kling
Comment 1
2013-05-11 11:13:55 PDT
Created
attachment 201465
[details]
Patch
Geoffrey Garen
Comment 2
2013-05-11 11:37:45 PDT
Comment on
attachment 201465
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=201465&action=review
r=me!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
> Source/WebKit2/UIProcess/WebProcessProxy.cpp:204 > + // Terminate the web process immediately if we have enough information to confidently do so. > + // This only works if we're using a network process. Otherwise we have to wait for the web process to clear caches.
I don't understand what "clear caches" means, but I guess that's OK.
Andreas Kling
Comment 3
2013-05-11 11:52:37 PDT
Committed
r149934
: <
http://trac.webkit.org/changeset/149934
>
Andreas Kling
Comment 4
2013-05-11 11:56:40 PDT
(In reply to
comment #2
)
> (From update of
attachment 201465
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=201465&action=review
> > r=me!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
Thanks!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
> > Source/WebKit2/UIProcess/WebProcessProxy.cpp:204 > > + // Terminate the web process immediately if we have enough information to confidently do so. > > + // This only works if we're using a network process. Otherwise we have to wait for the web process to clear caches. > > I don't understand what "clear caches" means, but I guess that's OK.
I changed that to "clean up." When doing networking in the web process, we have to wait for it to run [[NSURLCache sharedURLCache] removeAllCachedResponses] before exiting. (See WebProcess::platformTerminate())
Darin Adler
Comment 5
2013-05-11 16:08:08 PDT
Comment on
attachment 201465
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=201465&action=review
>>> Source/WebKit2/UIProcess/WebProcessProxy.cpp:204 >>> + // This only works if we're using a network process. Otherwise we have to wait for the web process to clear caches. >> >> I don't understand what "clear caches" means, but I guess that's OK. > > I changed that to "clean up." When doing networking in the web process, we have to wait for it to run [[NSURLCache sharedURLCache] removeAllCachedResponses] before exiting. (See WebProcess::platformTerminate())
I think the key phrase would probably be “caches on disk” or “in the file system” or some phrase like that.
> Source/WebKit2/UIProcess/WebProcessProxy.h:181 > + bool canTerminateChildProcess();
Should it be const?
Andreas Kling
Comment 6
2013-05-11 16:09:39 PDT
(In reply to
comment #5
)
> (From update of
attachment 201465
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=201465&action=review
> > > Source/WebKit2/UIProcess/WebProcessProxy.h:181 > > + bool canTerminateChildProcess(); > > Should it be const?
I wanted it to be, but that made the patch a bit huge. I'll fix separately.
Alexey Proskuryakov
Comment 7
2013-05-11 16:52:22 PDT
I'm surprised that this is a good idea. Even not considering other persistent state that may be inconsistent, a web page at least needs to send a ping request from unload.
Andreas Kling
Comment 8
2013-05-11 18:05:31 PDT
(In reply to
comment #7
)
> I'm surprised that this is a good idea. Even not considering other persistent state that may be inconsistent, a web page at least needs to send a ping request from unload.
How about pushing information about whether a web process has registered unload event handlers to the UI process, and then disabling this short-circuit in that case? What other persistent state do you have in mind? Do you think it would be better to start a watchdog timer when sending WebPage::Close and deferring this mechanism until that fires? (and canceling it if the child process terminates.)
Andreas Kling
Comment 9
2013-05-12 16:22:00 PDT
Web processes with unload event handlers registered will now prevent the UI process from killing them (
bug 115988
.)
Alexey Proskuryakov
Comment 10
2013-05-12 18:07:51 PDT
Unload (and beforeunload) are necessary to run for JS applications to cleanly shut down themselves. WebProcess itself may also be in the middle of writing to a file, and terminating it without a chance to finish that is not nice. This includes Keychain, Web SQL database, and possibly more.
> For <
rdar://problem/10103795
>.
Perhaps the right way forward is to chat about this change on IRC. There are already mechanisms that are supposed to prevent this kind of behavior, and it's unclear to me from this bug why they were found inadequate. I didn't look into this very deeply, but I was under the impression that we needed a bugfix, not a redesign.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug