Defer NetworkConfigurationHandler::CreateShillConfiguration callback

Defer NetworkConfigurationHandler::CreateShillConfiguration callback

Currently the code calls the NetworkConfigurationHandler
::CreateShillConfiguration callback immediately, which is only
valid if Shill updates Chrome with the new network configuration
in the same frame as the configuration was applied. At some point
the Shill behavior Chrome, breaking the (fragile) Chrome network
cofiguration layer. This CL defers the configuration success
callback until the NetworkStateHandler has been updated with
the new configuration (allowing a subsequent connection attempt
to succeed).

This CL also replaces SupportsWeakPtr with the more reliable WeakPtrFactory (apologies for the churn).


BUG=662571
Review-Url: https://codereview.chromium.org/2679423008
Cr-Commit-Position: refs/heads/master@{#449787}
Committed: https://chromium.googlesource.com/chromium/src/+/4be001fc71af06d07cb05c30e4b0665efadefed5

[stevenjb, 2017-02-10 00:46:18]

Description was changed from

==========
Defer NetworkConfigurationHandler::CreateShillConfiguration callback

Currently the code calls the NetworkConfigurationHandler
::CreateShillConfiguration callback immediately, which is only
valid if Shill updates Chrome with the new network configuration
in the same frame as the configuration was applied. At some point
the Shill behavior Chrome, breaking the (fragile) Chrome network
cofiguration layer. This CL defers the configuration success
callback until the NetworkStateHandler has been updated with
the new configuration (allowing a subsequent connection attempt
to succeed).

BUG=662571
==========

to

==========
Defer NetworkConfigurationHandler::CreateShillConfiguration callback

Currently the code calls the NetworkConfigurationHandler
::CreateShillConfiguration callback immediately, which is only
valid if Shill updates Chrome with the new network configuration
in the same frame as the configuration was applied. At some point
the Shill behavior Chrome, breaking the (fragile) Chrome network
cofiguration layer. This CL defers the configuration success
callback until the NetworkStateHandler has been updated with
the new configuration (allowing a subsequent connection attempt
to succeed).

This CL also replaces SupportsWeakPtr with the more reliable WeakPtrFactory
(apologies for the churn).


BUG=662571
==========

[stevenjb, 2017-02-10 00:46:29]

stevenjb@chromium.org changed reviewers:
+ tbarzic@chromium.org

[tbarzic, 2017-02-10 19:20:05]

https://codereview.chromium.org/2679423008/diff/1/chromeos/network/network_co...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/2679423008/diff/1/chromeos/network/network_co...
chromeos/network/network_configuration_handler.cc:412:
network_state_handler_->AddObserver(this, FROM_HERE);
the observer is never removed - and NetworkStateHandler observer list has
|check_empty| set.

https://codereview.chromium.org/2679423008/diff/1/chromeos/network/network_co...
chromeos/network/network_configuration_handler.cc:422:
network_state_handler_->RequestUpdateForNetwork(service_path.value());
this is to ensure NetworkListChanged is emitted (and only after network state
properties were updated), right? I'd add a comment about that.

https://codereview.chromium.org/2679423008/diff/1/chromeos/network/network_co...
chromeos/network/network_configuration_handler.cc:436: // should be unique so
can not already exist in |configure_callbacks_}.
Service path is unique per GUID, right?

Could we end up in situation where there are two configuration requests for a
network at the same time?
e.g. if CreateShillConfiguration is called for the second time before the first
call's callback is run? 

+nit:
s/can not/it cannot/

https://codereview.chromium.org/2679423008/diff/1/chromeos/network/network_co...
File chromeos/network/network_configuration_handler.h (right):

https://codereview.chromium.org/2679423008/diff/1/chromeos/network/network_co...
chromeos/network/network_configuration_handler.h:145: // Called when a
configuration completes. This will wait for the cahced
typo: cahced

https://codereview.chromium.org/2679423008/diff/1/chromeos/network/network_co...
chromeos/network/network_configuration_handler.h:215: // Map of configuraiton
callbacks to run once the service becomes available
typo: configuraiton

[stevenjb, 2017-02-10 21:04:11]

The CQ bit was checked by stevenjb@chromium.org to run a CQ dry run

[stevenjb, 2017-02-10 21:04:18]

https://codereview.chromium.org/2679423008/diff/1/chromeos/network/network_co...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/2679423008/diff/1/chromeos/network/network_co...
chromeos/network/network_configuration_handler.cc:412:
network_state_handler_->AddObserver(this, FROM_HERE);
On 2017/02/10 19:20:04, tbarzic wrote:
> the observer is never removed - and NetworkStateHandler observer list has
> |check_empty| set.

Doh. Good catch, thanks. Done.

https://codereview.chromium.org/2679423008/diff/1/chromeos/network/network_co...
chromeos/network/network_configuration_handler.cc:422:
network_state_handler_->RequestUpdateForNetwork(service_path.value());
On 2017/02/10 19:20:04, tbarzic wrote:
> this is to ensure NetworkListChanged is emitted (and only after network state
> properties were updated), right? I'd add a comment about that.

Kind of. Clarified the comment.

https://codereview.chromium.org/2679423008/diff/1/chromeos/network/network_co...
chromeos/network/network_configuration_handler.cc:436: // should be unique so
can not already exist in |configure_callbacks_}.
On 2017/02/10 19:20:04, tbarzic wrote:
> Service path is unique per GUID, right?
> 
> Could we end up in situation where there are two configuration requests for a
> network at the same time?
> e.g. if CreateShillConfiguration is called for the second time before the
first
> call's callback is run? 
> 
> +nit:
> s/can not/it cannot/
> 

While it may be theoretically possible for two updates to the same configuration
to occur, it's an extremely unlikely edge case, and we probably do not want the
first callback to occur in that case, but should not DCHECK.

Done/clarified and removed the DCHECK.

https://codereview.chromium.org/2679423008/diff/1/chromeos/network/network_co...
File chromeos/network/network_configuration_handler.h (right):

https://codereview.chromium.org/2679423008/diff/1/chromeos/network/network_co...
chromeos/network/network_configuration_handler.h:145: // Called when a
configuration completes. This will wait for the cahced
On 2017/02/10 19:20:04, tbarzic wrote:
> typo: cahced

Thanks, sorry done.

https://codereview.chromium.org/2679423008/diff/1/chromeos/network/network_co...
chromeos/network/network_configuration_handler.h:215: // Map of configuraiton
callbacks to run once the service becomes available
On 2017/02/10 19:20:04, tbarzic wrote:
> typo: configuraiton

Done.

[commit-bot: I haz the power, 2017-02-10 21:04:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbarzic, 2017-02-10 21:09:54]

https://codereview.chromium.org/2679423008/diff/20001/chromeos/network/networ...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/2679423008/diff/20001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:404:
network_state_handler_->RemoveObserver(this, FROM_HERE);
this should probably happen in NetworkStateHandlerObserver::OnShuttingDown (so
there's no dependency on destruction order in NetworkHandler)

[stevenjb, 2017-02-10 22:02:08]

https://codereview.chromium.org/2679423008/diff/20001/chromeos/network/networ...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/2679423008/diff/20001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:404:
network_state_handler_->RemoveObserver(this, FROM_HERE);
On 2017/02/10 21:09:54, tbarzic wrote:
> this should probably happen in NetworkStateHandlerObserver::OnShuttingDown (so
> there's no dependency on destruction order in NetworkHandler)

NetworkStateHandler is destroyed last because it has so many dependents, but
that's probably a little safer since it's called before any Network*Handler
destructors. Done.

[tbarzic, 2017-02-10 22:02:51]

lgtm

[stevenjb, 2017-02-10 22:38:40]

Unfortunately moving RemoveObserver to OnShutdown requires fixing a bunch of
tests. PTAL.

[tbarzic, 2017-02-10 22:47:48]

LGTM

https://codereview.chromium.org/2679423008/diff/60001/chromeos/network/auto_c...
File chromeos/network/auto_connect_handler_unittest.cc (right):

https://codereview.chromium.org/2679423008/diff/60001/chromeos/network/auto_c...
chromeos/network/auto_connect_handler_unittest.cc:140:
network_state_handler_->Shutdown();
not strictly part of this cl, but I wonder if we could introduce a test
NetworkHandler instance that sets up and manages test networking handlers.

[stevenjb, 2017-02-10 22:50:20]

https://codereview.chromium.org/2679423008/diff/60001/chromeos/network/auto_c...
File chromeos/network/auto_connect_handler_unittest.cc (right):

https://codereview.chromium.org/2679423008/diff/60001/chromeos/network/auto_c...
chromeos/network/auto_connect_handler_unittest.cc:140:
network_state_handler_->Shutdown();
On 2017/02/10 22:47:48, tbarzic wrote:
> not strictly part of this cl, but I wonder if we could introduce a test
> NetworkHandler instance that sets up and manages test networking handlers.

Yep, we could. There are a lot of them though, so it would need a way to specify
which handlers are needed. *shrug*

[stevenjb, 2017-02-10 22:50:35]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2017-02-10 22:51:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 23:39:14]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486767035803400,
"parent_rev": "8124bd64dc71d518a3fd789cbf4d0c32e1726418", "commit_rev":
"4be001fc71af06d07cb05c30e4b0665efadefed5"}

[commit-bot: I haz the power, 2017-02-10 23:39:54]

Description was changed from

==========
Defer NetworkConfigurationHandler::CreateShillConfiguration callback

Currently the code calls the NetworkConfigurationHandler
::CreateShillConfiguration callback immediately, which is only
valid if Shill updates Chrome with the new network configuration
in the same frame as the configuration was applied. At some point
the Shill behavior Chrome, breaking the (fragile) Chrome network
cofiguration layer. This CL defers the configuration success
callback until the NetworkStateHandler has been updated with
the new configuration (allowing a subsequent connection attempt
to succeed).

This CL also replaces SupportsWeakPtr with the more reliable WeakPtrFactory
(apologies for the churn).


BUG=662571
==========

to

==========
Defer NetworkConfigurationHandler::CreateShillConfiguration callback

Currently the code calls the NetworkConfigurationHandler
::CreateShillConfiguration callback immediately, which is only
valid if Shill updates Chrome with the new network configuration
in the same frame as the configuration was applied. At some point
the Shill behavior Chrome, breaking the (fragile) Chrome network
cofiguration layer. This CL defers the configuration success
callback until the NetworkStateHandler has been updated with
the new configuration (allowing a subsequent connection attempt
to succeed).

This CL also replaces SupportsWeakPtr with the more reliable WeakPtrFactory
(apologies for the churn).


BUG=662571

Review-Url: https://codereview.chromium.org/2679423008
Cr-Commit-Position: refs/heads/master@{#449787}
Committed:
https://chromium.googlesource.com/chromium/src/+/4be001fc71af06d07cb05c30e4b0...
==========

[commit-bot: I haz the power, 2017-02-10 23:39:55]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4be001fc71af06d07cb05c30e4b0...