[Android] Suspend shared timers for hidden renderers

[Android] Suspend shared timers for hidden renderers

Instead of tracking the visibility state of all renderers in the browser
and using that to kill shared timers, make each renderer process
responsible for the suspension of its own shared timers after it is
completely hidden. This ensures background tabs eventually go silent
even while a foreground tab (in a separate process) is active.

BUG=469049, 486135
Committed: https://crrev.com/73220f023ede1c7bd5c4bd5548e9b5f2b3531430
Cr-Commit-Position: refs/heads/master@{#347434}

[jdduke (slow), 2015-05-12 17:53:55]

Patchset #2 (id:20001) has been deleted

[jdduke (slow), 2015-05-12 18:12:47]

jdduke@chromium.org changed reviewers:
+ sievers@chromium.org

[jdduke (slow), 2015-05-12 18:12:48]

sievers, skyostil: Thoughts? 

I considered putting the logic in RenderProcessHostImpl (using an IPC to control
suspension), but it didn't seem all that better than just doing it here. I also
considered using a flag to control suspension, but this seemed like slightly
less wiring and I didn't have a strong preference between the approaches. After
this, WebView will be the only user of the IPC that suspends timers.

[jdduke (slow), 2015-05-12 18:14:36]

https://codereview.chromium.org/1133143003/diff/60001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1133143003/diff/60001/content/renderer/render...
content/renderer/render_thread_impl.cc:1192:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
I guess we can just query the scheduler task runner here.

[Sami, 2015-05-14 10:47:07]

skyostil@chromium.org changed reviewers:
+ skyostil@chromium.org

[Sami, 2015-05-14 10:47:08]

Like we talked offline, RendererSchedulerImpl is probably a better home for this
logic to avoid having the two systems stomping on each other.

[jdduke (slow), 2015-05-14 15:00:48]

On 2015/05/14 10:47:08, Sami wrote:
> Like we talked offline, RendererSchedulerImpl is probably a better home for
this
> logic to avoid having the two systems stomping on each other.

Can we do that before we stop relying on
BlinkPlatform::Suspend/ResumeSharedTimer (from RenderThreadImpl)?

[Sami, 2015-05-14 15:09:08]

On 2015/05/14 15:00:48, jdduke wrote:
> On 2015/05/14 10:47:08, Sami wrote:
> > Like we talked offline, RendererSchedulerImpl is probably a better home for
> this
> > logic to avoid having the two systems stomping on each other.
> 
> Can we do that before we stop relying on
> BlinkPlatform::Suspend/ResumeSharedTimer (from RenderThreadImpl)?

Timer suspension is already implemented there with
renderer_scheduler_->SuspendTimerQueue(). The only missing part is posting
timers from Blink the timer task queue so that suspension will actually have an
effect. In other words, the background suspension logic could be landed in the
scheduler today, but it would only take an effect once Blink starts using the
timer task queue.

[jdduke (slow), 2015-05-14 15:20:45]

On 2015/05/14 15:09:08, Sami wrote:
> On 2015/05/14 15:00:48, jdduke wrote:
> > On 2015/05/14 10:47:08, Sami wrote:
> > > Like we talked offline, RendererSchedulerImpl is probably a better home
for
> > this
> > > logic to avoid having the two systems stomping on each other.
> > 
> > Can we do that before we stop relying on
> > BlinkPlatform::Suspend/ResumeSharedTimer (from RenderThreadImpl)?
> 
> Timer suspension is already implemented there with
> renderer_scheduler_->SuspendTimerQueue(). The only missing part is posting
> timers from Blink the timer task queue so that suspension will actually have
an
> effect. In other words, the background suspension logic could be landed in the
> scheduler today, but it would only take an effect once Blink starts using the
> timer task queue.

Exactly, any ETA?

[Sami, 2015-05-14 15:36:35]

On 2015/05/14 15:20:45, jdduke wrote:
> On 2015/05/14 15:09:08, Sami wrote:
> > On 2015/05/14 15:00:48, jdduke wrote:
> > > On 2015/05/14 10:47:08, Sami wrote:
> > > > Like we talked offline, RendererSchedulerImpl is probably a better home
> for
> > > this
> > > > logic to avoid having the two systems stomping on each other.
> > > 
> > > Can we do that before we stop relying on
> > > BlinkPlatform::Suspend/ResumeSharedTimer (from RenderThreadImpl)?
> > 
> > Timer suspension is already implemented there with
> > renderer_scheduler_->SuspendTimerQueue(). The only missing part is posting
> > timers from Blink the timer task queue so that suspension will actually have
> an
> > effect. In other words, the background suspension logic could be landed in
the
> > scheduler today, but it would only take an effect once Blink starts using
the
> > timer task queue.
> 
> Exactly, any ETA?

Any day now :) The prerequisite patches had some trouble sticking initially but
are looking a bit better now:

https://codereview.chromium.org/1127123010/
https://codereview.chromium.org/1130413003/
https://codereview.chromium.org/1134523002/

[jdduke (slow), 2015-05-14 15:38:19]

On 2015/05/14 15:36:35, Sami wrote:
> On 2015/05/14 15:20:45, jdduke wrote:
> > On 2015/05/14 15:09:08, Sami wrote:
> > > On 2015/05/14 15:00:48, jdduke wrote:
> > > > On 2015/05/14 10:47:08, Sami wrote:
> > > > > Like we talked offline, RendererSchedulerImpl is probably a better
home
> > for
> > > > this
> > > > > logic to avoid having the two systems stomping on each other.
> > > > 
> > > > Can we do that before we stop relying on
> > > > BlinkPlatform::Suspend/ResumeSharedTimer (from RenderThreadImpl)?
> > > 
> > > Timer suspension is already implemented there with
> > > renderer_scheduler_->SuspendTimerQueue(). The only missing part is posting
> > > timers from Blink the timer task queue so that suspension will actually
have
> > an
> > > effect. In other words, the background suspension logic could be landed in
> the
> > > scheduler today, but it would only take an effect once Blink starts using
> the
> > > timer task queue.
> > 
> > Exactly, any ETA?
> 
> Any day now :) The prerequisite patches had some trouble sticking initially
but
> are looking a bit better now:
> 
> https://codereview.chromium.org/1127123010/
> https://codereview.chromium.org/1130413003/
> https://codereview.chromium.org/1134523002/

Oh nice, didn't realize it was that close. I'll move the code now then :).

[no sievers, 2015-05-15 00:10:38]

https://codereview.chromium.org/1133143003/diff/60001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1133143003/diff/60001/content/renderer/render...
content/renderer/render_thread_impl.cc:1888: // TODO(jdduke): Consider instead
using the "backgrounded" signal from the
Hmm is that a dangerous combination of not running IdleHandler() and
garbage-collection but running blink shared timers?

https://codereview.chromium.org/1133143003/diff/60001/content/renderer/render...
content/renderer/render_thread_impl.cc:1889: // browser, which accomodates audio
playback when the renderer is hidden, see
Hmm, so what's the plan for that? Should we turn the
OnSetWebKitSharedTimersSuspended() IPC into the opposite: a sort of timer wake
lock?

On the browser side, I also think that shouldn't be exposed to the application
anymore, but an implementation detail (like based on audible state of all tabs).
Seems like we might be able to manage that entirely renderer-side also then.

[jdduke (slow), 2015-05-15 15:41:19]

+qinmin per the comment about audio playback.

https://codereview.chromium.org/1133143003/diff/60001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1133143003/diff/60001/content/renderer/render...
content/renderer/render_thread_impl.cc:1889: // browser, which accomodates audio
playback when the renderer is hidden, see
On 2015/05/15 00:10:38, sievers wrote:
> Hmm, so what's the plan for that? Should we turn the
> OnSetWebKitSharedTimersSuspended() IPC into the opposite: a sort of timer wake
> lock?
> 
> On the browser side, I also think that shouldn't be exposed to the application
> anymore, but an implementation detail (like based on audible state of all
tabs).
> Seems like we might be able to manage that entirely renderer-side also then.

Ideally, yes, this could all be done renderer side. THe |SetBackgrounded| IPC is
sent when 1) all widgets in a renderer are hidden, and 2) the renderer has no
active audio. I don't know enough about audio playback management to know if the
renderer has sufficient data to make that call.

Ideally, once audio terminates, we'd get another signal from somewhere, and we
could start the timeout (maybe suspend it if we notice audio comes back in? not
sure of the granularity of the audio active signals...).

[jdduke (slow), 2015-08-27 22:55:36]

Sami, ps#6 implements something like what we discussed, where we tell the
scheduler we're backgrounded and let it  do the dirty work. I can't say that I
prefer that version much more than the original one (ps#5), primarily because it
introduces some platform specific code in the scheduler which until now has not
been necessary. Even then, I don't think an #ifdef is enough here, as this
behavior probably isn't appropriate for WebView.

I suppose we could pass in some arguments to the scheduler to configure the
behavior (pulled from the  ContentRendererClient), either in the constructor or
in the "backgrounded" method. Perhaps pass in the delay for killing timers
directly? Thoughts?

Haven't added tests, will do so after we figure out the right API.

[Sami, 2015-08-28 14:15:01]

On 2015/08/27 22:55:36, jdduke wrote:
> Sami, ps#6 implements something like what we discussed, where we tell the
> scheduler we're backgrounded and let it  do the dirty work. I can't say that I
> prefer that version much more than the original one (ps#5), primarily because
it
> introduces some platform specific code in the scheduler which until now has
not
> been necessary. Even then, I don't think an #ifdef is enough here, as this
> behavior probably isn't appropriate for WebView.

On first blush I think I prefer this new version since this is clearly a
scheduling decision so it kind of fits here. I could imagine us treating
backgrounding as a new "use case" (https://codereview.chromium.org/1320633002/)
which could also do other things like suspending network traffic. No need to
block on that patch though IMO.

> I suppose we could pass in some arguments to the scheduler to configure the
> behavior (pulled from the  ContentRendererClient), either in the constructor
or
> in the "backgrounded" method. Perhaps pass in the delay for killing timers
> directly? Thoughts?
> 
> Haven't added tests, will do so after we figure out the right API.

A boolean enable/disable seems like the cleanest to me. Could either be a
constructor parameter or just a setter/getter.

https://codereview.chromium.org/1133143003/diff/120001/components/scheduler/r...
File components/scheduler/renderer/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1133143003/diff/120001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.h:144: static const int
kSuspendTimersWhenBackgroundedDelayMillis = 5 * 60 * 1000;
Just thinking ahead to testing this, perhaps it would be better to have a
boolean enable that we can call where the scheduler is created? That way the
tests can make sure this works on regardless of the platform they're running on.

https://codereview.chromium.org/1133143003/diff/120001/content/child/child_th...
File content/child/child_thread_impl.cc (right):

https://codereview.chromium.org/1133143003/diff/120001/content/child/child_th...
content/child/child_thread_impl.cc:630: void
ChildThreadImpl::OnProcessBackgrounded(bool backgrounded) {
Nothing changed here -- you just moved this around, right?

https://codereview.chromium.org/1133143003/diff/120001/content/renderer/rende...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/1133143003/diff/120001/content/renderer/rende...
content/renderer/render_thread_impl.h:480: void
OnSetWebKitSharedTimersSuspendedAfterHidden(bool suspend);
I guess this isn't needed?

[jdduke (slow), 2015-08-31 18:36:08]

Added a test, thoughts? It's a bit more plumbing than I'd like, but I agree that
this is probably a more maintainable approach than cramming it in
RenderThreadImpl.

https://codereview.chromium.org/1133143003/diff/120001/components/scheduler/r...
File components/scheduler/renderer/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1133143003/diff/120001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.h:144: static const int
kSuspendTimersWhenBackgroundedDelayMillis = 5 * 60 * 1000;
On 2015/08/28 14:15:01, Sami wrote:
> Just thinking ahead to testing this, perhaps it would be better to have a
> boolean enable that we can call where the scheduler is created? That way the
> tests can make sure this works on regardless of the platform they're running
on.

Done.

https://codereview.chromium.org/1133143003/diff/120001/content/child/child_th...
File content/child/child_thread_impl.cc (right):

https://codereview.chromium.org/1133143003/diff/120001/content/child/child_th...
content/child/child_thread_impl.cc:630: void
ChildThreadImpl::OnProcessBackgrounded(bool backgrounded) {
On 2015/08/28 14:15:01, Sami wrote:
> Nothing changed here -- you just moved this around, right?

Yep, moved it to preserve ordering as I moved the declaration (had to make it
virtual).

https://codereview.chromium.org/1133143003/diff/120001/content/renderer/rende...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/1133143003/diff/120001/content/renderer/rende...
content/renderer/render_thread_impl.h:480: void
OnSetWebKitSharedTimersSuspendedAfterHidden(bool suspend);
On 2015/08/28 14:15:01, Sami wrote:
> I guess this isn't needed?

Done.

[Sami, 2015-09-01 11:09:29]

Thanks for adding the test. One more suggestion for making the scheduler logic a
little easier to read.

https://codereview.chromium.org/1133143003/diff/140001/components/scheduler/r...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1133143003/diff/140001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.cc:772:
SuspendTimerQueue();
Could you instead call ForceUpdatePolicy() here and have logic at the end of
UpdatePolicyLocked that disables timers when we're backgrounded? I think that
would be slightly clearer than reusing timer queue suspension.

[jdduke (slow), 2015-09-01 16:36:49]

Patchset #8 (id:160001) has been deleted

[jdduke (slow), 2015-09-01 16:37:03]

https://codereview.chromium.org/1133143003/diff/140001/components/scheduler/r...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1133143003/diff/140001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.cc:772:
SuspendTimerQueue();
On 2015/09/01 11:09:29, Sami wrote:
> Could you instead call ForceUpdatePolicy() here and have logic at the end of
> UpdatePolicyLocked that disables timers when we're backgrounded? I think that
> would be slightly clearer than reusing timer queue suspension.

Done. I played with another variant that got rid of these two methods and
instead set a sort of deadline after which timer suspension was allowed.
However, it didn't really save any state, and I slightly prefer the explicit
calls here instead of the time comparison magic.

[Sami, 2015-09-01 17:08:16]

Thanks, lgtm!

https://codereview.chromium.org/1133143003/diff/180001/components/scheduler/r...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1133143003/diff/180001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.cc:654: // Note that this
will only take affect for the next backgrounded signal.
nit: s/affect/effect/

https://codereview.chromium.org/1133143003/diff/180001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.cc:701:
state->SetBoolean("timer_queue_suspended_when_backgrounded_",
nit: no trailing underscores (here and below)

[jdduke (slow), 2015-09-01 17:38:58]

Thanks for review.

sievers@ could you take a look at content/renderer and content/child?

https://codereview.chromium.org/1133143003/diff/180001/components/scheduler/r...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1133143003/diff/180001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.cc:654: // Note that this
will only take affect for the next backgrounded signal.
On 2015/09/01 17:08:16, Sami wrote:
> nit: s/affect/effect/

Done. Embarrassing.

https://codereview.chromium.org/1133143003/diff/180001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.cc:701:
state->SetBoolean("timer_queue_suspended_when_backgrounded_",
On 2015/09/01 17:08:16, Sami wrote:
> nit: no trailing underscores (here and below)

Done.

[no sievers, 2015-09-03 19:33:48]

content lgtm

[jdduke (slow), 2015-09-03 22:03:26]

jdduke@chromium.org changed reviewers:
+ sky@chromium.org

[jdduke (slow), 2015-09-03 22:03:26]

Turns out we need a different signal than OS_ANDROID to enable this, as we don't
(yet anyway) want this behavior with WebView. I added a getter to
ContentRendererClient that gives us the granularity we need.

+sky for chrome/renderer

[sky, 2015-09-03 23:08:45]

LGTM

[jdduke (slow), 2015-09-04 16:07:36]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2015-09-04 16:07:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1133143003/#ps210001
(title: "Use ContentRendererClient")

[commit-bot: I haz the power, 2015-09-04 16:07:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133143003/210001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1133143003/210001

[commit-bot: I haz the power, 2015-09-04 17:03:57]

Committed patchset #10 (id:210001)

[commit-bot: I haz the power, 2015-09-04 17:04:42]

Patchset 10 (id:??) landed as
https://crrev.com/73220f023ede1c7bd5c4bd5548e9b5f2b3531430
Cr-Commit-Position: refs/heads/master@{#347434}

[Yaron, 2015-09-29 17:42:47]

On 2015/09/04 17:04:42, commit-bot: I haz the power wrote:
> Patchset 10 (id:??) landed as
> https://crrev.com/73220f023ede1c7bd5c4bd5548e9b5f2b3531430
> Cr-Commit-Position: refs/heads/master@{#347434}

drive-by: I don't know the full context of this but does it mean that
ChromeApplication.BackgroundProcessing logic can be simplified or removed?
There's some logic there and in ContentViewStatics which attempts to handle
background processing and suspend timers. Is it redundant now?

[Yaron, 2015-09-29 17:42:49]

On 2015/09/04 17:04:42, commit-bot: I haz the power wrote:
> Patchset 10 (id:??) landed as
> https://crrev.com/73220f023ede1c7bd5c4bd5548e9b5f2b3531430
> Cr-Commit-Position: refs/heads/master@{#347434}

drive-by: I don't know the full context of this but does it mean that
ChromeApplication.BackgroundProcessing logic can be simplified or removed?
There's some logic there and in ContentViewStatics which attempts to handle
background processing and suspend timers. Is it redundant now?

[jdduke (slow), 2015-09-29 17:47:53]

On 2015/09/29 17:42:49, Yaron wrote:
> On 2015/09/04 17:04:42, commit-bot: I haz the power wrote:
> > Patchset 10 (id:??) landed as
> > https://crrev.com/73220f023ede1c7bd5c4bd5548e9b5f2b3531430
> > Cr-Commit-Position: refs/heads/master@{#347434}
> 
> drive-by: I don't know the full context of this but does it mean that
> ChromeApplication.BackgroundProcessing logic can be simplified or removed?
> There's some logic there and in ContentViewStatics which attempts to handle
> background processing and suspend timers. Is it redundant now?

Eventually, yes absolutely. We still need to get accurate audio signals though
to ensure we always get the SetBackgrounded signal when appropriate (see
crbug.com/489789). I think we'll still need the ContentViewStatics hook to allow
WebView timer suspension, but we'll definitely be able to get rid of the
ChromeApplication.BackgroundProcessing stuff. Let me file a new bug to ensure
that cleanup happens.