More details: Tests usually fail with one of the error log below:
Log Type 1:
```
[2380:2393:0906/111643.935965:ERROR:network_service.cc(84)] Not implemented reached in std::unique_ptr<net::NetworkChangeNotifier> network::(anonymous namespace)::CreateNetworkChangeNotifierIfNeeded()
[2300:2300:0906/111643.975157:INFO:remote_commands_service.cc(38)] Fetching remote commands.
[2300:2300:0906/111643.992077:WARNING:remote_commands_service.cc(40)] Client is not registered.
[2300:2300:0906/111643.993726:INFO:remote_commands_invalidator.cc(32)] Initialize RemoteCommandsInvalidator.
[2300:2300:0906/111643.993776:INFO:remote_commands_invalidator.cc(57)] Starting RemoteCommandsInvalidator.
[2300:2300:0906/111643.993796:INFO:remote_commands_invalidator.cc(123)] RemoteCommandsInvalidator ReloadPolicyData.
[2300:2300:0906/111643.993830:INFO:remote_commands_invalidator.cc(167)] Unregister RemoteCommandsInvalidator.
[2300:2300:0906/111644.109479:ERROR:network_profile_handler.cc(83)] Manager properties returned from Shill don't contain the field Profiles
[2300:2300:0906/111644.109558:ERROR:network_sms_handler.cc(436)] NetworkSmsHandler: No list value for: Devices
[2300:2300:0906/111644.221522:ERROR:content_gpu_interface_provider.cc(83)] Not implemented reached in virtual void ash::ContentGpuInterfaceProvider::RegisterOzoneGpuInterfaces(service_manager::BinderRegistry *)
[2300:2300:0906/111644.241151:WARNING:shelf_button.cc(381)] An icon of size 32x32is being scaled up and will look blurry.
[2300:2300:0906/111644.321393:ERROR:arc_active_directory_enrollment_token_fetcher.cc(161)] Fetching an enrollment token failed. DM Status: 5
```
Log Type 2:
```
[7107:7120:0906/111709.536661:ERROR:network_service.cc(84)] Not implemented reached in std::unique_ptr<net::NetworkChangeNotifier> network::(anonymous namespace)::CreateNetworkChangeNotifierIfNeeded()
[7074:7074:0906/111709.562506:INFO:remote_commands_service.cc(38)] Fetching remote commands.
[7074:7074:0906/111709.579961:WARNING:remote_commands_service.cc(40)] Client is not registered.
[7074:7074:0906/111709.580093:INFO:remote_commands_invalidator.cc(32)] Initialize RemoteCommandsInvalidator.
[7074:7074:0906/111709.581531:INFO:remote_commands_invalidator.cc(57)] Starting RemoteCommandsInvalidator.
[7074:7074:0906/111709.581578:INFO:remote_commands_invalidator.cc(123)] RemoteCommandsInvalidator ReloadPolicyData.
[7074:7074:0906/111709.581598:INFO:remote_commands_invalidator.cc(167)] Unregister RemoteCommandsInvalidator.
[7074:7074:0906/111709.704561:ERROR:network_type_pattern.cc(134)] NetworkTypePattern: wifi: Can not match empty type.
[7074:7074:0906/111709.705999:ERROR:network_type_pattern.cc(134)] NetworkTypePattern: wifi: Can not match empty type.
[7074:7074:0906/111709.706009:ERROR:network_type_pattern.cc(134)] NetworkTypePattern: wifi: Can not match empty type.
[7074:7074:0906/111709.706058:ERROR:network_type_pattern.cc(134)] NetworkTypePattern: wifi: Can not match empty type.
[7074:7074:0906/111709.706064:ERROR:network_type_pattern.cc(134)] NetworkTypePattern: wifi: Can not match empty type.
[7074:7074:0906/111709.706070:ERROR:network_type_pattern.cc(134)] NetworkTypePattern: wifi: Can not match empty type.
[7074:7074:0906/111709.706097:ERROR:network_type_pattern.cc(134)] NetworkTypePattern: wifi: Can not match empty type.
[7074:7074:0906/111709.706103:ERROR:network_type_pattern.cc(134)] NetworkTypePattern: wifi: Can not match empty type.
[7074:7074:0906/111709.706109:ERROR:network_type_pattern.cc(134)] NetworkTypePattern: wifi: Can not match empty type.
[7074:7074:0906/111709.706169:ERROR:network_type_pattern.cc(134)] NetworkTypePattern: wifi: Can not match empty type.
[7074:7074:0906/111709.706176:ERROR:network_type_pattern.cc(134)] NetworkTypePattern: wifi: Can not match empty type.
[7074:7074:0906/111709.706182:ERROR:network_type_pattern.cc(134)] NetworkTypePattern: wifi: Can not match empty type.
[7074:7074:0906/111709.804834:ERROR:content_gpu_interface_provider.cc(83)] Not implemented reached in virtual void ash::ContentGpuInterfaceProvider::RegisterOzoneGpuInterfaces(service_manager::BinderRegistry *)
```
Will investigate whether `ash::ContentGpuInterfaceProvider::RegisterOzoneGpuInterfaces()` has anything to do with the issue.
+stevenjb@ (OWNER of //src/chromeos/network/): Can I have some insights on how I should make |NetworkChangeNotifier| work with Network Service on ChromeOS?
My naive thinking is we probably want to call |NetworkChangeNotifierFactoryChromeos::CreateInstance()| in |NetworkChangeNotifier::Create()|[1], but my guts tell me it won't be that easy (otherwise it would already be done).
One observation is that we are calling
```
net::NetworkChangeNotifier::SetFactory(
new NetworkChangeNotifierFactoryChromeos());
```
in |ChromeBrowserMainPartsChromeos::PreMainMessageLoopStart()|, which would then be picked up by |NetworkChangeNotifier::Create()| and should explain why it works without Network Service. So I'm also wondering maybe I should do something similarly to override the factory in the Network Service process, assuming there are some timing issues involved?
Thanks!
[1] https://cs.chromium.org/chromium/src/net/base/network_change_notifier.cc?l=206&rcl=a9c4e1d4481e67dbfc550c19b04252a8839fa35d
I'm not super familiar with NetworkChangeNotifier, but unfortunately the original authors have moved on.
As you noted, we override the factory in ChromeBrowserMainPartsChromeos::PreMainMessageLoopStart(). It is unclear to me why that wouldn't get called on browser_tests?
Re #c3: Sorry for the confusion. The background is '/net' code (e.g. |NetworkChangeNotifier::Create()|) would run in a stand along process (Network Service process), thus the override in the browser process won't work.
Sorry to hear that the original authors have moved on. I will do some experiments by myself then, but still it would be really helpful if I could get some input on how Chrome OS is different from other platforms on this issue though (e.g. Why do we have to override the factory instead of returning it directly).
Thanks!
I have not investigated the problem, but there's a whole bunch of parallel network infrastructure used by a bunch of ChromeOS network classes (Not just the NCN). chromeos/network/network_state_handler.* is one of the primary classes of that infrastructure, I believe. Some consumers of that stuff will need to go in the network service, some not, I believe. So it seems like the choices are either to move that stuff into the network service, or add a bunch of APIs so it could live outside the network service, but inform the network service of state changes.
I haven't looked into this (deliberately), so don't really have an opinion on what should be done about this stuff.
I will probably need to take a closer look at NetworkChangeNotifier myself, but scanning through NetworkChangeNotifierChromeos, I see that it is built on top of NetworkStateHandler, which wraps a DBus interface between Chrome and Shill (the connection manager process running on Chrome OS).
The problem is that Shill does not really support multiple clients, so having a separate process implementing NetworkStateHandler is a problem.
There are plans to create a mojo interface providing the functionality of NetworkStateHandler. I think that migrating NCNChromeos to use that interface and moving it to src/net is the correct long term solution.
Unfortunately that effort is not currently at the top of my priority list, as much as I would like it to be. I do hope to complete it by the end of Q4. If that is not soon enough, you may need to create a simpler bridge between Chrome and NetworkService.
Thanks mmenke@ and stevenjb@ for the additional background! It feels like we are having some dependency issues here, will dig into it and see what I can do.
> The problem is that Shill does not really support multiple clients, so having a separate process implementing NetworkStateHandler is a problem.
Just to clarify: Can I assume that Shill handles not only network but also other system functionalities. Which means we cannot move Shill entirely to the network service process since the browser process also has to access it.
> Tracking issue for cros_network_config.mojom: issue 862420
IIUC the plan was to still host network implementation in the browser process, and expose a set of mojo APIs (e.g. To the network service process). It feels to me that we're essentially adding an additional process hop to all network requests, which is no better than hosting network service in-process...
Will look into it anyway to see if I'm missing something.
Shill is a separate process. It's the Chrome OS version of Linux's conman.
NetworkStateHandler is used to configure network connections (by sending DBus messages to Shill) and reflect connection state (e.g. what wifi networks are visible, what networks are connected/connecting, etc). It is separate from the "network service" implementation in src/net.
Currently UI in src/chrome and src/ash is deeply integrated with NetworkStateHandler. Mid-term we are planning to convert src/ash to use the new mojo service and have the Chrome process host the cros_network_config mojo service. In theory we could migrate all of src/chrome to use the mojo service as well and move the service to the NetworkService process, but that would be a lot more work.
Actually the error log in #c1 doesn't seem to cause test failures. Will work on issue 884773 first which seem to be more related, and get back to this one later.
The following revision refers to this bug:
https://chromium.googlesource.com/chromium/src.git/+/31391ba33a8da1d9618fbfdb42aaa911cb986cf3
commit 31391ba33a8da1d9618fbfdb42aaa911cb986cf3
Author: Robbie McElrath <rmcelrath@chromium.org>
Date: Thu Nov 15 02:04:50 2018
Fix race condition in net::SerialWorker.
net::SerialWorker is a RefCountedThreadSafe object that also uses
WeakPtrFactory to create WeakPtrs of itself. To run whatever work it's
supposed to run, it PostTaskWithTraitsAndReply's to another sequence to
do the work there, and then gets called back when the work is complete.
The work callback holds a (refcounted) reference to the SerialWorker,
and the reply callback is bound to a WeakPtr to ensure the SerialWorker
will still be deleted if posting the reply fails. WeakPtr enforces that
all dereferences to it, as well as its invalidation, must happen on the
same sequence to avoid races. When the reply callback is being called,
it dereferences the SerialWorker WeakPtr on the original sequence,
which binds that WeakPtr to that sequence. It's possible for the
SerialWorker’s owner to release it after the WeakPtr has been checked,
but before the worker function has returned, in which case the worker
function will have the last reference to the SerialWorker. In that case,
when the worker function finally does return, it will delete the
SerialWorker, which will delete its WeakPtrFactory, which will
invalidate existing WeakPtrs. However, the WeakPtr was previously
dereferenced on the SerialWorker sequence, and now it’s being
invalidated on the worker sequence, which is invalid.
To fix this, this CL makes SerialWorker a RefCountedDeleteOnSequence to
ensure the deletion always happens in the right sequence.
Bug: 882610
Change-Id: If738d3724384ffd5ac210130415bce7262932feb
Reviewed-on: https://chromium-review.googlesource.com/c/1336066
Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
Reviewed-by: Eric Roman <eroman@chromium.org>
Reviewed-by: Eric Orth <ericorth@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608223}
[modify] https://crrev.com/31391ba33a8da1d9618fbfdb42aaa911cb986cf3/net/dns/dns_config_service_posix_unittest.cc
[modify] https://crrev.com/31391ba33a8da1d9618fbfdb42aaa911cb986cf3/net/dns/serial_worker.cc
[modify] https://crrev.com/31391ba33a8da1d9618fbfdb42aaa911cb986cf3/net/dns/serial_worker.h
Comment 1 by chongz@chromium.org
, Sep 10