Summary: | [GTK][WPE] Add a timer to periodically check the responsiveness of web processes with visible pages | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Miguel Gomez <magomez> | ||||||||||
Component: | WebKitGTK | Assignee: | Miguel Gomez <magomez> | ||||||||||
Status: | NEW --- | ||||||||||||
Severity: | Normal | CC: | achristensen, bugs-noreply, cdumez, cgarcia, mcatanzaro, svillar, youennf | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=246330 | ||||||||||||
Attachments: |
|
Description
Miguel Gomez
2021-04-21 02:31:23 PDT
Created attachment 426683 [details]
Patch
(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. 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. Created attachment 426902 [details]
Patch
Created attachment 427044 [details]
Patch
Created attachment 427047 [details]
Patch
This needs wk2 owner approval. 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.
> 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).
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. 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. (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. (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? |