Bug 201492 – Keeps running obsolete WebProcess-es for too long

• Component: WebKit2
• Status: RESOLVED FIXED

[Milan Crha]

I use webkit-2.26 branch at r249422. Compile the test application from bug #201033 comment #41. When it's run, only a single WebKitWebProcess is running, which makes perfect sense, because there's only single WebKitWebView. From `ps ax | grep WebKitWebProcess` I see:

>  3707 pts/1    SLl+   0:00 PREFIX/libexec/webkit2gtk-4.0/WebKitWebProcess 6 10

Click the 'Reload' button at the top of the test application. It shows "Loading...", then opens the previous content again. The 'ps' says:

>  3707 pts/1    SLl+   0:00 PREFIX/libexec/webkit2gtk-4.0/WebKitWebProcess 6 10
>  3767 pts/1    SLl+   0:00 PREFIX/libexec/webkit2gtk-4.0/WebKitWebProcess 11 15
>  3778 pts/1    SLl+   0:00 PREFIX/libexec/webkit2gtk-4.0/WebKitWebProcess 14 17

Click the 'Reload' button again.

>  3707 pts/1    SLl+   0:00 PREFIX/libexec/webkit2gtk-4.0/WebKitWebProcess 6 10
>  3767 pts/1    SLl+   0:00 PREFIX/libexec/webkit2gtk-4.0/WebKitWebProcess 11 15
>  3778 pts/1    SLl+   0:00 PREFIX/libexec/webkit2gtk-4.0/WebKitWebProcess 14 17
>  3798 pts/1    SLl+   0:00 PREFIX/libexec/webkit2gtk-4.0/WebKitWebProcess 17 19

and again

>  3767 pts/1    SLl+   0:00 PERFIX/libexec/webkit2gtk-4.0/WebKitWebProcess 11 15
>  3778 pts/1    SLl+   0:00 PERFIX/libexec/webkit2gtk-4.0/WebKitWebProcess 14 17
>  3798 pts/1    SLl+   0:00 PREFIX/libexec/webkit2gtk-4.0/WebKitWebProcess 17 19
>  3823 pts/1    SLl+   0:00 PREFIX/libexec/webkit2gtk-4.0/WebKitWebProcess 20 11

It stopped the first process only now. The processes do not seem to be reused.

I believe it's too late. Not only because of the used resources by obsolete WebProcess-es, but also if the page loaded there would do anything nasty, you just give it more time to do it, which can be considered a security issue from certain point of view. I checked and with this test application the WebProcess-es use around 70MB of resident memory, which is not that much, but it's 4 times, which is more that 1/4GB, which can count. And this is a very simple example, imagine that some applications create their own Web Extensions, which can add to the memory requirements, either directly or indirectly (by using DOM API, which caches the objects). I do not know whether you also discard the WebPage content as soon as the process gets obsolete, but I do not think it matters that much, we are still talking about wasted resources and if the page gets control of the process by executing its own code on the machine you'll not stop it doing it by clearing the page content, you'll stop it only by closing the parent process.

Trying the 'ps' again, all the processes from the last check are still there, after 5-10 minutes till I wrote this comment. Clicking the Reload four times (always waiting till the page fully loads) gives me this 'ps' output (all four processes are replaced):

>  3923 pts/1    SLl+   0:00 PREFIX/libexec/webkit2gtk-4.0/WebKitWebProcess 23 11
>  3934 pts/1    SLl+   0:00 PREFIX/libexec/webkit2gtk-4.0/WebKitWebProcess 26 16
>  3945 pts/1    SLl+   0:00 PREFIX/libexec/webkit2gtk-4.0/WebKitWebProcess 29 18
>  3956 pts/1    SLl+   0:00 PREFIX/libexec/webkit2gtk-4.0/WebKitWebProcess 32 10

Thus I'd prefer if you close the obsolete WebProcess-es as soon as they are obsolete.

[Carlos Garcia Campos]

This is not GTK specific. I think this is the web process cache. It's only used when there are more than 3GB of RAM available, and caches 4 process per GB of RAM available up to 30. Every process is cached for 30 minutes. Is this correct, Chris? There's a setting to disable it, but I don't know if that setting is also temporary and expected to be removed at some point. Otherwise we could expose it in our API.

[Milan Crha]

Why would I, as a user of the library, need to know anything about these "caches", when the processes are not reused even in such a simple test application which provides all the data on its own?

[Carlos Garcia Campos]

(In reply to Milan Crha from comment #2)
> Why would I, as a user of the library, need to know anything about these
> "caches", when the processes are not reused even in such a simple test
> application which provides all the data on its own?

Only apps know if these caches are useful for them. Is evolution setting a cache model? Maybe it would be better to automatically disable the web process cache when cache model is not WEBKIT_CACHE_MODEL_WEB_BROWSER.

[Milan Crha]

Evolution uses WEBKIT_CACHE_MODEL_DOCUMENT_VIEWER.

[Milan Crha]

One problem I face with this process cache I found right now: The WebExtension evolution defines has sever D-Bus methods which expect output, like:

    <method name='GetSelectionContentText'>
      <arg type='t' name='page_id' direction='in'/>
      <arg type='s' name='text_content' direction='out'/>
    </method>

Which basically calls GetSelectionContentText() with a page_id and expects a text of the selection as a return value. Being there multiple WebProcess-es having the same page_id, while only one being the one with currently viewed content in the WebView, how do I distinguish which it is, when the processes are just running in the background, with my Web Extension loaded and connected to my D-Bus connection?

[Chris Dumez]

The WebProcess cache is not enabled by default. It only gets enabled the first time the client process-swaps (i.e. navigates cross-site with PSON enabled). Therefore, this indicates that Evolution is process-swapping and that the cache is useful to it.

[Michael Catanzaro]

(In reply to Milan Crha from comment #5)
> Being there multiple WebProcess-es
> having the same page_id

That should never happen. Instead the page_id should change depending on which web process is in use.

If you want to avoid the process swaps, changing URLs from :// to :/// like we suggested earlier should *probably* suffice.

[Milan Crha]

(In reply to Chris Dumez from comment #6)
> Therefore, this indicates that Evolution is process-swapping and
> that the cache is useful to it.

It's questionable whether it's useful. It's not from my point of view. As said above, the test-wk2.c only switches between two pages, but the processes are changing anyway, thus the cache doesn't work for it (might be due to some other constraint).

(In reply to Michael Catanzaro from comment #7)
> That should never happen. Instead the page_id should change depending on
> which web process is in use.

I'll surely check that out with the webkit-2.26 branch to see how it works.

> If you want to avoid the process swaps, changing URLs from :// to :/// like
> we suggested earlier should *probably* suffice.

I'd like not to add workarounds for current behaviour. It led to the disaster with 2.26, where evo expects certain behaviour, which had been changed now. Thus I'd prefer to provide universal solution (if it's doable at all, aka in some extent universal).

[Chris Dumez]

(In reply to Carlos Garcia Campos from comment #3)
> (In reply to Milan Crha from comment #2)
> > Why would I, as a user of the library, need to know anything about these
> > "caches", when the processes are not reused even in such a simple test
> > application which provides all the data on its own?
> 
> Only apps know if these caches are useful for them. Is evolution setting a
> cache model? Maybe it would be better to automatically disable the web
> process cache when cache model is not WEBKIT_CACHE_MODEL_WEB_BROWSER.

I am fine with this idea.

[Milan Crha]

(In reply to Chris Dumez from comment #9)
> I am fine with this idea.

It works for me too, Evolution does that already :)

[Carlos Garcia Campos]

(In reply to Milan Crha from comment #8)
> (In reply to Chris Dumez from comment #6)
> > Therefore, this indicates that Evolution is process-swapping and
> > that the cache is useful to it.
> 
> It's questionable whether it's useful. It's not from my point of view. As
> said above, the test-wk2.c only switches between two pages, but the
> processes are changing anyway, thus the cache doesn't work for it (might be
> due to some other constraint).
> 
> (In reply to Michael Catanzaro from comment #7)
> > That should never happen. Instead the page_id should change depending on
> > which web process is in use.
> 
> I'll surely check that out with the webkit-2.26 branch to see how it works.

Note that I'm going to disable PSON in 2.26, so you only need to care about this in trunk. Page id returned by the web view will never change in 2.26.

> > If you want to avoid the process swaps, changing URLs from :// to :/// like
> > we suggested earlier should *probably* suffice.
> 
> I'd like not to add workarounds for current behaviour. It led to the
> disaster with 2.26, where evo expects certain behaviour, which had been
> changed now. Thus I'd prefer to provide universal solution (if it's doable
> at all, aka in some extent universal).

[Carlos Garcia Campos]

(In reply to Milan Crha from comment #5)
> One problem I face with this process cache I found right now: The
> WebExtension evolution defines has sever D-Bus methods which expect output,
> like:
> 
>     <method name='GetSelectionContentText'>
>       <arg type='t' name='page_id' direction='in'/>
>       <arg type='s' name='text_content' direction='out'/>
>     </method>
> 
> Which basically calls GetSelectionContentText() with a page_id and expects a
> text of the selection as a return value. Being there multiple WebProcess-es
> having the same page_id, while only one being the one with currently viewed
> content in the WebView, how do I distinguish which it is, when the processes
> are just running in the background, with my Web Extension loaded and
> connected to my D-Bus connection?

We should review all the usage of injected bundle and dom bindings in evo for the next release too. Because GetSelectionContentText could probably be implemented a lot easier (and more efficient) using webkit_web_view_run_javascript(). You don't need any DBus call, not to worry about page IDs and processes.

[Carlos Garcia Campos]

Created attachment 378185 [details]
Patch

[Michael Catanzaro]

Comment on attachment 378185 [details]
Patch

LGTM

[Milan Crha]

Possibly (for trunk) related bug #201540.

(In reply to Carlos Garcia Campos from comment #12)
> We should review all the usage of injected bundle and dom bindings in evo
> for the next release too.

Right, I plan to start looking on this. There are a lot of D-Bus functions which may not be needed once javascript is used. The only problem is asynchronicity, because evolution requires certain things immediately, not "once the other side decides to respond". Not talking that managing "spaghetti" code with all of its complexity leads to errors.

[Carlos Garcia Campos]

(In reply to Milan Crha from comment #15)
> Possibly (for trunk) related bug #201540.
> 
> (In reply to Carlos Garcia Campos from comment #12)
> > We should review all the usage of injected bundle and dom bindings in evo
> > for the next release too.
> 
> Right, I plan to start looking on this. There are a lot of D-Bus functions
> which may not be needed once javascript is used. The only problem is
> asynchronicity, because evolution requires certain things immediately, not
> "once the other side decides to respond".

Are you using DBus sync messages? You shouldn't be doing that in any case.

> Not talking that managing
> "spaghetti" code with all of its complexity leads to errors.

In most of the cases the js code required is quite simple.

[Milan Crha]

(In reply to Carlos Garcia Campos from comment #16)
> Are you using DBus sync messages? You shouldn't be doing that in any case.

There are some hacks in evo to make things "easier".

> In most of the cases the js code required is quite simple.

I meant things like 3-4 async calls in a row to finally get to the state where you can do things which the user requested by clicking a button in the GUI. Something I'll try to simplify for sure.

By the way, do you have any good web site with Javascript code learning? It's a very long time sine I touched Javascript the last time...

[Carlos Garcia Campos]

Chris, could you review this, please?

[Chris Dumez]

Comment on attachment 378185 [details]
Patch

r=me

[Carlos Garcia Campos]

(In reply to Chris Dumez from comment #19)
> Comment on attachment 378185 [details]
> Patch
> 
> r=me

Thanks!

[Carlos Garcia Campos]

Committed r249948: <https://trac.webkit.org/changeset/249948>

[Radar WebKit Bug Importer]

<rdar://problem/55431008>