[GCM] Add heartbeat manager and reconnection logic due to heartbeat failure

[GCM] Add heartbeat manager and reconnection logic due to heartbeat failure

The heartbeat manager maintains the heartbeat timer, handles heartbeat
interval updates from the server, and automatically triggers connection resets
if the heartbeat isn't properly acknowledged in a timely manner.

Also fixes an issue in reconnection where the client socket handle doesn't deal
well with a connection reset due to passing ownership of the socket.

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

[Nicolas Zea, 2013-12-19 23:27:51]

PTAL

[fgorski, 2013-12-19 23:55:13]

A few comments.

https://codereview.chromium.org/118133003/diff/1/google_apis/gcm/engine/conne...
File google_apis/gcm/engine/connection_handler_impl.cc (left):

https://codereview.chromium.org/118133003/diff/1/google_apis/gcm/engine/conne...
google_apis/gcm/engine/connection_handler_impl.cc:397: write_callback_.Reset();
Did you mean to remove these?

https://codereview.chromium.org/118133003/diff/1/google_apis/gcm/engine/heart...
File google_apis/gcm/engine/heartbeat_manager.cc (right):

https://codereview.chromium.org/118133003/diff/1/google_apis/gcm/engine/heart...
google_apis/gcm/engine/heartbeat_manager.cc:14: const int64
kCellHeartbeatDefaultMs = 1000 * 60 * 15;  // 15 minutes.
Cell should be 28, wifi 15.

https://codereview.chromium.org/118133003/diff/1/google_apis/gcm/engine/heart...
google_apis/gcm/engine/heartbeat_manager.cc:30: const base::Closure&
trigger_reconnect_callback) {
DCHECK both callbacks. You are doing that in OnHeartbeatAcked, you can do it
much earlier here.

https://codereview.chromium.org/118133003/diff/1/google_apis/gcm/engine/heart...
google_apis/gcm/engine/heartbeat_manager.cc:70: return base::TimeTicks();
Based on the documentation of the method it can be interpreted as HB Ack is due,
even though the time is not running. Can that backfire?

https://codereview.chromium.org/118133003/diff/1/google_apis/gcm/engine/heart...
google_apis/gcm/engine/heartbeat_manager.cc:100: // For unknown connections, use
the shorter cellular heartbeat interval.
longer :)

https://codereview.chromium.org/118133003/diff/1/google_apis/gcm/engine/mcs_c...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/118133003/diff/1/google_apis/gcm/engine/mcs_c...
google_apis/gcm/engine/mcs_client.cc:30: // TODO(zea): handle these at the
GCMClient layer.
What exactly do you mean?

https://codereview.chromium.org/118133003/diff/1/google_apis/gcm/engine/mcs_c...
google_apis/gcm/engine/mcs_client.cc:444: heartbeat_manager_.OnHeartbeatAcked();
Will that work if the manager hasn't been started yet? as in, if it is a login
response.

[Nicolas Zea, 2013-12-20 22:52:56]

https://codereview.chromium.org/118133003/diff/1/google_apis/gcm/engine/conne...
File google_apis/gcm/engine/connection_handler_impl.cc (left):

https://codereview.chromium.org/118133003/diff/1/google_apis/gcm/engine/conne...
google_apis/gcm/engine/connection_handler_impl.cc:397: write_callback_.Reset();
On 2013/12/19 23:55:13, Filip Gorski wrote:
> Did you mean to remove these?

Yes, otherwise calling Init above a second time DCHECKs.

https://codereview.chromium.org/118133003/diff/1/google_apis/gcm/engine/heart...
File google_apis/gcm/engine/heartbeat_manager.cc (right):

https://codereview.chromium.org/118133003/diff/1/google_apis/gcm/engine/heart...
google_apis/gcm/engine/heartbeat_manager.cc:14: const int64
kCellHeartbeatDefaultMs = 1000 * 60 * 15;  // 15 minutes.
On 2013/12/19 23:55:13, Filip Gorski wrote:
> Cell should be 28, wifi 15. 

Good catch, done.

https://codereview.chromium.org/118133003/diff/1/google_apis/gcm/engine/heart...
google_apis/gcm/engine/heartbeat_manager.cc:30: const base::Closure&
trigger_reconnect_callback) {
On 2013/12/19 23:55:13, Filip Gorski wrote:
> DCHECK both callbacks. You are doing that in OnHeartbeatAcked, you can do it
> much earlier here.

Done.

https://codereview.chromium.org/118133003/diff/1/google_apis/gcm/engine/heart...
google_apis/gcm/engine/heartbeat_manager.cc:70: return base::TimeTicks();
On 2013/12/19 23:55:13, Filip Gorski wrote:
> Based on the documentation of the method it can be interpreted as HB Ack is
due,
> even though the time is not running. Can that backfire?

Updated documentation.

https://codereview.chromium.org/118133003/diff/1/google_apis/gcm/engine/heart...
google_apis/gcm/engine/heartbeat_manager.cc:100: // For unknown connections, use
the shorter cellular heartbeat interval.
On 2013/12/19 23:55:13, Filip Gorski wrote:
> longer :)

Done.

https://codereview.chromium.org/118133003/diff/1/google_apis/gcm/engine/mcs_c...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/118133003/diff/1/google_apis/gcm/engine/mcs_c...
google_apis/gcm/engine/mcs_client.cc:30: // TODO(zea): handle these at the
GCMClient layer.
On 2013/12/19 23:55:13, Filip Gorski wrote:
> What exactly do you mean?

System status settings like these should be handled by the GCMClient, rather
than the MCSClient.

https://codereview.chromium.org/118133003/diff/1/google_apis/gcm/engine/mcs_c...
google_apis/gcm/engine/mcs_client.cc:444: heartbeat_manager_.OnHeartbeatAcked();
On 2013/12/19 23:55:13, Filip Gorski wrote:
> Will that work if the manager hasn't been started yet? as in, if it is a login
> response.

Yep, OnHeartbeatAcked does nothing if not running.

[fgorski, 2013-12-20 23:01:50]

lgtm

[jianli, 2013-12-20 23:10:03]

https://codereview.chromium.org/118133003/diff/20001/google_apis/gcm/engine/h...
File google_apis/gcm/engine/heartbeat_manager.cc (right):

https://codereview.chromium.org/118133003/diff/20001/google_apis/gcm/engine/h...
google_apis/gcm/engine/heartbeat_manager.cc:22: : waiting_for_ack_(false),
nit: alignment

https://codereview.chromium.org/118133003/diff/20001/google_apis/gcm/engine/h...
google_apis/gcm/engine/heartbeat_manager.cc:26:
HeartbeatManager::~HeartbeatManager() {}
nit: empty line before this.

https://codereview.chromium.org/118133003/diff/20001/google_apis/gcm/engine/h...
google_apis/gcm/engine/heartbeat_manager.cc:65: heartbeat_interval_ =
base::TimeDelta::FromMilliseconds(server_interval_ms_);
It seems to be easier to track heartbeat interval in ms and call
base::TimeDelta::FromMilliseconds to convert it when calling Timer::Start.

Also, it seem to be duplicate to have both server_interval_ms_ and
heartbeat_interval_. Can we use a bool flag to indicate the interval is from
server.

Do we need to restart timer here?

https://codereview.chromium.org/118133003/diff/20001/google_apis/gcm/engine/h...
File google_apis/gcm/engine/heartbeat_manager.h (right):

https://codereview.chromium.org/118133003/diff/20001/google_apis/gcm/engine/h...
google_apis/gcm/engine/heartbeat_manager.h:47: // manager is stopped.
What does it mean if the returned value is non-null and not less than
base::Timeticks::Now()?

Also, probably better to say current time, instead of base::Timeticks::Now().

https://codereview.chromium.org/118133003/diff/20001/google_apis/gcm/engine/h...
File google_apis/gcm/engine/heartbeat_manager_unittest.cc (right):

https://codereview.chromium.org/118133003/diff/20001/google_apis/gcm/engine/h...
google_apis/gcm/engine/heartbeat_manager_unittest.cc:40: TestHeartbeatManager*
manager() { return manager_.get(); }
nit: const modifier

[commit-bot: I haz the power, 2013-12-20 23:10:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/118133003/20001

[Nicolas Zea, 2013-12-26 22:55:01]

PTAL

https://codereview.chromium.org/118133003/diff/20001/google_apis/gcm/engine/h...
File google_apis/gcm/engine/heartbeat_manager.cc (right):

https://codereview.chromium.org/118133003/diff/20001/google_apis/gcm/engine/h...
google_apis/gcm/engine/heartbeat_manager.cc:22: : waiting_for_ack_(false),
On 2013/12/20 23:10:03, jianli wrote:
> nit: alignment

Done.

https://codereview.chromium.org/118133003/diff/20001/google_apis/gcm/engine/h...
google_apis/gcm/engine/heartbeat_manager.cc:26:
HeartbeatManager::~HeartbeatManager() {}
On 2013/12/20 23:10:03, jianli wrote:
> nit: empty line before this.

Done.

https://codereview.chromium.org/118133003/diff/20001/google_apis/gcm/engine/h...
google_apis/gcm/engine/heartbeat_manager.cc:65: heartbeat_interval_ =
base::TimeDelta::FromMilliseconds(server_interval_ms_);
On 2013/12/20 23:10:03, jianli wrote:
> It seems to be easier to track heartbeat interval in ms and call
> base::TimeDelta::FromMilliseconds to convert it when calling Timer::Start.
> 
> Also, it seem to be duplicate to have both server_interval_ms_ and
> heartbeat_interval_. Can we use a bool flag to indicate the interval is from
> server.
> 
> Do we need to restart timer here?

Switched to storing as int.

heartbeat_interval is sometimes the ack time (10 seconds), so we need to store
the server interval separately.

https://codereview.chromium.org/118133003/diff/20001/google_apis/gcm/engine/h...
File google_apis/gcm/engine/heartbeat_manager.h (right):

https://codereview.chromium.org/118133003/diff/20001/google_apis/gcm/engine/h...
google_apis/gcm/engine/heartbeat_manager.h:47: // manager is stopped.
On 2013/12/20 23:10:03, jianli wrote:
> What does it mean if the returned value is non-null and not less than
> base::Timeticks::Now()?
> 
> Also, probably better to say current time, instead of base::Timeticks::Now().

That's the time that the heartbeat will be sent. Reworded to make it a bit
clearer.

https://codereview.chromium.org/118133003/diff/20001/google_apis/gcm/engine/h...
File google_apis/gcm/engine/heartbeat_manager_unittest.cc (right):

https://codereview.chromium.org/118133003/diff/20001/google_apis/gcm/engine/h...
google_apis/gcm/engine/heartbeat_manager_unittest.cc:40: TestHeartbeatManager*
manager() { return manager_.get(); }
On 2013/12/20 23:10:03, jianli wrote:
> nit: const modifier

Done.

[fgorski, 2013-12-26 23:04:08]

It seems that you reverted some of the changes by accident.

https://codereview.chromium.org/118133003/diff/180001/google_apis/gcm/engine/...
File google_apis/gcm/engine/heartbeat_manager.cc (right):

https://codereview.chromium.org/118133003/diff/180001/google_apis/gcm/engine/...
google_apis/gcm/engine/heartbeat_manager.cc:16: const int64
kWifiHeartbeatDefaultMs = 1000 * 60 * 28;  // 28 minutes.
It seems that you've reverted some of the changes made in pack 2. 
Was that on purpose?

https://codereview.chromium.org/118133003/diff/180001/google_apis/gcm/engine/...
google_apis/gcm/engine/heartbeat_manager.cc:33: const base::Closure&
trigger_reconnect_callback) {
ditto

[Nicolas Zea, 2013-12-26 23:09:05]

Woops, good catch, apparently I  was checked out an older version. Brought back
the fixes.

https://codereview.chromium.org/118133003/diff/180001/google_apis/gcm/engine/...
File google_apis/gcm/engine/heartbeat_manager.cc (right):

https://codereview.chromium.org/118133003/diff/180001/google_apis/gcm/engine/...
google_apis/gcm/engine/heartbeat_manager.cc:16: const int64
kWifiHeartbeatDefaultMs = 1000 * 60 * 28;  // 28 minutes.
On 2013/12/26 23:04:09, Filip Gorski wrote:
> It seems that you've reverted some of the changes made in pack 2. 
> Was that on purpose?

Done.

https://codereview.chromium.org/118133003/diff/180001/google_apis/gcm/engine/...
google_apis/gcm/engine/heartbeat_manager.cc:33: const base::Closure&
trigger_reconnect_callback) {
On 2013/12/26 23:04:09, Filip Gorski wrote:
> ditto

Done.

[jianli, 2014-01-02 20:03:36]

lgtm

[commit-bot: I haz the power, 2014-01-02 20:07:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/118133003/270001

[commit-bot: I haz the power, 2014-01-02 23:24:13]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-02 23:25:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/118133003/270001

[commit-bot: I haz the power, 2014-01-03 00:53:35]

Change committed as 242848