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
Attachments
Patch (3.80 KB, patch)
2013-05-11 11:13 PDT, Andreas Kling
ggaren: review+
Andreas Kling
Comment 1 2013-05-11 11:13:55 PDT
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
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.