Initiate WebRTC event logs uploads on appropriate networks only

Avoid initiating the upload of WebRTC event logs when either:
* Not connected to any network.
* Connected to any network other than WiFi or Ethernet.

Note that this is not bullet-proof, since a machine may be
connected to multiple networks at the same time, but
NetworkConnectionTracker only reports one of those.
Also, if connected to a mobile hotspot, this wi will appear
to us as WiFi.

Bug: 775415
Change-Id: I44ed2560903751c27c1b1fe98a66ea89bd463a3e
Reviewed-on: https://chromium-review.googlesource.com/1122123Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Elad Alon <eladalon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571924}

chrome/browser/media/webrtc/webrtc_event_log_manager.cc

@@ -89,7 +89,7 @@ WebRtcEventLogManager::WebRtcEventLogManager()
       remote_logs_observer_(nullptr),
       local_logs_manager_(this),
       pc_tracker_proxy_(new PeerConnectionTrackerProxyImpl),
-      url_request_context_getter_was_set_(false),
+      first_browser_context_initializations_done_(false),
       task_runner_(base::CreateSequencedTaskRunnerWithTraits(
           {base::MayBlock(), base::TaskPriority::BACKGROUND,
            base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})) {
@@ -121,29 +121,41 @@ void WebRtcEventLogManager::EnableForBrowserContext(
   DCHECK(browser_context);
   CHECK(!browser_context->IsOffTheRecord());
 
-  // system_request_context() not available during instantiation; we get it
-  // when the first profile is loaded, which is also the earliest time when
-  // it could be needed.
+  // network_connection_tracker() and system_request_context() are not available
+  // during instantiation; we get them when the first profile is loaded, which
+  // is also the earliest time when they could be needed.
+  content::NetworkConnectionTracker* network_connection_tracker;
   net::URLRequestContextGetter* url_request_context_getter;
-  if (remote_logs_manager_ && !url_request_context_getter_was_set_) {
+  if (remote_logs_manager_ && !first_browser_context_initializations_done_) {
+    network_connection_tracker =
+        g_browser_process->network_connection_tracker();
+    DCHECK(network_connection_tracker);
+
     url_request_context_getter = g_browser_process->system_request_context();
     DCHECK(url_request_context_getter);
-    url_request_context_getter_was_set_ = true;
+
+    first_browser_context_initializations_done_ = true;
   } else {
+    network_connection_tracker = nullptr;
     url_request_context_getter = nullptr;
   }
 
-  // The object is destroyed by ~BrowserProcessImpl(), so base::Unretained(this)
-  // will not be dereferenced after destruction.
-  // |url_request_context_getter| is owned by IOThread. The internal task runner
-  // that uses it (|task_runner_|) stops before IOThread dies, so we can trust
-  // that |url_request_context_getter| will not be used after destruction.
+  // * The object is destroyed by ~BrowserProcessImpl(), so
+  //   base::Unretained(this) will not be dereferenced after destruction.
+  // * |url_request_context_getter| is owned by IOThread. The internal task
+  //   runner that uses it (|task_runner_|) stops before IOThread dies, so we
+  //   can trust that |url_request_context_getter| will not be used after
+  //   destruction.
+  // * |network_connection_tracker| is owned by BrowserProcessImpl, which
+  //   owns the IOThread, so the logic explaining why using base::Unretained
+  //   was safe for with |url_request_context_getter|, also applies to it.
   task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(
           &WebRtcEventLogManager::EnableForBrowserContextInternal,
           base::Unretained(this), GetBrowserContextId(browser_context),
           browser_context->GetPath(),
+          base::Unretained(network_connection_tracker),
           base::Unretained(url_request_context_getter), std::move(reply)));
 }
 
@@ -504,15 +516,22 @@ void WebRtcEventLogManager::OnLoggingTargetStopped(LoggingTarget target,
 void WebRtcEventLogManager::EnableForBrowserContextInternal(
     BrowserContextId browser_context_id,
     const base::FilePath& browser_context_dir,
+    content::NetworkConnectionTracker* network_connection_tracker,
     net::URLRequestContextGetter* context_getter,
     base::OnceClosure reply) {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
   DCHECK_NE(browser_context_id, kNullBrowserContextId);
 
   if (remote_logs_manager_) {
+    if (network_connection_tracker) {
+      remote_logs_manager_->SetNetworkConnectionTracker(
+          network_connection_tracker);
+    }
+
     if (context_getter) {
       remote_logs_manager_->SetUrlRequestContextGetter(context_getter);
     }
+
     remote_logs_manager_->EnableForBrowserContext(browser_context_id,
                                                   browser_context_dir);
   }
@@ -673,13 +692,13 @@ void WebRtcEventLogManager::SetRemoteLogsObserverInternal(
 void WebRtcEventLogManager::SetClockForTesting(base::Clock* clock,
                                                base::OnceClosure reply) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(reply);
 
   auto task = [](WebRtcEventLogManager* manager, base::Clock* clock,
                  base::OnceClosure reply) {
     manager->local_logs_manager_.SetClockForTesting(clock);
-    if (reply) {
-      BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, std::move(reply));
-    }
+
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, std::move(reply));
   };
 
   // The object is destroyed by ~BrowserProcessImpl(), so base::Unretained(this)
@@ -692,14 +711,14 @@ void WebRtcEventLogManager::SetPeerConnectionTrackerProxyForTesting(
     std::unique_ptr<PeerConnectionTrackerProxy> pc_tracker_proxy,
     base::OnceClosure reply) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(reply);
 
   auto task = [](WebRtcEventLogManager* manager,
                  std::unique_ptr<PeerConnectionTrackerProxy> pc_tracker_proxy,
                  base::OnceClosure reply) {
     manager->pc_tracker_proxy_ = std::move(pc_tracker_proxy);
-    if (reply) {
-      BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, std::move(reply));
-    }
+
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, std::move(reply));
   };
 
   // The object is destroyed by ~BrowserProcessImpl(), so base::Unretained(this)
@@ -714,6 +733,7 @@ void WebRtcEventLogManager::SetWebRtcEventLogUploaderFactoryForTesting(
     base::OnceClosure reply) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(remote_logs_manager_);  // The test would otherwise be meaningless.
+  DCHECK(reply);
 
   auto task =
       [](WebRtcEventLogManager* manager,
@@ -722,10 +742,8 @@ void WebRtcEventLogManager::SetWebRtcEventLogUploaderFactoryForTesting(
         auto* remote_logs_manager = manager->remote_logs_manager_.get();
         remote_logs_manager->SetWebRtcEventLogUploaderFactoryForTesting(
             std::move(uploader_factory));
-        if (reply) {
-          BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                                  std::move(reply));
-        }
+
+        BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, std::move(reply));
       };
 
   // The object is destroyed by ~BrowserProcessImpl(), so base::Unretained(this)

chrome/browser/media/webrtc/webrtc_event_log_manager.h

@@ -25,6 +25,7 @@
 
 namespace content {
 class BrowserContext;
+class NetworkConnectionTracker;
 };
 
 // This is a singleton class running in the browser UI thread (ownership of
@@ -197,6 +198,7 @@ class WebRtcEventLogManager final : public content::RenderProcessHostObserver,
   void EnableForBrowserContextInternal(
       BrowserContextId browser_context_id,
       const base::FilePath& browser_context_dir,
+      content::NetworkConnectionTracker* network_connection_tracker,
       net::URLRequestContextGetter* context_getter,
       base::OnceClosure reply);
   void DisableForBrowserContextInternal(BrowserContextId browser_context_id,
@@ -303,10 +305,10 @@ class WebRtcEventLogManager final : public content::RenderProcessHostObserver,
   // peer connection. In (relevant) unit tests, a mock will be injected.
   std::unique_ptr<PeerConnectionTrackerProxy> pc_tracker_proxy_;
 
-  // The global system_request_context() is sent down to |remote_logs_manager_|
-  // with the first enabled browser context.
+  // The globals network_connection_tracker() and system_request_context() are
+  // sent down to |remote_logs_manager_| with the first enabled browser context.
   // This member must only be accessed on the UI thread.
-  bool url_request_context_getter_was_set_;
+  bool first_browser_context_initializations_done_;
 
   // The main logic will run sequentially on this runner, on which blocking
   // tasks are allowed.

chrome/browser/media/webrtc/webrtc_event_log_manager_remote.cc

@@ -90,6 +90,20 @@ std::string CreateLogId() {
   DCHECK_EQ(log_id.find_first_not_of("0123456789ABCDEF"), std::string::npos);
   return log_id;
 }
+
+// Do not attempt to upload when there is no active connection.
+// Do not attempt to upload if the connection is known to be a mobile one.
+// Err on the side of caution with unknown connection types (by not uploading).
+// Note #1: A device may have multiple connections, so this is not bullet-proof.
+// Note #2: Does not attempt to recognize mobile hotspots.
+bool UploadSupportedUsingConnectionType(
+    network::mojom::ConnectionType connection) {
+  if (connection == network::mojom::ConnectionType::CONNECTION_ETHERNET ||
+      connection == network::mojom::ConnectionType::CONNECTION_WIFI) {
+    return true;
+  }
+  return false;
+}
 }  // namespace
 
 const size_t kMaxActiveRemoteBoundWebRtcEventLogs = 3;
@@ -113,7 +127,9 @@ WebRtcRemoteEventLogManager::WebRtcRemoteEventLogManager(
               ::switches::kWebRtcRemoteEventLogUploadNoSuppression)),
       proactive_prune_scheduling_delta_(GetProactivePruningDelta()),
       proactive_prune_scheduling_started_(false),
-      observer_(observer) {
+      observer_(observer),
+      network_connection_tracker_(nullptr),
+      uploading_supported_for_connection_type_(false) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DETACH_FROM_SEQUENCE(io_task_sequence_checker_);
   // Proactive pruning would not do anything at the moment; it will be started
@@ -126,11 +142,46 @@ WebRtcRemoteEventLogManager::~WebRtcRemoteEventLogManager() {
   // TODO(crbug.com/775415): Purge from disk files which were being uploaded
   // while destruction took place, thereby avoiding endless attempts to upload
   // the same file.
+
+  // |network_connection_tracker_| might already have posted a task back to us,
+  // but it will not run, because |io_task_sequence_checker_|'s TaskRunner has
+  // already been stopped.
+  network_connection_tracker_->RemoveNetworkConnectionObserver(this);
+}
+
+void WebRtcRemoteEventLogManager::SetNetworkConnectionTracker(
+    content::NetworkConnectionTracker* network_connection_tracker) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(io_task_sequence_checker_);
+  DCHECK(network_connection_tracker);
+  DCHECK(!network_connection_tracker_);
+
+  // |this| is only destroyed (on the UI thread) after the TaskRunner to which
+  // |io_task_sequence_checker_| is bound stops, so both base::Unretained(this)
+  // and AddNetworkConnectionObserver() are safe.
+
+  network_connection_tracker_ = network_connection_tracker;
+  network_connection_tracker_->AddNetworkConnectionObserver(this);
+
+  auto callback =
+      base::BindOnce(&WebRtcRemoteEventLogManager::OnConnectionChanged,
+                     base::Unretained(this));
+  network::mojom::ConnectionType connection_type;
+  const bool sync_answer = network_connection_tracker_->GetConnectionType(
+      &connection_type, std::move(callback));
+
+  if (sync_answer) {
+    OnConnectionChanged(connection_type);
+  }
+
+  // Because this happens while enabling the first browser context, there is no
+  // necessity to consider uploading yet.
+  DCHECK_EQ(enabled_browser_contexts_.size(), 0u);
 }
 
 void WebRtcRemoteEventLogManager::SetUrlRequestContextGetter(
     net::URLRequestContextGetter* context_getter) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(io_task_sequence_checker_);
+  DCHECK(context_getter);
   DCHECK(!uploader_factory_);
   uploader_factory_ =
       std::make_unique<WebRtcEventLogUploaderImpl::Factory>(context_getter);
@@ -345,6 +396,24 @@ void WebRtcRemoteEventLogManager::RenderProcessHostExitedDestroyed(
   MaybeStartUploading();
 }
 
+// TODO(crbug.com/775415): Fix the issue where OnConnectionChanged sometimes
+// experiences toggling behavior shortly after a network change, by delaying
+// potential uploads until a few seconds of stable connection.
+void WebRtcRemoteEventLogManager::OnConnectionChanged(
+    network::mojom::ConnectionType type) {
+  const bool supported_before = uploading_supported_for_connection_type_;
+
+  uploading_supported_for_connection_type_ =
+      UploadSupportedUsingConnectionType(type);
+
+  if (uploading_supported_for_connection_type_ && !supported_before) {
+    MaybeStartUploading();
+  }
+
+  // TODO(crbug.com/775415): Support pausing uploads when connection goes down,
+  // or switches to an unsupported connection type.
+}
+
 void WebRtcRemoteEventLogManager::OnWebRtcEventLogUploadComplete(
     const base::FilePath& file_path,
     bool upload_successful) {
@@ -353,6 +422,9 @@ void WebRtcRemoteEventLogManager::OnWebRtcEventLogUploadComplete(
   // Post a task to deallocate the uploader (can't do this directly,
   // because this function is a callback from the uploader), potentially
   // starting a new upload for the next file.
+  // |this| is only destroyed (on the UI thread) after the TaskRunner to which
+  // |io_task_sequence_checker_| is bound stops, so base::Unretained(this) is
+  // safe.
   base::SequencedTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::BindOnce(
@@ -515,6 +587,9 @@ void WebRtcRemoteEventLogManager::RecurringPendingLogsPrune() {
 
   PrunePendingLogs();
 
+  // |this| is only destroyed (on the UI thread) after the TaskRunner to which
+  // |io_task_sequence_checker_| is bound stops, so base::Unretained(this) is
+  // safe.
   base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE,
       base::BindOnce(&WebRtcRemoteEventLogManager::RecurringPendingLogsPrune,
@@ -623,6 +698,9 @@ bool WebRtcRemoteEventLogManager::AdditionalActiveLogAllowed(
 
 bool WebRtcRemoteEventLogManager::UploadingAllowed() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(io_task_sequence_checker_);
+  if (!uploading_supported_for_connection_type_) {
+    return false;
+  }
   return upload_suppression_disabled_ || active_peer_connections_.empty();
 }
 

chrome/browser/media/webrtc/webrtc_event_log_manager_remote.h

@@ -14,6 +14,7 @@
 #include "base/time/time.h"
 #include "chrome/browser/media/webrtc/webrtc_event_log_manager_common.h"
 #include "chrome/browser/media/webrtc/webrtc_event_log_uploader.h"
+#include "content/public/browser/network_connection_tracker.h"
 
 // TODO(crbug.com/775415): Avoid uploading logs when Chrome shutdown imminent.
 
@@ -22,7 +23,8 @@ class URLRequestContextGetter;
 }  // namespace net
 
 class WebRtcRemoteEventLogManager final
-    : public WebRtcEventLogUploaderObserver {
+    : public content::NetworkConnectionTracker::NetworkConnectionObserver,
+      public WebRtcEventLogUploaderObserver {
   using BrowserContextId = WebRtcEventLogPeerConnectionKey::BrowserContextId;
   using LogFilesMap = std::map<WebRtcEventLogPeerConnectionKey, LogFile>;
   using PeerConnectionKey = WebRtcEventLogPeerConnectionKey;
@@ -31,6 +33,13 @@ class WebRtcRemoteEventLogManager final
   explicit WebRtcRemoteEventLogManager(WebRtcRemoteEventLogsObserver* observer);
   ~WebRtcRemoteEventLogManager() override;
 
+  // Sets a content::NetworkConnectionTracker which will be used to track
+  // network connectivity.
+  // Must not be called more than once.
+  // Must be called before any call to EnableForBrowserContext().
+  void SetNetworkConnectionTracker(
+      content::NetworkConnectionTracker* network_connection_tracker);
+
   // Sets a net::URLRequestContextGetter which will be used for uploads.
   // Must not be called more than once.
   // Must be called before any call to EnableForBrowserContext().
@@ -120,6 +129,9 @@ class WebRtcRemoteEventLogManager final
   // were associated with the renderer process.
   void RenderProcessHostExitedDestroyed(int render_process_id);
 
+  // content::NetworkConnectionTracker::NetworkConnectionObserver implementation
+  void OnConnectionChanged(network::mojom::ConnectionType type) override;
+
   // WebRtcEventLogUploaderObserver implementation.
   void OnWebRtcEventLogUploadComplete(const base::FilePath& file_path,
                                       bool upload_successful) override;
@@ -310,6 +322,13 @@ class WebRtcRemoteEventLogManager final
   // currently busy uploading it to a remote server.
   std::unique_ptr<WebRtcEventLogUploader> uploader_;
 
+  // Provides notifications of network changes.
+  content::NetworkConnectionTracker* network_connection_tracker_;
+
+  // Whether the network we are currently connected to, if any, is one over
+  // which we may upload.
+  bool uploading_supported_for_connection_type_;
+
   // Producer of uploader objects. (In unit tests, this would create
   // null-implementation uploaders, or uploaders whose behavior is controlled
   // by the unit test.)