Bug 224862 – [GTK][WPE] Add a timer to periodically check the responsiveness of web processes with visible pages

• Status: NEW
• Component: WebKitGTK

[Miguel Gomez]

Currently there are 3 ways to detect the responsiveness of a web process:
- Check that there's a reply from the web process when there's an input event
- Check that there's an answer from the web process when there's an API call (only for some methods)
- Periodical timer pinging the web process when the process doesn't have any visible web page (BackgroundProcessResponsivenessTimer)

What we don't have is a way to periodically ping the processes with visible pages. This means that on situations where the user is not generating input events we won't detect unresponsive web processes (watching a video without interacting with the page, for example). We would need to do some periodical check on visible pages as well.

[Miguel Gomez]

Created attachment 426683 [details]
Patch

[Miguel Gomez]

(In reply to Miguel Gomez from comment #1)
> Created attachment 426683 [details]
> Patch

Basically I've added a new timer similar to BackgroundProcessResponsivenessTimer that is enabled when the process contains some visible page. It pings the web process every 10 seconds and waits for an answer for 3 seconds. It gets disabled when the page becomes invisible.

[Miguel Gomez]

I think there's a better solution than the one I implemented, so please don't review this patch. I'll send a new one with a better approach.

[Miguel Gomez]

Created attachment 426902 [details]
Patch

[Miguel Gomez]

Created attachment 427044 [details]
Patch

[Miguel Gomez]

Created attachment 427047 [details]
Patch

[Carlos Garcia Campos]

This needs wk2 owner approval.

[Carlos Garcia Campos]

Comment on attachment 427047 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        web process was received, and it's enabled for the GTK and WPE ports only.

I wonder why this is only useful for GTK/WPE.

> Source/WebKit/UIProcess/ResponsivenessTimer.cpp:218
> +bool ResponsivenessTimer::isPeriodicalCheckActive() const
> +{
> +    return m_periodicalCheckTimer.isActive();
> +}

Do we really need this? This is private, right?

> Source/WebKit/UIProcess/ResponsivenessTimer.h:90
>      RunLoop::Timer<ResponsivenessTimer> m_timer;
> +    RunLoop::Timer<ResponsivenessTimer> m_periodicalCheckTimer;

Now that I see this, we should set a priority for this timers when USE(GLIB_EVENT_LOOP). Not as a part of this patch, though.

> Source/WebKit/UIProcess/WebProcessProxy.cpp:1629
>      m_backgroundResponsivenessTimer.updateState();
> +    m_responsivenessTimer.scheduleNextPeriodicalCheckIfNeeded();

I wonder if we should merge all responsivness timers in a single class.

[Miguel Gomez]

 
> I wonder if we should merge all responsivness timers in a single class.

We could use the BackgroundProcessResponsivenessTimer to always perform periodic pings (whether the proxy has visible pages or not), but use different intervals: if there are visible pages ping every 10 seconds and if not use the incremental interval that's currently used. I thought of doing this, but then it wouldn't be a BackgroundProcessResponsivenessTimer anymore, but a PeriodicResponsivenessTimer (or something like that).

[Miguel Gomez]

As a side note, I've tested how this works on Chrome and Firefox. Chrome doesn't seem to use any periodic timer, and the unresponsiveness is only detected if there's user input. Firefox, on the other hand, detects unresponsive pages without the need of user input.

[Sergio Villar Senin]

I wonder what are the implications from battery usage POV for embedded devices having this periodic poll. Maybe waking up every 10s or so is not that a big deal but I guess it depends on the device. It'd be awesome if we could schedule it in a way that is not execute until some other work kicks in.

Anyway not blocking it just dumping some thoughts.

[Miguel Gomez]

(In reply to Sergio Villar Senin from comment #11)
> I wonder what are the implications from battery usage POV for embedded
> devices having this periodic poll. Maybe waking up every 10s or so is not
> that a big deal but I guess it depends on the device. It'd be awesome if we
> could schedule it in a way that is not execute until some other work kicks
> in.
> 
> Anyway not blocking it just dumping some thoughts.

The timer only works as long as the page is visible. When the page is not visible the BackgroundProcessResponsivenessTimer is the one working (that's already happening), but using much bigger timeout periods (starts at 20_s and doubles each time it fires).

The question here is whether the applications using the web view will be setting the appropriate ActivityState flags. If locking the device causes the app to turn of the visibility of the page, this timer won't kick in (the background one will). But if the visibility stays on, this timer will run every ten seconds. Unless the device has some other way to maybe suspend the processes while locked, in which case it doesn't matter what we do.

But this is the same situation as video playback for example. Locking a device that's playing a video should stop the playback. I've just tested in my phone that locking the device stops the video playback on chrome, so something is being done on the web view to notify that the playback must be stopped.

[Sergio Villar Senin]

(In reply to Miguel Gomez from comment #12)
> (In reply to Sergio Villar Senin from comment #11)
> > I wonder what are the implications from battery usage POV for embedded
> > devices having this periodic poll. Maybe waking up every 10s or so is not
> > that a big deal but I guess it depends on the device. It'd be awesome if we
> > could schedule it in a way that is not execute until some other work kicks
> > in.
> > 
> > Anyway not blocking it just dumping some thoughts.
> 
> The timer only works as long as the page is visible. When the page is not
> visible the BackgroundProcessResponsivenessTimer is the one working (that's
> already happening), but using much bigger timeout periods (starts at 20_s
> and doubles each time it fires).
> 
> The question here is whether the applications using the web view will be
> setting the appropriate ActivityState flags. If locking the device causes
> the app to turn of the visibility of the page, this timer won't kick in (the
> background one will). But if the visibility stays on, this timer will run
> every ten seconds. Unless the device has some other way to maybe suspend the
> processes while locked, in which case it doesn't matter what we do.
> 
> But this is the same situation as video playback for example. Locking a
> device that's playing a video should stop the playback. I've just tested in
> my phone that locking the device stops the video playback on chrome, so
> something is being done on the web view to notify that the playback must be
> stopped.

https://www.w3.org/TR/page-visibility-2/ maybe?