Bug 224862 - [GTK][WPE] Add a timer to periodically check the responsiveness of web processes with visible pages
Summary: [GTK][WPE] Add a timer to periodically check the responsiveness of web proces...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Miguel Gomez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-04-21 02:31 PDT by Miguel Gomez
Modified: 2021-04-29 05:00 PDT (History)
7 users (show)

See Also:


Attachments
Patch (21.59 KB, patch)
2021-04-21 07:47 PDT, Miguel Gomez
no flags Details | Formatted Diff | Diff
Patch (14.69 KB, patch)
2021-04-23 06:29 PDT, Miguel Gomez
no flags Details | Formatted Diff | Diff
Patch (8.32 KB, patch)
2021-04-26 07:42 PDT, Miguel Gomez
no flags Details | Formatted Diff | Diff
Patch (12.37 KB, patch)
2021-04-26 08:17 PDT, Miguel Gomez
magomez: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Miguel Gomez 2021-04-21 02:31:23 PDT
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.
Comment 1 Miguel Gomez 2021-04-21 07:47:35 PDT
Created attachment 426683 [details]
Patch
Comment 2 Miguel Gomez 2021-04-21 07:53:12 PDT
(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.
Comment 3 Miguel Gomez 2021-04-23 00:50:23 PDT
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.
Comment 4 Miguel Gomez 2021-04-23 06:29:44 PDT
Created attachment 426902 [details]
Patch
Comment 5 Miguel Gomez 2021-04-26 07:42:46 PDT
Created attachment 427044 [details]
Patch
Comment 6 Miguel Gomez 2021-04-26 08:17:23 PDT
Created attachment 427047 [details]
Patch
Comment 7 Carlos Garcia Campos 2021-04-29 01:44:41 PDT
This needs wk2 owner approval.
Comment 8 Carlos Garcia Campos 2021-04-29 01:51:35 PDT
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.
Comment 9 Miguel Gomez 2021-04-29 02:16:04 PDT
 
> 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).
Comment 10 Miguel Gomez 2021-04-29 02:18:08 PDT
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.
Comment 11 Sergio Villar Senin 2021-04-29 03:20:23 PDT
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.
Comment 12 Miguel Gomez 2021-04-29 04:04:23 PDT
(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.
Comment 13 Sergio Villar Senin 2021-04-29 05:00:48 PDT
(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?