Bug 169456 – Allow termination of background WebProcesses that go over a given CPU usage threshold

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Allow termination background WebProcesses that goes over a given CPU usage threshold.

[Radar WebKit Bug Importer]

<rdar://problem/30960968>

[Chris Dumez]

Created attachment 304080 [details]
Patch

[Chris Dumez]

Created attachment 304084 [details]
Patch

[Chris Dumez]

Created attachment 304096 [details]
Patch

[Andreas Kling]

Comment on attachment 304096 [details]
Patch

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

> Source/WebKit2/ChangeLog:20
> +        Once a WebProcess has been terminated, we do not the client know until one of its

do not -> do not let

> Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:382
> +    // Use the largest limit among all pages in this process.

Is this level of granularity really useful? When would we want to have different CPU limits for two different non-visible pages in the same process?

[Chris Dumez]

(In reply to comment #5)
> Comment on attachment 304096 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304096&action=review
> 
> > Source/WebKit2/ChangeLog:20
> > +        Once a WebProcess has been terminated, we do not the client know until one of its
> 
> do not -> do not let
> 
> > Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:382
> > +    // Use the largest limit among all pages in this process.
> 
> Is this level of granularity really useful? When would we want to have
> different CPU limits for two different non-visible pages in the same process?

Responded offline.

[Chris Dumez]

Created attachment 304263 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 304263 [details]
Patch

Clearing flags on attachment: 304263

Committed r213857: <http://trac.webkit.org/changeset/213857>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Created attachment 304283 [details]
Test page

[Geoffrey Garen]

Comment on attachment 304263 [details]
Patch

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

> Source/WebKit2/ChangeLog:18
> +        If such limit is set, whenever a WebContent process has no visible pages, we start
> +        monitoring its CPU usage over 15 minutes periods. At the end of each period, we
> +        check if the process' average CPU usage over this period was greater than the
> +        background CPU limit. If it greater, the WebContent process send an IPC message to
> +        the UIProcess letting it know that it exceeded the CPU limit. The UI process will
> +        then log a message and terminate the process unless it has any audio playing.

It seems like this design won't be able to solve a very common cause of background CPU usage: an infinite loop. If the main thread is busy looping, we'll never send this message.

> Source/WebKit2/ChangeLog:24
> +        Once a WebProcess has been terminated, we do not let the client know until one of its
> +        pages becomes visible again. When this happens, we call the processDidCrash
> +        delegate and Safari will take care of reloading the tab and showing the crash
> +        banner then. This is done because we do not want to reload content that is
> +        using a lot of CPU while in the background.

This is pretty weird. We're just not telling the truth about when the process crashed.

Why is this behavior specific to the CPU limit, whereas the memory limit notifies of crash right away?

Why does WebKit make the reload decision for Safari in this upside-down way, instead of allowing Safari to make the policy decision?

[Chris Dumez]

(In reply to comment #11)
> Comment on attachment 304263 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304263&action=review
> 
> > Source/WebKit2/ChangeLog:18
> > +        If such limit is set, whenever a WebContent process has no visible pages, we start
> > +        monitoring its CPU usage over 15 minutes periods. At the end of each period, we
> > +        check if the process' average CPU usage over this period was greater than the
> > +        background CPU limit. If it greater, the WebContent process send an IPC message to
> > +        the UIProcess letting it know that it exceeded the CPU limit. The UI process will
> > +        then log a message and terminate the process unless it has any audio playing.
> 
> It seems like this design won't be able to solve a very common cause of
> background CPU usage: an infinite loop. If the main thread is busy looping,
> we'll never send this message.

This case was already taken care of in <rdar://problem/30558745>.
We call the processDidBecomeUnresponsive in this case and Safari terminates it if it is a background process.

> 
> > Source/WebKit2/ChangeLog:24
> > +        Once a WebProcess has been terminated, we do not let the client know until one of its
> > +        pages becomes visible again. When this happens, we call the processDidCrash
> > +        delegate and Safari will take care of reloading the tab and showing the crash
> > +        banner then. This is done because we do not want to reload content that is
> > +        using a lot of CPU while in the background.
> 
> This is pretty weird. We're just not telling the truth about when the
> process crashed.

We would notify the client right away but then we would want to implement the logic of delaying the load until the tab becomes visible on client side.
One possible issue of doing so currently is that the current API does not currently let the client know why a process crashed (regular crash, vs termination due to CPU use / memory use).
Unless we change the API, this means it would be hard to implement different policies on client side (maybe we do not want the policies to be different?).

> 
> Why is this behavior specific to the CPU limit, whereas the memory limit
> notifies of crash right away?

I have not coordinated this with Andreas but I personally think we should do the same in the "termination due to memory use" case of background tabs.
This is to prevent yoyo effect.

> 
> Why does WebKit make the reload decision for Safari in this upside-down way,
> instead of allowing Safari to make the policy decision?

Explained above but I am sure we can make this work if we'd want to. We just need to agree on the best way to do so.

[Andreas Kling]

(In reply to comment #12)
> (In reply to comment #11)
> > Why is this behavior specific to the CPU limit, whereas the memory limit
> > notifies of crash right away?
> 
> I have not coordinated this with Andreas but I personally think we should do
> the same in the "termination due to memory use" case of background tabs.
> This is to prevent yoyo effect.

I agree. It would be better to make these two behave the same :)

I think it would be nice to let clients be able to distinguish between different causes of Web process termination. I don’t think it’s a good idea to have WebKit delay the reporting or decide how to handle web process termination. In particular, this doesn’t consider clients who never intend to put a web view in a window, such as an email client that does some processing of HTML email in the background.

[Chris Dumez]

(In reply to comment #14)
> I think it would be nice to let clients be able to distinguish between
> different causes of Web process termination. I don’t think it’s a good idea
> to have WebKit delay the reporting or decide how to handle web process
> termination. In particular, this doesn’t consider clients who never intend
> to put a web view in a window, such as an email client that does some
> processing of HTML email in the background.

Agreed, it would be much nicer to deal with this on client side and will work on this.
I do not know if we really need to distinguish different kinds of terminations. As mentioned earlier, we could maybe treat all crashes / terminations the same way in Safari (i.e. reload only when the tabs becomes visible again).

Unless there is a specific need to expose the termination reason, I would try and avoid it.