Fire SystemMonitor::{RESUME,SUSPEND}_EVENT on Android.

Fire SystemMonitor::{RESUME,SUSPEND}_EVENT on Android.

This improves SystemMonitor on Android by firing RESUME/SUSPEND events when the
Android main activity goes to foreground/background.
This lets the Android port use the same code path as other platforms to support
the close idle network connections functionnality.

SUSPEND events are now fired 1 minute after the main activity goes to
background.

Additionnally this CL cleans up ActivityStatus used by the Java side
SystemMonitor class. ActivityStatus was suffering from various refactorings and
was providing a counter intuitive interface with the Listener/StateListener
duality.
In particular it was possible to call SystemMonitor.onPause/Resume() as opposed
to onStateChange(). This could lead to SystemMonitor.getActivity() returning
null. The issue was raised while this CL was prepared.

BUG=164495
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172864

[Philippe, 2012-12-11 17:42:48]

I'm sending this to you Marcus for a sanity check. I will add other reviewers
after you confirmed this looks sane although you are an OWNER of pretty much all
the directories affected by this CL :)

There will be a small CL too on the application side that we will have to land
when doing the DEPS roll.

[Philippe, 2012-12-11 18:05:37]

On 2012/12/11 17:42:48, Philippe wrote:
> I'm sending this to you Marcus for a sanity check. I will add other reviewers
> after you confirmed this looks sane although you are an OWNER of pretty much
all
> the directories affected by this CL :)
> 
> There will be a small CL too on the application side that we will have to land
> when doing the DEPS roll.

+Ryan for changes in net/http/http_network_layer.cc
+Yaron for everything else

[Philippe, 2012-12-11 18:09:58]

On 2012/12/11 17:42:48, Philippe wrote:
> I'm sending this to you Marcus for a sanity check. I will add other reviewers
> after you confirmed this looks sane although you are an OWNER of pretty much
all
> the directories affected by this CL :)
> 
> There will be a small CL too on the application side that we will have to land
> when doing the DEPS roll.

FYI, I didn't wait for Marcus finally since this is almost the end of the day in
Europe :)

[Ryan Hamilton, 2012-12-11 18:14:02]

net/http/http_network_layer.cc LGTM

[bulach, 2012-12-11 19:00:20]

lgtm for android, one nit below, thanks! you may want to add vandebo, he
reviewed my previous patch for system_monitor.

https://chromiumcodereview.appspot.com/11538008/diff/3001/base/android/java/s...
File base/android/java/src/org/chromium/base/SystemMonitor.java (right):

https://chromiumcodereview.appspot.com/11538008/diff/3001/base/android/java/s...
base/android/java/src/org/chromium/base/SystemMonitor.java:26: private final
Handler mHandler = new Handler();
this would be clearer, no need for comment:

private final Handler mMainHandler = new Handler(Looper.getMainLooper());

[Yaron, 2012-12-11 19:25:23]

+digit - The ActivityStatus changes seem like they're going backwards. I think
you caught digit in the middle of a refactoring:
https://codereview.chromium.org/11419287/ (i.e. listener is becoming
StateListener and you're unwinding the new codepath)

[Philippe, 2012-12-12 09:46:13]

On 2012/12/11 19:25:23, Yaron wrote:
> +digit - The ActivityStatus changes seem like they're going backwards. I think
> you caught digit in the middle of a refactoring:
> https://codereview.chromium.org/11419287/ (i.e. listener is becoming
> StateListener and you're unwinding the new codepath)

Thanks guys!

+vandebo for changes in base/system_monitor/
+willchan for base/system_monitor/ approval

Yaron, thanks for catching this. I will talk to Digit. I think my change is
still compatible with his needs (after a sync + small changes).
However I might have to go a little further in the cleanup/refactoring.
ActivityStatus was initially designed to notify listeners about state changes of
the *main* activity. The problem is that it is now used for all the activities
subclassing ChromiumActivity. Clients are assuming they are notified for changes
of the main activity only. This is no longer true. In my case I don't want to
close the idle connections when a popup activity is stopped. I'm only interested
in the main activity's state. This might not be too problematic yet if only a
few clients are subclassing ChromiumActivity but this could change soon. I will
see how I can address that.

[Philippe, 2012-12-12 10:02:47]

On 2012/12/12 09:46:13, Philippe wrote:
> On 2012/12/11 19:25:23, Yaron wrote:
> > +digit - The ActivityStatus changes seem like they're going backwards. I
think
> > you caught digit in the middle of a refactoring:
> > https://codereview.chromium.org/11419287/ (i.e. listener is becoming
> > StateListener and you're unwinding the new codepath)
> 
> Thanks guys!
> 
> +vandebo for changes in base/system_monitor/
> +willchan for base/system_monitor/ approval
> 
> Yaron, thanks for catching this. I will talk to Digit. I think my change is
> still compatible with his needs (after a sync + small changes).
> However I might have to go a little further in the cleanup/refactoring.
> ActivityStatus was initially designed to notify listeners about state changes
of
> the *main* activity. The problem is that it is now used for all the activities
> subclassing ChromiumActivity. Clients are assuming they are notified for
changes
> of the main activity only. This is no longer true. In my case I don't want to
> close the idle connections when a popup activity is stopped. I'm only
interested
> in the main activity's state. This might not be too problematic yet if only a
> few clients are subclassing ChromiumActivity but this could change soon. I
will
> see how I can address that.

Please ignore my last comment, I misunderstood ChromiumActivity. Only *main*
activities are supposed to subclass it.

[Philippe, 2012-12-12 10:39:52]

https://chromiumcodereview.appspot.com/11538008/diff/3001/base/android/java/s...
File base/android/java/src/org/chromium/base/SystemMonitor.java (right):

https://chromiumcodereview.appspot.com/11538008/diff/3001/base/android/java/s...
base/android/java/src/org/chromium/base/SystemMonitor.java:26: private final
Handler mHandler = new Handler();
On 2012/12/11 19:00:20, bulach wrote:
> this would be clearer, no need for comment:
> 
> private final Handler mMainHandler = new Handler(Looper.getMainLooper());

Nice, thanks!

[Philippe, 2012-12-12 16:36:41]

On 2012/12/12 10:39:52, Philippe wrote:
>
https://chromiumcodereview.appspot.com/11538008/diff/3001/base/android/java/s...
> File base/android/java/src/org/chromium/base/SystemMonitor.java (right):
> 
>
https://chromiumcodereview.appspot.com/11538008/diff/3001/base/android/java/s...
> base/android/java/src/org/chromium/base/SystemMonitor.java:26: private final
> Handler mHandler = new Handler();
> On 2012/12/11 19:00:20, bulach wrote:
> > this would be clearer, no need for comment:
> > 
> > private final Handler mMainHandler = new Handler(Looper.getMainLooper());
> 
> Nice, thanks!

FYI, I need to land this CL before the end of Friday. The bug is a ReleaseBlock
Dev.
I'm just missing the review/approval of the changes in base/system_monitor/
(vandebo/willchan).

[willchan no longer on Chromium, 2012-12-12 17:30:06]

The change in system_monitor_android.cc lgtm.

I am concerned about the change in net/. Your change looks correct, but it begs
the question...how did this work before? I know this had to have worked at some
point in time, since rvargas@ explicitly wrote this to work around bugs in the
Windows XP kernel leading to BSOD when sockets were still alive on suspend, so
we had to close the idle ones when we suspended.

I did some archaeology and it looks like I regressed the behavior a year and a
half ago! https://codereview.chromium.org/7043007
https://codereview.chromium.org/7043007/diff/17003/net/http/http_network_layer.h
https://codereview.chromium.org/7043007/diff/17003/net/http/http_network_laye...

Good ol' willchan, always breaking shit. But that sort of means we don't need
this suspend/resume crap, and that everything's covered by the PowerObserver in
URLRequestJob already. I have to wonder if even that's necessary! It's also very
likely the case that it's unnecessary due to our NetworkChangeNotifier which
flushes the socket pools on network changes, which are also triggered when
suspend/resume happens (at least on desktop). When your app gets backgrounded, I
assume NetworkChangeNotifier doesn't fire any events, right?

Btw, are you sure it saves more batteries to close the idle sockets?

[Philippe, 2012-12-12 17:59:31]

On 2012/12/12 17:30:06, willchan wrote:
> The change in system_monitor_android.cc lgtm.
> 
> I am concerned about the change in net/. Your change looks correct, but it
begs
> the question...how did this work before? I know this had to have worked at
some
> point in time, since rvargas@ explicitly wrote this to work around bugs in the
> Windows XP kernel leading to BSOD when sockets were still alive on suspend, so
> we had to close the idle ones when we suspended.
> 
> I did some archaeology and it looks like I regressed the behavior a year and a
> half ago! https://codereview.chromium.org/7043007
>
https://codereview.chromium.org/7043007/diff/17003/net/http/http_network_layer.h
>
https://codereview.chromium.org/7043007/diff/17003/net/http/http_network_laye...
> 
> Good ol' willchan, always breaking shit. But that sort of means we don't need
> this suspend/resume crap, and that everything's covered by the PowerObserver
in
> URLRequestJob already. I have to wonder if even that's necessary! It's also
very
> likely the case that it's unnecessary due to our NetworkChangeNotifier which
> flushes the socket pools on network changes, which are also triggered when
> suspend/resume happens (at least on desktop). When your app gets backgrounded,
I
> assume NetworkChangeNotifier doesn't fire any events, right?
> 
> Btw, are you sure it saves more batteries to close the idle sockets?

Thanks Will for the details and the investigation!

Indeed NetworkChangeNotifier doesn't fire any event when the app goes to
background. I suspect that the platform will emit a 'no connection' event (that
the Android NetworkChangeNotifier will catch) when the phone switches to
'hibernate' mode. But we still want to call OnSuspend() in case the user
backgrounded Chrome and uses GMail actively.
If you think that URLRequestJob::OnSuspend() covers us (I think it should) then
I'm totally fine with reverting my changes in http_network_layer.cc and removing
PowerObserver from its parent classes (as well as On{Resume,Suspend}()). The
next patch set will do that.

I don't have much background on the battery stats. I'm just the poor guy who was
unlucky enough to notice that lines 40-41 in process_utils.cc got lost during
upstreaming (see https://chromiumcodereview.appspot.com/11411315) :). Adding
them back resulted in massive crashes that I didn't manage to reproduce
unfortunately.
So I noticed that there was some platform independent code (depending on power
events) already closing the idle connections. This was a good opportunity to
remove our crashy Android specific code (process_utils.cc) which seems to be
wrong. By the way you might find something terribly wrong from the threading
perspective by looking at it :).

I think the initial motivation was to be aligned with what the Android stock
browser does. I heard that they did some experiments at that time and that
closing these idle connections had a big impact. I'm CCing some guys who will be
able to tell us more about this.

Similarly we also close the JavaScript timers when the app goes to background.

[Philippe, 2012-12-12 18:03:16]

On 2012/12/12 17:59:31, Philippe wrote:
> On 2012/12/12 17:30:06, willchan wrote:
> > The change in system_monitor_android.cc lgtm.
> > 
> > I am concerned about the change in net/. Your change looks correct, but it
> begs
> > the question...how did this work before? I know this had to have worked at
> some
> > point in time, since rvargas@ explicitly wrote this to work around bugs in
the
> > Windows XP kernel leading to BSOD when sockets were still alive on suspend,
so
> > we had to close the idle ones when we suspended.
> > 
> > I did some archaeology and it looks like I regressed the behavior a year and
a
> > half ago! https://codereview.chromium.org/7043007
> >
>
https://codereview.chromium.org/7043007/diff/17003/net/http/http_network_layer.h
> >
>
https://codereview.chromium.org/7043007/diff/17003/net/http/http_network_laye...
> > 
> > Good ol' willchan, always breaking shit. But that sort of means we don't
need
> > this suspend/resume crap, and that everything's covered by the PowerObserver
> in
> > URLRequestJob already. I have to wonder if even that's necessary! It's also
> very
> > likely the case that it's unnecessary due to our NetworkChangeNotifier which
> > flushes the socket pools on network changes, which are also triggered when
> > suspend/resume happens (at least on desktop). When your app gets
backgrounded,
> I
> > assume NetworkChangeNotifier doesn't fire any events, right?
> > 
> > Btw, are you sure it saves more batteries to close the idle sockets?
> 
> Thanks Will for the details and the investigation!
> 
> Indeed NetworkChangeNotifier doesn't fire any event when the app goes to
> background. I suspect that the platform will emit a 'no connection' event
(that
> the Android NetworkChangeNotifier will catch) when the phone switches to
> 'hibernate' mode. But we still want to call OnSuspend() in case the user
> backgrounded Chrome and uses GMail actively.
> If you think that URLRequestJob::OnSuspend() covers us (I think it should)
then
> I'm totally fine with reverting my changes in http_network_layer.cc and
removing
> PowerObserver from its parent classes (as well as On{Resume,Suspend}()). The
> next patch set will do that.
> 
> I don't have much background on the battery stats. I'm just the poor guy who
was
> unlucky enough to notice that lines 40-41 in process_utils.cc got lost during
> upstreaming (see https://chromiumcodereview.appspot.com/11411315) :). Adding
> them back resulted in massive crashes that I didn't manage to reproduce
> unfortunately.
> So I noticed that there was some platform independent code (depending on power
> events) already closing the idle connections. This was a good opportunity to
> remove our crashy Android specific code (process_utils.cc) which seems to be
> wrong. By the way you might find something terribly wrong from the threading
> perspective by looking at it :).
> 
> I think the initial motivation was to be aligned with what the Android stock
> browser does. I heard that they did some experiments at that time and that
> closing these idle connections had a big impact. I'm CCing some guys who will
be
> able to tell us more about this.
> 
> Similarly we also close the JavaScript timers when the app goes to background.

I can also make NetworkChangeNotifierAndroid subscribe to power suspend events
and fire a 'no connection' type event when this happens to be aligned with
desktop (and same thing for the power resume case).

What do you think?

[Philippe, 2012-12-12 18:19:53]

On 2012/12/12 18:03:16, Philippe wrote:
> On 2012/12/12 17:59:31, Philippe wrote:
> > On 2012/12/12 17:30:06, willchan wrote:
> > > The change in system_monitor_android.cc lgtm.
> > > 
> > > I am concerned about the change in net/. Your change looks correct, but it
> > begs
> > > the question...how did this work before? I know this had to have worked at
> > some
> > > point in time, since rvargas@ explicitly wrote this to work around bugs in
> the
> > > Windows XP kernel leading to BSOD when sockets were still alive on
suspend,
> so
> > > we had to close the idle ones when we suspended.
> > > 
> > > I did some archaeology and it looks like I regressed the behavior a year
and
> a
> > > half ago! https://codereview.chromium.org/7043007
> > >
> >
>
https://codereview.chromium.org/7043007/diff/17003/net/http/http_network_layer.h
> > >
> >
>
https://codereview.chromium.org/7043007/diff/17003/net/http/http_network_laye...
> > > 
> > > Good ol' willchan, always breaking shit. But that sort of means we don't
> need
> > > this suspend/resume crap, and that everything's covered by the
PowerObserver
> > in
> > > URLRequestJob already. I have to wonder if even that's necessary! It's
also
> > very
> > > likely the case that it's unnecessary due to our NetworkChangeNotifier
which
> > > flushes the socket pools on network changes, which are also triggered when
> > > suspend/resume happens (at least on desktop). When your app gets
> backgrounded,
> > I
> > > assume NetworkChangeNotifier doesn't fire any events, right?
> > > 
> > > Btw, are you sure it saves more batteries to close the idle sockets?
> > 
> > Thanks Will for the details and the investigation!
> > 
> > Indeed NetworkChangeNotifier doesn't fire any event when the app goes to
> > background. I suspect that the platform will emit a 'no connection' event
> (that
> > the Android NetworkChangeNotifier will catch) when the phone switches to
> > 'hibernate' mode. But we still want to call OnSuspend() in case the user
> > backgrounded Chrome and uses GMail actively.
> > If you think that URLRequestJob::OnSuspend() covers us (I think it should)
> then
> > I'm totally fine with reverting my changes in http_network_layer.cc and
> removing
> > PowerObserver from its parent classes (as well as On{Resume,Suspend}()). The
> > next patch set will do that.
> > 
> > I don't have much background on the battery stats. I'm just the poor guy who
> was
> > unlucky enough to notice that lines 40-41 in process_utils.cc got lost
during
> > upstreaming (see https://chromiumcodereview.appspot.com/11411315) :). Adding
> > them back resulted in massive crashes that I didn't manage to reproduce
> > unfortunately.
> > So I noticed that there was some platform independent code (depending on
power
> > events) already closing the idle connections. This was a good opportunity to
> > remove our crashy Android specific code (process_utils.cc) which seems to be
> > wrong. By the way you might find something terribly wrong from the threading
> > perspective by looking at it :).
> > 
> > I think the initial motivation was to be aligned with what the Android stock
> > browser does. I heard that they did some experiments at that time and that
> > closing these idle connections had a big impact. I'm CCing some guys who
will
> be
> > able to tell us more about this.
> > 
> > Similarly we also close the JavaScript timers when the app goes to
background.
> 
> I can also make NetworkChangeNotifierAndroid subscribe to power suspend events
> and fire a 'no connection' type event when this happens to be aligned with
> desktop (and same thing for the power resume case).
> 
> What do you think?

I reverted the changes in http_network_layer.cc. I will do a separate CL
cleaning it up (and possibly doing what I mentioned before about making the
NetworkChangeNotifierAndroid subscribe to power events).

[willchan no longer on Chromium, 2012-12-12 18:24:13]

Perfect, thanks. Please send the follow up changes to the power and network
change notifications in net/ to szym@ & pauljensen@. I've pointed them to this
thread so they can read the history and figure out the right path forward.

[sgurun-gerrit only, 2012-12-12 19:01:58]

I guess you mean the idle socket changes I did almost a year ago. I think these
were to defer closing the idle sockets so that network interface would not be
activated just for the purpose of sending a FIN which would waste a lot of power
(somebody had stats I guess, I don't know who though).  

When tcp keepalive is off, I am not sure why or how much power would be wasted
for keeping the idle sockets open. In fact explicitly closing them would turn on
the interface and waste more power, am I missing anything?


On 2012/12/12 18:24:13, willchan wrote:
> Perfect, thanks. Please send the follow up changes to the power and network
> change notifications in net/ to szym@ & pauljensen@. I've pointed them to this
> thread so they can read the history and figure out the right path forward.

[Philippe, 2012-12-12 20:25:08]

On 2012/12/12 19:01:58, sgurun wrote:
> I guess you mean the idle socket changes I did almost a year ago. I think
these
> were to defer closing the idle sockets so that network interface would not be
> activated just for the purpose of sending a FIN which would waste a lot of
power
> (somebody had stats I guess, I don't know who though).  
> 
> When tcp keepalive is off, I am not sure why or how much power would be wasted
> for keeping the idle sockets open. In fact explicitly closing them would turn
on
> the interface and waste more power, am I missing anything?
> 
> 
> On 2012/12/12 18:24:13, willchan wrote:
> > Perfect, thanks. Please send the follow up changes to the power and network
> > change notifications in net/ to szym@ & pauljensen@. I've pointed them to
this
> > thread so they can read the history and figure out the right path forward.

You made a good point, thanks for bringing this up!

I don't have much background with all this as I said before but I think that
these subtleties make one more reason to stay away from an Android specific
implementation and use instead the same mechanism as other platforms (that can
be improved too).

To be honest I'm more interested in fixing the crash right now caused by
process_utils.cc (ReleaseBlock dev bug whose fix has to land before Friday EOD).
This CL should guarantee that and should also align us at least partially with
all platforms regarding the power events handled  by URLRequestJob.

I will do a follow up change anyway that can be discussed without this deadline
pressure.

I will submit this CL in a few hours (during the morning in Paris where I'm
working from).

[willchan no longer on Chromium, 2012-12-12 20:34:12]

SGTM!


On Wed, Dec 12, 2012 at 12:25 PM, <pliard@chromium.org> wrote:

> On 2012/12/12 19:01:58, sgurun wrote:
>
>> I guess you mean the idle socket changes I did almost a year ago. I think
>>
> these
>
>> were to defer closing the idle sockets so that network interface would
>> not be
>> activated just for the purpose of sending a FIN which would waste a lot of
>>
> power
>
>> (somebody had stats I guess, I don't know who though).
>>
>
Ah yes, https://code.google.com/p/chromium/issues/detail?id=101820. This
means that even when we call Close() on the sockets, it won't actually
close the OS handle. And I believe this is a better approach than waking up
the radio to send a FIN, *unless* we believe the server(s) will send
data/FINs that will wake us up repeatedly later.


>
>  When tcp keepalive is off, I am not sure why or how much power would be
>> wasted
>> for keeping the idle sockets open. In fact explicitly closing them would
>> turn
>>
> on
>
>> the interface and waste more power, am I missing anything?
>>
>
Nope, you're not missing anything unless the server is sending TCP packets,
as I've mentioned above. I do not believe we need to make changes to
forcibly close these sockets rather than delaying FINs.


>
>
>  On 2012/12/12 18:24:13, willchan wrote:
>> > Perfect, thanks. Please send the follow up changes to the power and
>> network
>> > change notifications in net/ to szym@ & pauljensen@. I've pointed them
>> to
>>
> this
>
>> > thread so they can read the history and figure out the right path
>> forward.
>>
>
> You made a good point, thanks for bringing this up!
>
> I don't have much background with all this as I said before but I think
> that
> these subtleties make one more reason to stay away from an Android specific
> implementation and use instead the same mechanism as other platforms (that
> can
> be improved too).
>
> To be honest I'm more interested in fixing the crash right now caused by
> process_utils.cc (ReleaseBlock dev bug whose fix has to land before Friday
> EOD).
> This CL should guarantee that and should also align us at least partially
> with
> all platforms regarding the power events handled  by URLRequestJob.
>
> I will do a follow up change anyway that can be discussed without this
> deadline
> pressure.
>
> I will submit this CL in a few hours (during the morning in Paris where I'm
> working from).
>
>
https://chromiumcodereview.**appspot.com/11538008/<https://chromiumcodereview...
>