GCM: D-Bus methods for wake-on-packet

GCM: D-Bus methods for wake-on-packet

Program the NIC to wake-on-packet for packets arriving from
the GCM server.  Chrome notifies the connection manager (shill)
by calling D-Bus methods.  The return status indicates if the
operation succeeded or not (depending on hardware support and
the type of connection).

This is a resubmission of a previous patch with the same title
and functionality containing fixes and a workaround for a problem
that is independent of this patch.  Bug 415821 remains open
for that problem.

Keywords: wowlan, wake-on-lan, wake-on-wlan

BUG=360295
TEST=verified dbus methods are called, and packets are sent on the
TEST=connection passed to the methods.  Also verified that shill
TEST=programs the NIC correctly, and packets wake up the device.

Committed: https://crrev.com/e2b442be9c01e033aebd119ee49acd9cac83ee34
Cr-Commit-Position: refs/heads/master@{#296516}

[Luigi Semenzato, 2014-09-24 01:19:33]

semenzato@chromium.org changed reviewers:
+ derat@chromium.org, fgorski@chromium.org, scheib@chromium.org,
stevenjb@chromium.org, yoz@chromium.org, zea@chromium.org

[Luigi Semenzato, 2014-09-24 01:30:35]

This is a resubmission of https://codereview.chromium.org/409883006/ with some
fixes.

The root problem that caused an intermittent failure in the BVTs is not fixed,
but there is a workaround (check for null IP address in
shill_manager_client.cc).  I reproduced the failure with the old CL, and this CL
does not reproduce it.  (Failure rate with old CL: about 20%.  New CL: no
failures in 20 runs.)

I have opened crbug.com/415821 for the root problem but I wish to be excused
from fixing it.  It is not in my code (and I am by far not the best person to
fix it) and unfortunately I have more urgent work elsewhere.

I have also fixed a possible failure in ShutDown() in gcm_profile_service.cc,
although that never got a chance to hit.  The old CL dereferenced driver_ before
checking if it was NULL.

Thanks!

[Daniel Erat, 2014-09-24 14:40:53]

could you put a diff between the old change and this one somewhere for all of
the reviewers who already lgtm-ed the old one?

[Daniel Erat, 2014-09-24 14:41:25]

On 2014/09/24 14:40:53, Daniel Erat wrote:
> could you put a diff between the old change and this one somewhere for all of
> the reviewers who already lgtm-ed the old one?

ugh. that last comment was not in fact meant to be an el-gee-tee-em. :-/

[fgorski, 2014-09-24 16:50:47]

lgtm

[scheib, 2014-09-24 17:05:14]

chrome/browser/extensions/extension_gcm_app_handler_unittest.cc lgtm, comment
improvement suggested.

https://codereview.chromium.org/597683005/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_gcm_app_handler_unittest.cc (right):

https://codereview.chromium.org/597683005/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_gcm_app_handler_unittest.cc:224: // Create a
DBus thread manager setter for its side effect.
Would be best to explicitly state the side effect intended. Is this "Initialize
main DBusThreadManager for testing." ?

[Luigi Semenzato, 2014-09-24 17:08:22]

A diff of diffs is correct but messy.  Let me summarize first.

file gcm_profile_service.cc:
line 176, changed #ifdef CHROMEOS to #if defined(OS_CHROMEOS)
line 217, moved the OS_CHROMEOS-specific code inside "if (driver_)"

file shill_manager_client.cc:
added "if (ip_endpoint.address().size() == 0) ..." guards in
AddWakeOnPacketConnection and RemoveWakeOnPacketConnection.

No other changes.  Complete diff of diffs follows.  Thanks!

semenzato@luigi:~/Downloads$ diff x.diff y.diff
3c3
< index
9a36d6060dfe4348fb4216fd4649fba8a2e26d34..f4a3c86675a157ca127ae355e0a04f72d6f31b38
100644
---
> index
8def8dc64576da9958a334a5d219da51c031c6ac..a8520b615e26d3e89153c409a4bcd7c81c20bc4f
100644
6c6
< @@ -440,7 +440,7 @@ deps_os = {
---
> @@ -432,7 +432,7 @@ deps_os = {
135c135
< index
0539acc212bc5dcee3ad535f239acc9b35705254..c0fc8529a9bf134541b7d173242c18d1c8cdf896
100644
---
> index
0223cbeef2b1e2e72ffe9dce4f48c987ce5821b8..0de6385d77b6fe0d6f1fa820c404316e224569b5
100644
148c148
< @@ -167,6 +170,11 @@ GCMProfileService::GCMProfileService(
---
> @@ -170,6 +173,11 @@ GCMProfileService::GCMProfileService(
152c152
< +#ifdef CHROMEOS
---
> +#if defined(OS_CHROMEOS)
160c160
< @@ -202,6 +210,10 @@ void GCMProfileService::Shutdown() {
---
> @@ -205,8 +213,11 @@ void GCMProfileService::Shutdown() {
163a164,165
> -
>    if (driver_) {
165,166c167,168
< +  driver_->RemoveConnectionObserver(chromeos_connection_observer_.get());
< +  chromeos_connection_observer_.reset();
---
> +    driver_->RemoveConnectionObserver(chromeos_connection_observer_.get());
> +    chromeos_connection_observer_.reset();
168,169d169
<  
<    if (driver_) {
170a171,172
>      driver_.reset();
>    }
215c217
< index
e48b74ed8d7cd372442ffb7059d08a67dbe93e5e..f4325f33dbaa02472edef6c32faf8aba30422f25
100644
---
> index
b9ba1b851096b16a2b16c3729d6c6ff9cc557ce9..7520ef3e83bbe10ba55678618c55da7e4d1d5406
100644
218c220
< @@ -2660,6 +2660,8 @@
---
> @@ -2645,6 +2645,8 @@
229c231
< index
d57917ff3307f4992047f73928d4da48a2a39b7a..934985e6f9b4dc08a860e7ce1223ebfa4419652a
100644
---
> index
2ef8327d0f07f07938ce013adbe012d8bfc41dc1..8e22da38bdbadd1bcfae2d51e52e073db56d9cd2
100644
324c326
< index
0ad602f22bbb52656ea07c47ee1b11451972b10f..d4c00195de85d950dad651e72c780bd371cf2ff8
100644
---
> index
0ad602f22bbb52656ea07c47ee1b11451972b10f..f03ee38f38c406c80433c45c04e9a6286c78309f
100644
335c337
< @@ -210,6 +211,43 @@ class ShillManagerClientImpl : public ShillManagerClient
{
---
> @@ -210,6 +211,51 @@ class ShillManagerClientImpl : public ShillManagerClient
{
342a345,348
> +    if (ip_endpoint.address().size() == 0) {
> +      LOG(ERROR) << "AddWakeOnPacketConnection: null address";
> +      return;
> +    }
355a362,365
> +    if (ip_endpoint.address().size() == 0) {
> +      LOG(ERROR) << "RemoveWakeOnPacketConnection: null address";
> +      return;
> +    }

[Daniel Erat, 2014-09-24 17:22:57]

thanks, lgtm.

(for future reference, one way to make this simpler is to upload the original
revert of the revert as the first revision of your patch and then upload the
updated version as the second revision; then the "Delta from patch set" links
can be used to see the fixes.)

https://codereview.chromium.org/597683005/diff/1/chromeos/dbus/shill_manager_...
File chromeos/dbus/shill_manager_client.cc (right):

https://codereview.chromium.org/597683005/diff/1/chromeos/dbus/shill_manager_...
chromeos/dbus/shill_manager_client.cc:218: if (ip_endpoint.address().size() ==
0) {
nit: ip_endpoint.address().empty()

https://codereview.chromium.org/597683005/diff/1/chromeos/dbus/shill_manager_...
chromeos/dbus/shill_manager_client.cc:235: if (ip_endpoint.address().size() ==
0) {
nit: ip_endpoint.address().empty()

[Luigi Semenzato, 2014-09-24 17:58:30]

The CQ bit was checked by semenzato@chromium.org

[commit-bot: I haz the power, 2014-09-24 17:59:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/597683005/20001

[commit-bot: I haz the power, 2014-09-24 18:02:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-24 18:02:14]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/60168)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/65060)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Luigi Semenzato, 2014-09-24 20:19:29]

DEPS merge conflict resolved.

[Luigi Semenzato, 2014-09-24 20:19:33]

The CQ bit was checked by semenzato@chromium.org

[commit-bot: I haz the power, 2014-09-24 20:20:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/597683005/40001

[commit-bot: I haz the power, 2014-09-24 21:06:24]

Committed patchset #3 (id:40001) as fd725573ccf444e9000a70163e02cfa9ed2e5915

[commit-bot: I haz the power, 2014-09-24 21:07:00]

Patchset 3 (id:??) landed as
https://crrev.com/e2b442be9c01e033aebd119ee49acd9cac83ee34
Cr-Commit-Position: refs/heads/master@{#296516}