cros: Move GetCurrentNetworkId call off IO thread.

Make DataReductionProxyConfig/NetworkQualityEstimator call
net::GetWifiSSID() on a worker thread instead of the IO thread
on ChromeOS as a work around for https://crbug.com/821607.

This CL does not solve the underlying problem that is still being
investigated. It gives the user a crippled system instead of a dead
one with a frozen screen.

Bug: 821607
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I8e4db5091e8b080ed2bd7f9bc4a3e04b6e6e8018
Reviewed-on: https://chromium-review.googlesource.com/1020297Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552828}

chrome/browser/profiles/profile_impl_io_data.cc

@@ -39,6 +39,7 @@
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "components/cookie_config/cookie_store_util.h"
+#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h"
 #include "components/data_reduction_proxy/core/browser/data_store_impl.h"
@@ -192,6 +193,21 @@ void ProfileImplIOData::Handle::Init(
           g_browser_process->io_thread()->net_log(), profile_->GetPrefs(),
           BrowserThread::GetTaskRunnerForThread(BrowserThread::IO),
           BrowserThread::GetTaskRunnerForThread(BrowserThread::UI)));
+
+#if defined(OS_CHROMEOS)
+  // Set a task runner for the get network id call in DataReductionProxyConfig
+  // to work around the bug that recv() in AddressTrackerLinux blocks IO thread
+  // and freezes the screen. Using SingleThreadTaskRunner so that task scheduler
+  // does not create too many worker threads when https://crbug.com/821607
+  // happens.
+  // TODO(https://crbug.com/821607): Remove after the bug is resolved.
+  io_data_->data_reduction_proxy_io_data()
+      ->config()
+      ->set_get_network_id_task_runner(
+          base::CreateSingleThreadTaskRunnerWithTraits(
+              {base::MayBlock(), base::TaskPriority::BACKGROUND,
+               base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}));
+#endif
 }
 
 content::ResourceContext*

components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc

@@ -24,6 +24,7 @@
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/task_runner_util.h"
 #include "base/time/default_tick_clock.h"
 #include "build/build_config.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator.h"
@@ -119,6 +120,52 @@ void RecordWarmupURLFetchAttemptEvent(
                             WarmupURLFetchAttemptEvent::kCount);
 }
 
+std::string DoGetCurrentNetworkID() {
+  // It is possible that the connection type changed between when
+  // GetConnectionType() was called and when the API to determine the
+  // network name was called. Check if that happened and retry until the
+  // connection type stabilizes. This is an imperfect solution but should
+  // capture majority of cases, and should not significantly affect estimates
+  // (that are approximate to begin with).
+
+  while (true) {
+    net::NetworkChangeNotifier::ConnectionType connection_type =
+        net::NetworkChangeNotifier::GetConnectionType();
+    std::string ssid_mccmnc;
+
+    switch (connection_type) {
+      case net::NetworkChangeNotifier::ConnectionType::CONNECTION_UNKNOWN:
+      case net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE:
+      case net::NetworkChangeNotifier::ConnectionType::CONNECTION_BLUETOOTH:
+      case net::NetworkChangeNotifier::ConnectionType::CONNECTION_ETHERNET:
+        break;
+      case net::NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI:
+#if defined(OS_ANDROID) || defined(OS_LINUX) || defined(OS_WIN)
+        ssid_mccmnc = net::GetWifiSSID();
+#endif
+        break;
+      case net::NetworkChangeNotifier::ConnectionType::CONNECTION_2G:
+      case net::NetworkChangeNotifier::ConnectionType::CONNECTION_3G:
+      case net::NetworkChangeNotifier::ConnectionType::CONNECTION_4G:
+#if defined(OS_ANDROID)
+        ssid_mccmnc = net::android::GetTelephonyNetworkOperator();
+#endif
+        break;
+    }
+
+    if (connection_type == net::NetworkChangeNotifier::GetConnectionType()) {
+      if (connection_type >= net::NetworkChangeNotifier::CONNECTION_2G &&
+          connection_type <= net::NetworkChangeNotifier::CONNECTION_4G) {
+        // No need to differentiate cellular connections by the exact
+        // connection type.
+        return "cell," + ssid_mccmnc;
+      }
+      return base::IntToString(connection_type) + "," + ssid_mccmnc;
+    }
+  }
+  NOTREACHED();
+}
+
 }  // namespace
 
 namespace data_reduction_proxy {
@@ -627,7 +674,22 @@ void DataReductionProxyConfig::OnNetworkChanged(
 
   connection_type_ = type;
   RecordNetworkChangeEvent(NETWORK_CHANGED);
-  network_properties_manager_->OnChangeInNetworkID(GetCurrentNetworkID());
+
+  if (!get_network_id_task_runner_) {
+    ContinueNetworkChanged(GetCurrentNetworkID());
+    return;
+  }
+
+  base::PostTaskAndReplyWithResult(
+      get_network_id_task_runner_.get(), FROM_HERE,
+      base::BindOnce(&DoGetCurrentNetworkID),
+      base::BindOnce(&DataReductionProxyConfig::ContinueNetworkChanged,
+                     weak_factory_.GetWeakPtr()));
+}
+
+void DataReductionProxyConfig::ContinueNetworkChanged(
+    const std::string& network_id) {
+  network_properties_manager_->OnChangeInNetworkID(network_id);
 
   ReloadConfig();
 
@@ -804,50 +866,7 @@ DataReductionProxyConfig::GetProxiesForHttp() const {
 
 std::string DataReductionProxyConfig::GetCurrentNetworkID() const {
   DCHECK(thread_checker_.CalledOnValidThread());
-
-  // It is possible that the connection type changed between when
-  // GetConnectionType() was called and when the API to determine the
-  // network name was called. Check if that happened and retry until the
-  // connection type stabilizes. This is an imperfect solution but should
-  // capture majority of cases, and should not significantly affect estimates
-  // (that are approximate to begin with).
-
-  while (true) {
-    net::NetworkChangeNotifier::ConnectionType connection_type =
-        net::NetworkChangeNotifier::GetConnectionType();
-    std::string ssid_mccmnc;
-
-    switch (connection_type) {
-      case net::NetworkChangeNotifier::ConnectionType::CONNECTION_UNKNOWN:
-      case net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE:
-      case net::NetworkChangeNotifier::ConnectionType::CONNECTION_BLUETOOTH:
-      case net::NetworkChangeNotifier::ConnectionType::CONNECTION_ETHERNET:
-        break;
-      case net::NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI:
-#if defined(OS_ANDROID) || defined(OS_LINUX) || defined(OS_WIN)
-        ssid_mccmnc = net::GetWifiSSID();
-#endif
-        break;
-      case net::NetworkChangeNotifier::ConnectionType::CONNECTION_2G:
-      case net::NetworkChangeNotifier::ConnectionType::CONNECTION_3G:
-      case net::NetworkChangeNotifier::ConnectionType::CONNECTION_4G:
-#if defined(OS_ANDROID)
-        ssid_mccmnc = net::android::GetTelephonyNetworkOperator();
-#endif
-        break;
-    }
-
-    if (connection_type == net::NetworkChangeNotifier::GetConnectionType()) {
-      if (connection_type >= net::NetworkChangeNotifier::CONNECTION_2G &&
-          connection_type <= net::NetworkChangeNotifier::CONNECTION_4G) {
-        // No need to differentiate cellular connections by the exact
-        // connection type.
-        return "cell," + ssid_mccmnc;
-      }
-      return base::IntToString(connection_type) + "," + ssid_mccmnc;
-    }
-  }
-  NOTREACHED();
+  return DoGetCurrentNetworkID();
 }
 
 const NetworkPropertiesManager&

components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h

@@ -206,6 +206,11 @@ class DataReductionProxyConfig
       std::pair<bool /* is_secure_proxy */, bool /*is_core_proxy */>>
   GetInFlightWarmupProxyDetails() const;
 
+  void set_get_network_id_task_runner(
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
+    get_network_id_task_runner_ = task_runner;
+  }
+
  protected:
   virtual base::TimeTicks GetTicksNow() const;
 
@@ -272,6 +277,13 @@ class DataReductionProxyConfig
   void OnNetworkChanged(
       net::NetworkChangeNotifier::ConnectionType type) override;
 
+  // Invoked to continue network changed handling after the network id is
+  // retrieved. If |get_network_id_task_runner_| is set, the network id is
+  // fetched on the worker thread. Otherwise, OnNetworkChanged calls this
+  // directly. This is a workaround for https://crbug.com/821607 where
+  // net::GetWifiSSID() call gets stuck.
+  void ContinueNetworkChanged(const std::string& network_id);
+
   // Requests the secure proxy check URL. Upon completion, returns the results
   // to the caller via the |fetcher_callback|. Virtualized for unit testing.
   virtual void SecureProxyCheck(SecureProxyCheckerCallback fetcher_callback);
@@ -335,6 +347,9 @@ class DataReductionProxyConfig
 
   scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
 
+  // Optional task runner for GetCurrentNetworkID.
+  scoped_refptr<base::SingleThreadTaskRunner> get_network_id_task_runner_;
+
   // The caller must ensure that the |net_log_|, if set, outlives this instance.
   // It is used to create new instances of |net_log_with_source_| on secure
   // proxy checks. |net_log_with_source_| permits the correlation of the begin

net/nqe/network_quality_estimator.cc

@@ -158,6 +158,46 @@ void RecordEffectiveConnectionTypeAccuracy(
   histogram->Add(std::abs(metric));
 }
 
+nqe::internal::NetworkID DoGetCurrentNetworkID() {
+  // It is possible that the connection type changed between when
+  // GetConnectionType() was called and when the API to determine the
+  // network name was called. Check if that happened and retry until the
+  // connection type stabilizes. This is an imperfect solution but should
+  // capture majority of cases, and should not significantly affect estimates
+  // (that are approximate to begin with).
+  while (true) {
+    nqe::internal::NetworkID network_id(
+        NetworkChangeNotifier::GetConnectionType(), std::string(), INT32_MIN);
+
+    switch (network_id.type) {
+      case NetworkChangeNotifier::ConnectionType::CONNECTION_UNKNOWN:
+      case NetworkChangeNotifier::ConnectionType::CONNECTION_NONE:
+      case NetworkChangeNotifier::ConnectionType::CONNECTION_BLUETOOTH:
+      case NetworkChangeNotifier::ConnectionType::CONNECTION_ETHERNET:
+        break;
+      case NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI:
+#if defined(OS_ANDROID) || defined(OS_LINUX) || defined(OS_WIN)
+        network_id.id = GetWifiSSID();
+#endif
+        break;
+      case NetworkChangeNotifier::ConnectionType::CONNECTION_2G:
+      case NetworkChangeNotifier::ConnectionType::CONNECTION_3G:
+      case NetworkChangeNotifier::ConnectionType::CONNECTION_4G:
+#if defined(OS_ANDROID)
+        network_id.id = android::GetTelephonyNetworkOperator();
+#endif
+        break;
+      default:
+        NOTREACHED() << "Unexpected connection type = " << network_id.type;
+        break;
+    }
+
+    if (network_id.type == NetworkChangeNotifier::GetConnectionType())
+      return network_id;
+  }
+  NOTREACHED();
+}
+
 }  // namespace
 
 NetworkQualityEstimator::NetworkQualityEstimator(
@@ -662,8 +702,36 @@ void NetworkQualityEstimator::OnConnectionTypeChanged(
 
 void NetworkQualityEstimator::GatherEstimatesForNextConnectionType() {
   DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (!get_network_id_task_runner_) {
+    ContinueGatherEstimatesForNextConnectionType(GetCurrentNetworkID());
+    return;
+  }
+
+  // Doing PostTaskAndReplyWithResult by handle because it requires the result
+  // type have a default constructor and nqe::internal::NetworkID does not have
+  // that.
+  get_network_id_task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          [](scoped_refptr<base::TaskRunner> reply_task_runner,
+             base::OnceCallback<void(const nqe::internal::NetworkID&)>
+                 reply_callback) {
+            reply_task_runner->PostTask(
+                FROM_HERE, base::BindOnce(std::move(reply_callback),
+                                          DoGetCurrentNetworkID()));
+          },
+          base::ThreadTaskRunnerHandle::Get(),
+          base::BindOnce(&NetworkQualityEstimator::
+                             ContinueGatherEstimatesForNextConnectionType,
+                         weak_ptr_factory_.GetWeakPtr())));
+}
+
+void NetworkQualityEstimator::ContinueGatherEstimatesForNextConnectionType(
+    const nqe::internal::NetworkID& network_id) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   // Update the local state as part of preparation for the new connection.
-  current_network_id_ = GetCurrentNetworkID();
+  current_network_id_ = network_id;
   RecordNetworkIDAvailability();
 
   // Read any cached estimates for the new network. If cached estimates are
@@ -1264,43 +1332,7 @@ nqe::internal::NetworkID NetworkQualityEstimator::GetCurrentNetworkID() const {
   // TODO(tbansal): crbug.com/498068 Add NetworkQualityEstimatorAndroid class
   // that overrides this method on the Android platform.
 
-  // It is possible that the connection type changed between when
-  // GetConnectionType() was called and when the API to determine the
-  // network name was called. Check if that happened and retry until the
-  // connection type stabilizes. This is an imperfect solution but should
-  // capture majority of cases, and should not significantly affect estimates
-  // (that are approximate to begin with).
-  while (true) {
-    nqe::internal::NetworkID network_id(
-        NetworkChangeNotifier::GetConnectionType(), std::string(), INT32_MIN);
-
-    switch (network_id.type) {
-      case NetworkChangeNotifier::ConnectionType::CONNECTION_UNKNOWN:
-      case NetworkChangeNotifier::ConnectionType::CONNECTION_NONE:
-      case NetworkChangeNotifier::ConnectionType::CONNECTION_BLUETOOTH:
-      case NetworkChangeNotifier::ConnectionType::CONNECTION_ETHERNET:
-        break;
-      case NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI:
-#if defined(OS_ANDROID) || defined(OS_LINUX) || defined(OS_WIN)
-        network_id.id = GetWifiSSID();
-#endif
-        break;
-      case NetworkChangeNotifier::ConnectionType::CONNECTION_2G:
-      case NetworkChangeNotifier::ConnectionType::CONNECTION_3G:
-      case NetworkChangeNotifier::ConnectionType::CONNECTION_4G:
-#if defined(OS_ANDROID)
-        network_id.id = android::GetTelephonyNetworkOperator();
-#endif
-        break;
-      default:
-        NOTREACHED() << "Unexpected connection type = " << network_id.type;
-        break;
-    }
-
-    if (network_id.type == NetworkChangeNotifier::GetConnectionType())
-      return network_id;
-  }
-  NOTREACHED();
+  return DoGetCurrentNetworkID();
 }
 
 bool NetworkQualityEstimator::ReadCachedNetworkQualityEstimate() {

net/nqe/network_quality_estimator.h

@@ -41,6 +41,7 @@
 
 namespace base {
 class TickClock;
+class TaskRunner;
 }  // namespace base
 
 namespace net {
@@ -216,6 +217,11 @@ class NET_EXPORT NetworkQualityEstimator
   typedef nqe::internal::Observation Observation;
   typedef nqe::internal::ObservationBuffer ObservationBuffer;
 
+  void set_get_network_id_task_runner(
+      scoped_refptr<base::TaskRunner> task_runner) {
+    get_network_id_task_runner_ = task_runner;
+  }
+
  protected:
   // NetworkChangeNotifier::ConnectionTypeObserver implementation:
   void OnConnectionTypeChanged(
@@ -492,6 +498,14 @@ class NET_EXPORT NetworkQualityEstimator
   // in the connection type.
   void GatherEstimatesForNextConnectionType();
 
+  // Invoked to continue GatherEstimatesForNextConnectionType work after getting
+  // network id. If |get_network_id_task_runner_| is set, the network id is
+  // fetched on a worker thread. Otherwise, GatherEstimatesForNextConnectionType
+  // calls this directly. This is a workaround for https://crbug.com/821607
+  // where net::GetWifiSSID() call gets stuck.
+  void ContinueGatherEstimatesForNextConnectionType(
+      const nqe::internal::NetworkID& network_id);
+
   // Updates the value of |cached_estimate_applied_| if |observation| is
   // computed from a cached estimate. |buffer| is the observation buffer to
   // which the cached estimate is being added to.
@@ -622,6 +636,9 @@ class NET_EXPORT NetworkQualityEstimator
   // Time when the last RTT observation from a socket watcher was received.
   base::TimeTicks last_socket_watcher_rtt_notification_;
 
+  // Optional task runner to get network id.
+  scoped_refptr<base::TaskRunner> get_network_id_task_runner_;
+
   base::WeakPtrFactory<NetworkQualityEstimator> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkQualityEstimator);

services/network/network_service.cc

@@ -11,6 +11,7 @@
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial_params.h"
+#include "base/task_scheduler/post_task.h"
 #include "build/build_config.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 #include "net/base/logging_network_change_observer.h"
@@ -134,6 +135,19 @@ NetworkService::NetworkService(
           network_quality_estimator_params),
       net_log_);
 
+#if defined(OS_CHROMEOS)
+  // Set a task runner for the get network id call for NetworkQualityEstimator
+  // to workaround https://crbug.com/821607 where AddressTrackerLinux stucks
+  // with a recv() call and blocks IO thread. Using SingleThreadTaskRunner so
+  // that task scheduler does not create too many worker threads when the
+  // problem happens.
+  // TODO(https://crbug.com/821607): Remove after the bug is resolved.
+  network_quality_estimator_->set_get_network_id_task_runner(
+      base::CreateSingleThreadTaskRunnerWithTraits(
+          {base::MayBlock(), base::TaskPriority::BACKGROUND,
+           base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}));
+#endif
+
   host_resolver_ = CreateHostResolver();
 }