Bug 201970 – Introduce LegacyGlobalSettings for settings the NetworkProcess needs from a WebProcessPool

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Introduce LegacyGlobalSettings for settings the NetworkProcess needs from a WebProcessPool

[Alex Christensen]

Created attachment 379101 [details]
Patch

[Alex Christensen]

Created attachment 379108 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Alex Christensen]

Note to glib API maintainers: webkit_web_context_set_cache_model and webkit_web_context_get_cache_model should be deprecated and replaced by a new value on WebsiteDataStore that is used if it is set and falls back to the legacy global value.  We have found the existing users of the C API only have one context so this is not a regression, and I imagine the same is true of the glib API users.  This is a necessary change moving forward.

[Alex Christensen]

Created attachment 379109 [details]
Patch

[Alex Christensen]

Note: the WebsiteDataStore replacement should probably be more like a way to disable the memory cache and a way to disable the disk cache than something as strange as a 3-value "cache model"

[Carlos Garcia Campos]

But the cache model is not only used to enable/disable caches, it also adjusts the capacity of the caches.

[Alex Christensen]

I guess that’s true. We might still want a cache model based api for simplicity, but it will definitely need to be on the website data store where the cache is.

[Geoffrey Garen]

Comment on attachment 379109 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        I'm starting by moving the cache model to this new abstraction.
> +        We were using it in tests to disable the page cache, which should be done with a boolean on the pool configuration, not by changing the cache model.

Longer-term, I think page caching should be a setting on the website data store.

> Source/WebKit/ChangeLog:10
> +        We were also using it in WKContextSetCacheModel which has several clients that won't change quickly, so this abstraction is used to maintain existing behavior.

Does this patch maintain existing behavior, or does it create a new behavior where WKContextSetCacheModel is now global instead of per-context? (How do we know that it's OK to be global?)

[Alex Christensen]

(In reply to Geoffrey Garen from comment #9)
> Longer-term, I think page caching should be a setting on the website data
> store.
I think I agree.  That detail doesn't need to be decided for this patch.  It does need to be decided for the replacement for WKContextSetCacheModel.

> Does this patch maintain existing behavior, or does it create a new behavior
> where WKContextSetCacheModel is now global instead of per-context? (How do
> we know that it's OK to be global?)
This patch maintains existing behavior because every client using WKContextSetCacheModel has one WKContextRef that they use in their process for everything.  This patch is necessary because we don't want the NetworkProcess to get settings from any one ProcessPool.

[Alex Christensen]

Right now I'll also have to make hstsStorageDirectory a legacy global setting.  There may be one or two more, but most of the things can move to WebsiteDataStore.

[Carlos Garcia Campos]

(In reply to Alex Christensen from comment #10)
> (In reply to Geoffrey Garen from comment #9)
> > Longer-term, I think page caching should be a setting on the website data
> > store.
> I think I agree.  That detail doesn't need to be decided for this patch.  It
> does need to be decided for the replacement for WKContextSetCacheModel.
> 
> > Does this patch maintain existing behavior, or does it create a new behavior
> > where WKContextSetCacheModel is now global instead of per-context? (How do
> > we know that it's OK to be global?)
> This patch maintains existing behavior because every client using
> WKContextSetCacheModel has one WKContextRef that they use in their process
> for everything.

That's not necessarily true for GLib API users, though.

>  This patch is necessary because we don't want the
> NetworkProcess to get settings from any one ProcessPool.

Isn't there a network process per process pool?

[Alex Christensen]

(In reply to Carlos Garcia Campos from comment #12)
> (In reply to Alex Christensen from comment #10)
> > (In reply to Geoffrey Garen from comment #9)
> > > Longer-term, I think page caching should be a setting on the website data
> > > store.
> > I think I agree.  That detail doesn't need to be decided for this patch.  It
> > does need to be decided for the replacement for WKContextSetCacheModel.
> > 
> > > Does this patch maintain existing behavior, or does it create a new behavior
> > > where WKContextSetCacheModel is now global instead of per-context? (How do
> > > we know that it's OK to be global?)
> > This patch maintains existing behavior because every client using
> > WKContextSetCacheModel has one WKContextRef that they use in their process
> > for everything.
> 
> That's not necessarily true for GLib API users, though.
I think it's probably a good assumption, but you're right, there could be someone with two contexts that intentionally has different cache models.  I think that's unlikely in practice.  In any case, the cache-determining API needs to be moved to WebsiteDataStore, where the cache is.
> 
> >  This patch is necessary because we don't want the
> > NetworkProcess to get settings from any one ProcessPool.
> 
> Isn't there a network process per process pool?
I'm working to make it so there's only one network process.  That's why I'm doing this.

[Geoffrey Garen]

> > This patch maintains existing behavior because every client using
> > WKContextSetCacheModel has one WKContextRef that they use in their process
> > for everything.
> 
> That's not necessarily true for GLib API users, though.

What are the requirements for GLib API compatibility?

What is the best way to know if GLib API users need support for WKContextSetCacheModel on multiple WKContextRefs?

> >  This patch is necessary because we don't want the
> > NetworkProcess to get settings from any one ProcessPool.
> 
> Isn't there a network process per process pool?

The goal of this patch set is to change that behavior and establish a singleton network process per app. The WebsiteDataStore APIs are broken if you launch multiple network processes.

[Geoffrey Garen]

Comment on attachment 379109 [details]
Patch

r=me

If this does create a problem for Glib API users, we should discuss further how to resolve it.

[Alex Christensen]

http://trac.webkit.org/r250148

[Radar WebKit Bug Importer]

<rdar://problem/55570450>

[Carlos Garcia Campos]

(In reply to Alex Christensen from comment #13)
> (In reply to Carlos Garcia Campos from comment #12)
> > (In reply to Alex Christensen from comment #10)
> > > (In reply to Geoffrey Garen from comment #9)
> > > > Longer-term, I think page caching should be a setting on the website data
> > > > store.
> > > I think I agree.  That detail doesn't need to be decided for this patch.  It
> > > does need to be decided for the replacement for WKContextSetCacheModel.
> > > 
> > > > Does this patch maintain existing behavior, or does it create a new behavior
> > > > where WKContextSetCacheModel is now global instead of per-context? (How do
> > > > we know that it's OK to be global?)
> > > This patch maintains existing behavior because every client using
> > > WKContextSetCacheModel has one WKContextRef that they use in their process
> > > for everything.
> > 
> > That's not necessarily true for GLib API users, though.
> I think it's probably a good assumption, but you're right, there could be
> someone with two contexts that intentionally has different cache models.  I
> think that's unlikely in practice.  In any case, the cache-determining API
> needs to be moved to WebsiteDataStore, where the cache is.

Ah, no, that's not what I meant, I meant only using different contexts, I also think it's unlikely that they use a different cache model per context, though.

> > 
> > >  This patch is necessary because we don't want the
> > > NetworkProcess to get settings from any one ProcessPool.
> > 
> > Isn't there a network process per process pool?
> I'm working to make it so there's only one network process.  That's why I'm
> doing this.

Ok, now I understand. I hope we can just deprecate APIs but keeping backwards compatibility.

[Carlos Garcia Campos]

A quick search in Debian shows only a few apps setting the cache model, and they either use a single context or the same model for all contexts.

[Chris Dumez]

Comment on attachment 379109 [details]
Patch

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

> Source/WebKit/UIProcess/API/APIProcessPoolConfiguration.h:74
> +    bool usesPageCache() const { return m_usesPageCache; }

This seems to have introduced a regression. The cacheModel was obeyed by the WebContent process and was setting the PageCache's maxSize in the WebProcess. However, this m_usesPageCache flag only controls the UIProcess (i.e. creation of SuspendedPage proxies). However, it no longer updates the size of the PageCache in the WebProcess it seems.

[Chris Dumez]

(In reply to Chris Dumez from comment #20)
> Comment on attachment 379109 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379109&action=review
> 
> > Source/WebKit/UIProcess/API/APIProcessPoolConfiguration.h:74
> > +    bool usesPageCache() const { return m_usesPageCache; }
> 
> This seems to have introduced a regression. The cacheModel was obeyed by the
> WebContent process and was setting the PageCache's maxSize in the
> WebProcess. However, this m_usesPageCache flag only controls the UIProcess
> (i.e. creation of SuspendedPage proxies). However, it no longer updates the
> size of the PageCache in the WebProcess it seems.

Will likely deal with this at the same time as https://bugs.webkit.org/show_bug.cgi?id=202929.