I wonder if this was fixed by the recent scheduler change. Trying now

Awesome, thanks Dale! :)
Do you have a date when the scheduler commit landed?
I'd be happy to check out a commit after that and test it in ChromePublic on my devices as well - that way you don't have to do it.

Hmmm, locally I'm getting a "data source error." and playback stops because we can't read off the network. I wonder if the network connection is being closed.

Will have to take a closer look.

Scheduler change was this one https://chromium.googlesource.com/chromium/src/+/31abb5e88e0dc5df1788e75d5096d3a9d4ad1e0c 

Looks like it's the suspend timer firing on the URLRequestJob:

https://cs.chromium.org/chromium/src/net/url_request/url_request_job.cc?l=342

We need a way for that to not fire for actively playing URL loads on Android. Suspend does not work the same on Android as it does on other platforms.

+network folk. Any suggestions on the right approach here? Essentially if there's playing media that's intended to remain playing after suspend we need to keep existing network connections alive and maintain the ability to create new ones (for redirects, XHR based loading).

(this used to work previously because we handed off media loading and playback to the android system -- now Chrome handles it in M52+)

+scheduler folk since this has impact there too. The scheduler itself already defers a host of events when audio is playing, this seems like another class of events that should be deferred (PowerObserver suspend/resume notifications).

Something like this in general will work, but needs more thought.

https://codereview.chromium.org/2310343002

I'm OOO tomorrow, so avayvod@ if you want to pick this up as the background-playback expert and turn that into a non-crappy CL, it's all yours :)

Any idea why the suspension here works differently on Android vs. other platforms? Also, I'm wondering if we should even be backgrounding the process if it's playing audio.

I think the backgrounding already works as expected in v52.
In v51:
While foregrounded, state is foreground_app and oom scores are around 40-50
When backgrounded, if media is playing, state is perceptible_app and oom scores are around 200
When backgrounded, if no media is playing, state is background_app and oom scores are around 450-500

This is identical in Chrome v52 (setting it properly to perceptible_app)
When backgrounded with media, the oom protection immediately swaps to backgrounded_app as soon as the audio stops playing.

So if we can just prevent the audio from stopping, Android should already properly take care of the correct backgrounding state.

+mmenke, do you know much of this behavior?

It seems to me that, if suspend doesn't do what it does on Android as with other platforms, it shouldn't call OnSuspend to begin with. It appears the net stack currently shuts off everything when this happens, idle sockets and all.

Suspend on desktop means suspend to RAM or suspend to disk (And keeping a socket open through that will quite possibly result in having a blackholed object hanging around).  If it means something completely different on Android, I agree we should be using a different notification.

Suspend on Android means there's no visible activity IIRC: https://cs.chromium.org/chromium/src/base/power_monitor/power_monitor_device_source_android.cc?rcl=0&l=31

PowerMonitor (.java) sends the suspend event as soon as Chrome's activities are paused by the system.

Yea....That sounds sufficiently different from what suspend means on desktop, that I'm doubtful that doing the same thing everywhere on both types of events makes any sense at all.  Two notifications really seems like the way to go, with an audit of existing consumers.

Yeah, it sounds like that's completely different then. That should look like no Chrome windows being visible or all of them minimized or something.

Given that's the M52 issue, what's the minimum change we could do to fix this particular issue in the best mergable way?

Wrap the suspend listeners (for example, in URLRequestJob) with #if !defined(OS_ANDROID) ?

Device: Nexus 6
Android version: MOB31H
Web site: "http://www.kqed.org/radio/listen/"

Bisect info:
Good build: 51.0.2696.0  
Bad Build:  51.0.2697.0

https://chromium.googlesource.com/chromium/src/+log/51.0.2696.0..51.0.2697.0?pretty=fuller&n=10000

Is this really a regression?  The net behavior has been the same for quite a while, so doesn't seem like the solution is to rush in a change for old code.  If it is a regression, what CL made it break recently?

Oops, missed comment 20.  None of the net/ changes look relevant to me, though I may be missing something.

The media pipeline became unified with the desktop one and started using the network stack (before the browser process would usually fetch the bits using the Android MediaServer, for example).

https://chromium.googlesource.com/chromium/src/+/7defc181e284d5e24a00a9254d4dc7ac3f2c15cb is the revision from the range that flipped the bit. IIRC, it has been launched 100% on 52 stable since then (it's controlled by a Finch experiment in 51 too).

I don't think we should rush any fix, but fix it correctly and if that turns out to happen quickly and be small enough merge it back to M54. Since we haven't seen a huge outcry for this, I don't think we need to do anything risky -- but it should be fixed.

I suspect there are some valid power reasons for killing hanging network connections when chrome goes into the background -- so it's not clear that we don't want this behavior in general. Does anyone know the context around why we use Suspend/Resume signals on Android at all?

Here's the original commit and referenced bug:
https://chromium.googlesource.com/chromium/src/+/11c25f6b90e8da1f1507cfc21ecea8e72c2849ef
https://bugs.chromium.org/p/chromium/issues/detail?id=164495

+pliard if he wants to add any context.

> I suspect there are some valid power reasons for killing hanging network connections when chrome goes into the background

Note that's not the same as what the OnSuspend logic in the net stack does.

We shouldn't automatically kill active *requests* just because we're going to the background, this bug being an example. That should instead by triggered by the *page* paying attention to Page Visibility API and punting all of its timers. (Of course, then we have misbehaving pages and we may wish to enforce things on them. But I think that is better done by either killing the tab outright or perhaps putting the net stack for that tab in some kind of degraded mode. Either way, not policy we can decide at such a low-level hook.)

I can easily imagine wanting to close or cap *idle* sockets and sessions when we go into the background (although, as with everything, there is a bit of a tradeoff here), and I believe we do that on low memory pressure. That would have to be a different signal than OnSuspend.

As David says, the reason we kill sockets in on suspend on desktop is because the entire PC is going away, not because Chrome is being backgrounded.  The fact that Android's system "suspend" notification was hooked up to desktop Chrome's desktop "OnSuspend" notification is probably a bug, caused by confusion over the fact that "suspend" means two entirely different things.

davidben, mmenke@ can one of pick up this bug or reassign within the network team then? Or alternatively if you want to specify the appropriate fix here I'm happy to implement it.

If the URLRequestJob just shouldn't get Suspend/Resume on Android (instead relying on higher level layers to handle idle suspensions) that's a fairly easy fix to make.

I think we should remove the suspend/resume notification on Android, most likely.  I don't have the time to audit all callers, however, to verify that.  This seems more a Chrome for Android issue than a network stack issue, since it's the API's meaning that changed out from under us, and only on one platform.

As luck would have it most of the callers are media or network, can the network team at least audit the ones in net?

https://cs.chromium.org/search/?q=OnSuspend%5C(%5C)&p=3&sq=package:chromium&type=cs

I know several of the media ones are also special casing non-Android, so I agree it'd be nice to stop sending suspend on Android.

Sure.  The network stack only has two uses of it, both are for the exact same reason - to prevent us from keeping around blackholed sockets when entering and then leaving sleep/suspend mode, so neither makes sense when Chrome is merely backgrounded.

I've audited all the calls outside of net/. The only OnSuspend() receiver which might care about this seems to be:

https://cs.chromium.org/chromium/src/gpu/ipc/service/gpu_watchdog_thread.cc?l=414

The rest either explicitly have !ANDROID clauses, just issue log messages, or aren't compiled on Android.

+sievers@ who thoughts on if OnSuspend/OnResume are necessary calls for the GPU watchdog on Android?

https://codereview.chromium.org/2324923002 sent to CQ for testing.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/610004f26299d1eb31b18f7dce3e2d3740eb8c04

commit 610004f26299d1eb31b18f7dce3e2d3740eb8c04
Author: dalecurtis <dalecurtis@chromium.org>
Date: Fri Sep 09 20:27:07 2016

Remove OnSuspend/OnResume notifications on Android.

These are incorrectly mixing disparate concepts. Chrome code expects
OnSuspend/OnResume to mean the system is about to go to sleep, not
just that there is no more visible activity.

This prevents background tasks like background audio from working
properly since the network connections are force killed. We could
workaround this by improving when OnSuspend/OnResume is sent, but
it seems no one really cares, so just delete these.

In fact a couple pieces of code explicitly reject OnSupend/OnResume
from Android since they do not mean what is expected. An alternate
signal for app suspension should be sent if this is ever needed
again in the future.

BUG= 644515 
TEST=background audio playback works.

Review-Url: https://codereview.chromium.org/2324923002
Cr-Commit-Position: refs/heads/master@{#417691}

[modify] https://crrev.com/610004f26299d1eb31b18f7dce3e2d3740eb8c04/base/android/java/src/org/chromium/base/PowerMonitor.java
[modify] https://crrev.com/610004f26299d1eb31b18f7dce3e2d3740eb8c04/base/power_monitor/power_monitor_device_source_android.cc
[modify] https://crrev.com/610004f26299d1eb31b18f7dce3e2d3740eb8c04/content/renderer/media/peer_connection_tracker.cc
[modify] https://crrev.com/610004f26299d1eb31b18f7dce3e2d3740eb8c04/content/renderer/media/peer_connection_tracker_unittest.cc
[modify] https://crrev.com/610004f26299d1eb31b18f7dce3e2d3740eb8c04/media/renderers/audio_renderer_impl.cc

Will let this soak for a couple weeks and merge back to M54 if things look good.

Your change meets the bar and is auto-approved for M54 (branch: 2840)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9ea1d94129fd5eb34437c3d3f0dccee91e7d0e3f

commit 9ea1d94129fd5eb34437c3d3f0dccee91e7d0e3f
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Mon Sep 26 22:08:07 2016

Remove OnSuspend/OnResume notifications on Android.

These are incorrectly mixing disparate concepts. Chrome code expects
OnSuspend/OnResume to mean the system is about to go to sleep, not
just that there is no more visible activity.

This prevents background tasks like background audio from working
properly since the network connections are force killed. We could
workaround this by improving when OnSuspend/OnResume is sent, but
it seems no one really cares, so just delete these.

In fact a couple pieces of code explicitly reject OnSupend/OnResume
from Android since they do not mean what is expected. An alternate
signal for app suspension should be sent if this is ever needed
again in the future.

BUG= 644515 
TEST=background audio playback works.

Review-Url: https://codereview.chromium.org/2324923002
Cr-Commit-Position: refs/heads/master@{#417691}
(cherry picked from commit 610004f26299d1eb31b18f7dce3e2d3740eb8c04)

Review URL: https://codereview.chromium.org/2369963003 .

Cr-Commit-Position: refs/branch-heads/2840@{#537}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/9ea1d94129fd5eb34437c3d3f0dccee91e7d0e3f/base/android/java/src/org/chromium/base/PowerMonitor.java
[modify] https://crrev.com/9ea1d94129fd5eb34437c3d3f0dccee91e7d0e3f/base/power_monitor/power_monitor_device_source_android.cc
[modify] https://crrev.com/9ea1d94129fd5eb34437c3d3f0dccee91e7d0e3f/media/renderers/audio_renderer_impl.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9ea1d94129fd5eb34437c3d3f0dccee91e7d0e3f

commit 9ea1d94129fd5eb34437c3d3f0dccee91e7d0e3f
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Mon Sep 26 22:08:07 2016

Remove OnSuspend/OnResume notifications on Android.

These are incorrectly mixing disparate concepts. Chrome code expects
OnSuspend/OnResume to mean the system is about to go to sleep, not
just that there is no more visible activity.

This prevents background tasks like background audio from working
properly since the network connections are force killed. We could
workaround this by improving when OnSuspend/OnResume is sent, but
it seems no one really cares, so just delete these.

In fact a couple pieces of code explicitly reject OnSupend/OnResume
from Android since they do not mean what is expected. An alternate
signal for app suspension should be sent if this is ever needed
again in the future.

BUG= 644515 
TEST=background audio playback works.

Review-Url: https://codereview.chromium.org/2324923002
Cr-Commit-Position: refs/heads/master@{#417691}
(cherry picked from commit 610004f26299d1eb31b18f7dce3e2d3740eb8c04)

Review URL: https://codereview.chromium.org/2369963003 .

Cr-Commit-Position: refs/branch-heads/2840@{#537}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/9ea1d94129fd5eb34437c3d3f0dccee91e7d0e3f/base/android/java/src/org/chromium/base/PowerMonitor.java
[modify] https://crrev.com/9ea1d94129fd5eb34437c3d3f0dccee91e7d0e3f/base/power_monitor/power_monitor_device_source_android.cc
[modify] https://crrev.com/9ea1d94129fd5eb34437c3d3f0dccee91e7d0e3f/media/renderers/audio_renderer_impl.cc