Allow process cache to cache processes when using a non-default persistent WebsiteDataStore

[Alex Christensen]

Allow process cache to cache processes when using a non-default persistent WebsiteDataStore

[Alex Christensen]

Created attachment 377675 [details]
Patch

[Chris Dumez]

Comment on attachment 377675 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377675&action=review

> Source/WebKit/UIProcess/WebProcessCache.cpp:69
> +    if (sessionID.isEphemeral() && !process.processPool().hasPagesUsingWebsiteDataStore(process.websiteDataStore())) {

I am not sure we want to change this behavior. It currently matches the logic WebProcessPool::pageBeginUsingWebsiteDataStore() / WebProcessPool::pageEndUsingWebsiteDataStore(). Once there is no longer any page using a page using a non-default session, we remove the session from the network process. Once this is done, why do we want to keep caching WebContent processes for that session?

Your bug title claims to allow caching for WebContent processes for non-default persistent WebsiteDataStores. However, they are already cached as long as there are pages using the WebsiteDataStore in question. What your patch does is allow to cache such processes *after* that session has been removed in memory because there are no longer any pages using it. It is not clear to be we want to do this.

[Alex Christensen]

I think I agree, but there's definitely something strange going on.  Could you try with the PR attached to rdar://problem/47030981 ?  We're also removing sessions from the NetworkProcess when I don't think we should be, exposing https://bugs.webkit.org/show_bug.cgi?id=201324 .  There's still some implicit assumption that the only persistent session is the default session.  I think it's related to a combination of WebProcessPool::pageBeginUsingWebsiteDataStore and the use of nonDefaultDataStores()

[Chris Dumez]

> We're also removing sessions from the NetworkProcess when I don't think we should be.

Do you mean :
1. that you seem non-default persistent sessions getting destroyed even though there are still pages using it?
or
2. that we should keep non-default persistent sessions in memory even when they no longer have no pages, similarly to what we do for the default session

?

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> > We're also removing sessions from the NetworkProcess when I don't think we should be.
> 
> Do you mean :
> 1. that you seem non-default persistent sessions getting destroyed even
> though there are still pages using it?
> or
> 2. that we should keep non-default persistent sessions in memory even when
> they no longer have no pages, similarly to what we do for the default session
> 
> ?

If we made it so that non-default persistent session stay in memory (i.e. update WebProcessPool::pageBeginUsingWebsiteDataStore() / WebProcessPool::pageEndUsingWebsiteDataStore(), so option 2) then I think the change you're suggesting would then make sense. And maybe it would perf in the scenario you're considering.

[Alex Christensen]

Our use of nonDefaultDataStores() isn't related to the perf scenario I'm looking at.  I think option 2 might be what we need.  I'll look into it.

[Alex Christensen]

Created attachment 377724 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r249341

[Radar WebKit Bug Importer]

<rdar://problem/54894208>