New issue
Advanced search Search tips

Issue 847422 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

DBusThreadManager::GetSetterForTesting() should not alter the online/offline state of browsertests

Project Member Reported by pmarko@chromium.org, May 29 2018

Issue description

Task:
Ensure that calls to DBusThreadManager::GetSetterForTesting in a browser test's SetUpInProcessBrowserTestFixture() do not have side effects on FakeShillManagerClient's state.

---
Background:
browser_tests tend to call DBusThreadManager::GetSetterForTesting in SetUpInProcessBrowserTestFixture() to install e.g. a fake session manager client or a fake system clock.
(If called in SetUpOnMainThread(), it's too late - CHECK(!g_dbus_thread_manager)[1] fails)

Calling GetSetterForTesting results in the call chain:
- DBusThreadManager::GetSetterForTesting
- DBusThreadManager::InitializeClients
- DBusClientsCommon::Initialize
- FakeShillManagerClient::SetupDefaultEnvironment

However, in SetUpInProcessBrowserTestFixture, there is no ThreadTaskRunnerHandle yet (base::ThreadTaskRunnerHandle::Get() returns nullptr).
This leads to FakeShillManagerClient::SetupDefaultEnvironment skipping its usual setup work[2].
The InitializeClients call will not be repeated - it is skipped in DBusThreadManager::Initialize[3] if GetSetterForTesting was called previously.

This means that FakeShillManagerClient does not have the default "fake networks" set up as a side-effect of calling DBusThreadManager::GetSetterForTesting in SetUpInProcessBrowserTestFixture.

An effect of this is that NetworkChangeNotifier::IsOffline() returns true[4], which makes HostResolverImpl::ProcTask::OnLookupComplete[5] remaps errors on host resolution to ERR_INTERNET_DISCONNECTED[6].
Note that non-localhost URL resolution defaults to returning ERR_NOT_IMPLEMENTED in browsertest due to using TestHostResolver[7] in browsertests.

An effect of this is that device_management_service retries DMServer requests, because IsConnectionError[9] now returns true. This causes some (e.g. FRE) tests to time out.

---
Workaround:
Manually call 
DBusThreadManager::Get()
    ->GetShillManagerClient()
    ->GetTestInterface()
    ->SetupDefaultEnvironment();
e.g. in SetUpOnMainThread().

[1] https://cs.chromium.org/chromium/src/chromeos/dbus/dbus_thread_manager.cc?rcl=06b9fe48cdf3660a7d7124937fee1c7ff026553e&l=317
[2] https://cs.chromium.org/chromium/src/chromeos/dbus/fake_shill_manager_client.cc?rcl=5b0a03c08e1dd3bf9616cb1fe7024ca72f6af258&l=617
[3] https://cs.chromium.org/chromium/src/chromeos/dbus/dbus_thread_manager.cc?rcl=5b0a03c08e1dd3bf9616cb1fe7024ca72f6af258&l=296
[4] https://cs.chromium.org/chromium/src/net/base/network_change_notifier.h?rcl=5b0a03c08e1dd3bf9616cb1fe7024ca72f6af258&l=365
[5] https://cs.chromium.org/chromium/src/net/dns/host_resolver_impl.cc?rcl=5b0a03c08e1dd3bf9616cb1fe7024ca72f6af258&l=816
[6] https://cs.chromium.org/chromium/src/net/dns/host_resolver_impl.cc?rcl=5b0a03c08e1dd3bf9616cb1fe7024ca72f6af258&l=816
[7] https://cs.chromium.org/chromium/src/content/public/test/test_host_resolver.cc?type=cs&sq=package:chromium&g=0&l=64
[8] https://cs.chromium.org/chromium/src/components/policy/core/common/cloud/device_management_service.cc?rcl=5b0a03c08e1dd3bf9616cb1fe7024ca72f6af258&l=82
 

Sign in to add a comment