When possible, terminate web processes immediately when closing their last page.

• Component: WebKit2
• Status: RESOLVED FIXED

[Andreas Kling]

For <rdar://problem/10103795>.

[Andreas Kling]

Created attachment 201465 [details]
Patch

[Geoffrey Garen]

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]

Committed r149934: <http://trac.webkit.org/changeset/149934>

[Andreas Kling]

(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 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]

(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]

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]

(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]

Web processes with unload event handlers registered will now prevent the UI process from killing them (bug 115988.)

[Alexey Proskuryakov]

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.