Update WakeLock policy for RTCPeerConnections.

This CL changes the policy so that the WakeLock is enabled only when an
RTCPeerConnection becomes connected.
Previously the WakeLock was enabled as soon as the RTCPeerConnection was
created. This caused problems for some users, since some sites create
peer connections that are never connected and are only closed when the
peer connection is removed.

Bug: 866200
Change-Id: I47e573f9555960c1a64a0f9834afb9d5dc048a94
Reviewed-on: https://chromium-review.googlesource.com/c/1315210Reviewed-by: Henrik Boström <hbos@chromium.org>
Reviewed-by: Harald Alvestrand <hta@chromium.org>
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608905}

content/browser/webrtc/webrtc_internals.cc

@@ -104,7 +104,7 @@ WebRTCInternals::WebRTCInternals(int aggregate_updates_ms,
           base::CommandLine::ForCurrentProcess()->GetSwitchValuePath(
               switches::kWebRtcLocalEventLogging)),
       event_log_recordings_(false),
-      num_open_connections_(0),
+      num_connected_connections_(0),
       should_block_power_saving_(should_block_power_saving),
       aggregate_updates_ms_(aggregate_updates_ms),
       weak_factory_(this) {
@@ -180,13 +180,12 @@ void WebRTCInternals::OnAddPeerConnection(int render_process_id,
   dict->SetString("constraints", constraints);
   dict->SetString("url", url);
   dict->SetBoolean("isOpen", true);
+  dict->SetBoolean("connected", false);
 
   if (observers_.might_have_observers())
     SendUpdate("addPeerConnection", dict->CreateDeepCopy());
 
   peer_connection_data_.Append(std::move(dict));
-  ++num_open_connections_;
-  UpdateWakeLock();
 
   if (render_process_id_set_.insert(render_process_id).second) {
     RenderProcessHost* host = RenderProcessHost::FromID(render_process_id);
@@ -221,8 +220,16 @@ void WebRTCInternals::OnUpdatePeerConnection(
   if (!record)
     return;
 
-  if (type == "stop")
+  if (type == "iceConnectionStateChange") {
+    if (value == "connected" || value == "checking" || value == "completed") {
+      MaybeMarkPeerConnectionAsConnected(record);
+    } else if (value == "failed" || value == "disconnected" ||
+               value == "closed" || value == "new") {
+      MaybeMarkPeerConnectionAsNotConnected(record);
+    }
+  } else if (type == "stop") {
     MaybeClosePeerConnection(record);
+  }
 
   // Don't update entries if there aren't any observers.
   if (!observers_.might_have_observers())
@@ -556,9 +563,30 @@ void WebRTCInternals::MaybeClosePeerConnection(base::DictionaryValue* record) {
     return;
 
   record->SetBoolean("isOpen", false);
-  --num_open_connections_;
-  DCHECK_GE(num_open_connections_, 0);
-  UpdateWakeLock();
+  MaybeMarkPeerConnectionAsNotConnected(record);
+}
+
+void WebRTCInternals::MaybeMarkPeerConnectionAsConnected(
+    base::DictionaryValue* record) {
+  bool was_connected = true;
+  record->GetBoolean("connected", &was_connected);
+  if (!was_connected) {
+    ++num_connected_connections_;
+    record->SetBoolean("connected", true);
+    UpdateWakeLock();
+  }
+}
+
+void WebRTCInternals::MaybeMarkPeerConnectionAsNotConnected(
+    base::DictionaryValue* record) {
+  bool was_connected = false;
+  record->GetBoolean("connected", &was_connected);
+  if (was_connected) {
+    record->SetBoolean("connected", false);
+    --num_connected_connections_;
+    DCHECK_GE(num_connected_connections_, 0);
+    UpdateWakeLock();
+  }
 }
 
 void WebRTCInternals::UpdateWakeLock() {
@@ -566,12 +594,13 @@ void WebRTCInternals::UpdateWakeLock() {
   if (!should_block_power_saving_)
     return;
 
-  if (num_open_connections_ == 0) {
+  if (num_connected_connections_ == 0) {
     DVLOG(1)
         << ("Cancel the wake lock on application suspension since no "
             "PeerConnections are active anymore.");
     GetWakeLock()->CancelWakeLock();
-  } else if (num_open_connections_ != 0) {
+  } else {
+    DCHECK_GT(num_connected_connections_, 0);
     DVLOG(1) << ("Preventing the application from being suspended while one or "
                  "more PeerConnections are active.");
     GetWakeLock()->RequestWakeLock();

content/browser/webrtc/webrtc_internals.h

@@ -122,7 +122,7 @@ class CONTENT_EXPORT WebRTCInternals : public RenderProcessHostObserver,
   bool IsEventLogRecordingsEnabled() const;
   bool CanToggleEventLogRecordings() const;
 
-  int num_open_connections() const { return num_open_connections_; }
+  int num_connected_connections() const { return num_connected_connections_; }
 
  protected:
   // Constructor/Destructor are protected to allow tests to derive from the
@@ -170,6 +170,9 @@ class CONTENT_EXPORT WebRTCInternals : public RenderProcessHostObserver,
   // is stopped or removed.
   void MaybeClosePeerConnection(base::DictionaryValue* record);
 
+  void MaybeMarkPeerConnectionAsConnected(base::DictionaryValue* record);
+  void MaybeMarkPeerConnectionAsNotConnected(base::DictionaryValue* record);
+
   // Called whenever a PeerConnection is created or stopped in order to
   // request/cancel a wake lock on suspending the current application for power
   // saving.
@@ -235,9 +238,10 @@ class CONTENT_EXPORT WebRTCInternals : public RenderProcessHostObserver,
   bool event_log_recordings_;
   base::FilePath event_log_recordings_file_path_;
 
-  // While |num_open_connections_| is greater than zero, request a wake lock
-  // service. This prevents the application from being suspended while remoting.
-  int num_open_connections_;
+  // While |num_connected_connections_| is greater than zero, request a wake
+  // lock service. This prevents the application from being suspended while
+  // remoting.
+  int num_connected_connections_;
   const bool should_block_power_saving_;
 
   // Set of render process hosts that |this| is registered as an observer on.

content/browser/webrtc/webrtc_internals_unittest.cc

@@ -6,6 +6,7 @@
 
 #include <memory>
 #include <string>
+#include <utility>
 
 #include "base/run_loop.h"
 #include "base/task/post_task.h"
@@ -24,6 +25,10 @@ namespace {
 static const char kContraints[] = "c";
 static const char kRtcConfiguration[] = "r";
 static const char kUrl[] = "u";
+static const char* kWakeLockConnectingValues[] = {"checking", "connected",
+                                                  "completed"};
+static const char* kWakeLockDisconnectingValues[] = {"disconnected", "closed",
+                                                     "failed", "new"};
 
 class MockWebRtcInternalsProxy : public WebRTCInternalsUIObserver {
  public:
@@ -49,7 +54,7 @@ class MockWebRtcInternalsProxy : public WebRTCInternalsUIObserver {
 
 class MockWakeLock : public device::mojom::WakeLock {
  public:
-  MockWakeLock(device::mojom::WakeLockRequest request)
+  explicit MockWakeLock(device::mojom::WakeLockRequest request)
       : binding_(this, std::move(request)), has_wakelock_(false) {}
   ~MockWakeLock() override {}
 
@@ -448,52 +453,247 @@ TEST_F(WebRtcInternalsTest, AudioDebugRecordingsFileSelectionCanceled) {
   base::RunLoop().RunUntilIdle();
 }
 
-TEST_F(WebRtcInternalsTest, WakeLock) {
-  int kRenderProcessId = 1;
-  const int pid = 1;
-  const int lid[] = {1, 2, 3};
+TEST_F(WebRtcInternalsTest, WakeLockCreateRemove) {
+  const int kRenderProcessId = 1;
+  const int kPid = 1;
+  const int kLid = 1;
 
   WebRTCInternalsForTest webrtc_internals;
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
+  EXPECT_FALSE(webrtc_internals.HasWakeLock());
 
-  // Add a few peer connections.
-  EXPECT_EQ(0, webrtc_internals.num_open_connections());
+  webrtc_internals.OnAddPeerConnection(kRenderProcessId, kPid, kLid, kUrl,
+                                       kRtcConfiguration, kContraints);
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
   EXPECT_FALSE(webrtc_internals.HasWakeLock());
-  webrtc_internals.OnAddPeerConnection(kRenderProcessId, pid, lid[0], kUrl,
+
+  webrtc_internals.OnRemovePeerConnection(kPid, kLid);
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
+  EXPECT_FALSE(webrtc_internals.HasWakeLock());
+
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(WebRtcInternalsTest, WakeLockConnecting) {
+  const int kRenderProcessId = 1;
+  const int kPid = 1;
+  const int kLid = 1;
+
+  for (const char* value : kWakeLockConnectingValues) {
+    WebRTCInternalsForTest webrtc_internals;
+    EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
+    EXPECT_FALSE(webrtc_internals.HasWakeLock());
+
+    webrtc_internals.OnAddPeerConnection(kRenderProcessId, kPid, kLid, kUrl,
+                                         kRtcConfiguration, kContraints);
+    EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
+    EXPECT_FALSE(webrtc_internals.HasWakeLock());
+
+    webrtc_internals.OnUpdatePeerConnection(kPid, kLid,
+                                            "iceConnectionStateChange", value);
+    EXPECT_EQ(webrtc_internals.num_connected_connections(), 1);
+    EXPECT_TRUE(webrtc_internals.HasWakeLock());
+
+    webrtc_internals.OnRemovePeerConnection(kPid, kLid);
+    EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
+    EXPECT_FALSE(webrtc_internals.HasWakeLock());
+  }
+
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(WebRtcInternalsTest, WakeLockConnectingSequence) {
+  const int kRenderProcessId = 1;
+  const int kPid = 1;
+  const int kLid = 1;
+
+  WebRTCInternalsForTest webrtc_internals;
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
+  EXPECT_FALSE(webrtc_internals.HasWakeLock());
+
+  webrtc_internals.OnAddPeerConnection(kRenderProcessId, kPid, kLid, kUrl,
                                        kRtcConfiguration, kContraints);
-  EXPECT_EQ(1, webrtc_internals.num_open_connections());
-  EXPECT_TRUE(webrtc_internals.HasWakeLock());
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
+  EXPECT_FALSE(webrtc_internals.HasWakeLock());
 
-  webrtc_internals.OnAddPeerConnection(kRenderProcessId, pid, lid[1], kUrl,
+  // A sequence of connecting messages should not increase the number of
+  // connected connections beyond 1.
+  for (const char* value : kWakeLockConnectingValues) {
+    webrtc_internals.OnUpdatePeerConnection(kPid, kLid,
+                                            "iceConnectionStateChange", value);
+    EXPECT_EQ(webrtc_internals.num_connected_connections(), 1);
+    EXPECT_TRUE(webrtc_internals.HasWakeLock());
+  }
+
+  webrtc_internals.OnRemovePeerConnection(kPid, kLid);
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
+  EXPECT_FALSE(webrtc_internals.HasWakeLock());
+
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(WebRtcInternalsTest, WakeLockDisconnecting) {
+  const int kRenderProcessId = 1;
+  const int kPid = 1;
+  const int kLid = 1;
+
+  for (const char* value : kWakeLockDisconnectingValues) {
+    WebRTCInternalsForTest webrtc_internals;
+    EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
+    EXPECT_FALSE(webrtc_internals.HasWakeLock());
+
+    webrtc_internals.OnAddPeerConnection(kRenderProcessId, kPid, kLid, kUrl,
+                                         kRtcConfiguration, kContraints);
+    EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
+    EXPECT_FALSE(webrtc_internals.HasWakeLock());
+
+    webrtc_internals.OnUpdatePeerConnection(
+        kPid, kLid, "iceConnectionStateChange", "connected");
+    EXPECT_EQ(webrtc_internals.num_connected_connections(), 1);
+    EXPECT_TRUE(webrtc_internals.HasWakeLock());
+
+    webrtc_internals.OnUpdatePeerConnection(kPid, kLid,
+                                            "iceConnectionStateChange", value);
+    EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
+    EXPECT_FALSE(webrtc_internals.HasWakeLock());
+
+    webrtc_internals.OnRemovePeerConnection(kPid, kLid);
+    EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
+    EXPECT_FALSE(webrtc_internals.HasWakeLock());
+  }
+
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(WebRtcInternalsTest, WakeLockDisconnectingSequence) {
+  const int kRenderProcessId = 1;
+  const int kPid = 1;
+  const int kLid = 1;
+
+  WebRTCInternalsForTest webrtc_internals;
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
+  EXPECT_FALSE(webrtc_internals.HasWakeLock());
+
+  webrtc_internals.OnAddPeerConnection(kRenderProcessId, kPid, kLid, kUrl,
                                        kRtcConfiguration, kContraints);
-  EXPECT_EQ(2, webrtc_internals.num_open_connections());
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
+  EXPECT_FALSE(webrtc_internals.HasWakeLock());
+
+  webrtc_internals.OnUpdatePeerConnection(
+      kPid, kLid, "iceConnectionStateChange", "connected");
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 1);
   EXPECT_TRUE(webrtc_internals.HasWakeLock());
 
-  webrtc_internals.OnAddPeerConnection(kRenderProcessId, pid, lid[2], kUrl,
+  // A sequence of disconnecting messages should not decrease the number of
+  // connected connections below zero.
+  for (const char* value : kWakeLockDisconnectingValues) {
+    webrtc_internals.OnUpdatePeerConnection(kPid, kLid,
+                                            "iceConnectionStateChange", value);
+    EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
+    EXPECT_FALSE(webrtc_internals.HasWakeLock());
+  }
+
+  webrtc_internals.OnRemovePeerConnection(kPid, kLid);
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
+  EXPECT_FALSE(webrtc_internals.HasWakeLock());
+
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(WebRtcInternalsTest, WakeLockReconnect) {
+  const int kRenderProcessId = 1;
+  const int kPid = 1;
+  const int kLid = 1;
+
+  WebRTCInternalsForTest webrtc_internals;
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
+  EXPECT_FALSE(webrtc_internals.HasWakeLock());
+
+  webrtc_internals.OnAddPeerConnection(kRenderProcessId, kPid, kLid, kUrl,
                                        kRtcConfiguration, kContraints);
-  EXPECT_EQ(3, webrtc_internals.num_open_connections());
-  EXPECT_TRUE(webrtc_internals.HasWakeLock());
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
+  EXPECT_FALSE(webrtc_internals.HasWakeLock());
 
-  // Remove a peer connection without closing it first.
-  webrtc_internals.OnRemovePeerConnection(pid, lid[2]);
-  EXPECT_EQ(2, webrtc_internals.num_open_connections());
+  webrtc_internals.OnUpdatePeerConnection(
+      kPid, kLid, "iceConnectionStateChange", "connected");
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 1);
   EXPECT_TRUE(webrtc_internals.HasWakeLock());
 
-  // Close the remaining peer connections.
-  webrtc_internals.OnUpdatePeerConnection(pid, lid[1], "stop", std::string());
-  EXPECT_EQ(1, webrtc_internals.num_open_connections());
+  webrtc_internals.OnUpdatePeerConnection(
+      kPid, kLid, "iceConnectionStateChange", "disconnected");
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
+  EXPECT_FALSE(webrtc_internals.HasWakeLock());
+
+  webrtc_internals.OnUpdatePeerConnection(
+      kPid, kLid, "iceConnectionStateChange", "connected");
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 1);
   EXPECT_TRUE(webrtc_internals.HasWakeLock());
 
-  webrtc_internals.OnUpdatePeerConnection(pid, lid[0], "stop", std::string());
-  EXPECT_EQ(0, webrtc_internals.num_open_connections());
+  webrtc_internals.OnRemovePeerConnection(kPid, kLid);
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
   EXPECT_FALSE(webrtc_internals.HasWakeLock());
 
-  // Remove the remaining peer connections.
-  webrtc_internals.OnRemovePeerConnection(pid, lid[1]);
-  EXPECT_EQ(0, webrtc_internals.num_open_connections());
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(WebRtcInternalsTest, WakeLockMultplePeerConnections) {
+  const int kRenderProcessId = 1;
+  const int kPid = 1;
+  const int kLids[] = {1, 2, 3};
+
+  WebRTCInternalsForTest webrtc_internals;
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
   EXPECT_FALSE(webrtc_internals.HasWakeLock());
 
-  webrtc_internals.OnRemovePeerConnection(pid, lid[0]);
-  EXPECT_EQ(0, webrtc_internals.num_open_connections());
+  for (const int lid : kLids) {
+    webrtc_internals.OnAddPeerConnection(kRenderProcessId, kPid, lid, kUrl,
+                                         kRtcConfiguration, kContraints);
+    EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
+    EXPECT_FALSE(webrtc_internals.HasWakeLock());
+  }
+
+  webrtc_internals.OnUpdatePeerConnection(
+      kPid, kLids[0], "iceConnectionStateChange", "connected");
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 1);
+  EXPECT_TRUE(webrtc_internals.HasWakeLock());
+
+  webrtc_internals.OnUpdatePeerConnection(
+      kPid, kLids[1], "iceConnectionStateChange", "completed");
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 2);
+  EXPECT_TRUE(webrtc_internals.HasWakeLock());
+
+  webrtc_internals.OnUpdatePeerConnection(
+      kPid, kLids[2], "iceConnectionStateChange", "checking");
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 3);
+  EXPECT_TRUE(webrtc_internals.HasWakeLock());
+
+  // A duplicate message should not alter the number of connected connections.
+  webrtc_internals.OnUpdatePeerConnection(
+      kPid, kLids[2], "iceConnectionStateChange", "checking");
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 3);
+  EXPECT_TRUE(webrtc_internals.HasWakeLock());
+
+  webrtc_internals.OnUpdatePeerConnection(kPid, kLids[0],
+                                          "iceConnectionStateChange", "closed");
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 2);
+  EXPECT_TRUE(webrtc_internals.HasWakeLock());
+
+  webrtc_internals.OnUpdatePeerConnection(kPid, kLids[1], "stop",
+                                          std::string());
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 1);
+  EXPECT_TRUE(webrtc_internals.HasWakeLock());
+
+  webrtc_internals.OnRemovePeerConnection(kPid, kLids[0]);
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 1);
+  EXPECT_TRUE(webrtc_internals.HasWakeLock());
+
+  webrtc_internals.OnRemovePeerConnection(kPid, kLids[1]);
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 1);
+  EXPECT_TRUE(webrtc_internals.HasWakeLock());
+
+  // Remove the remaining open peer connection.
+  webrtc_internals.OnRemovePeerConnection(kPid, kLids[2]);
+  EXPECT_EQ(webrtc_internals.num_connected_connections(), 0);
   EXPECT_FALSE(webrtc_internals.HasWakeLock());
 
   base::RunLoop().RunUntilIdle();