Don't collect a log per peerconnection if webrtc-internals isn't open.

content/browser/webrtc/webrtc_internals.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/webrtc/webrtc_internals.h"
 
 #include <stddef.h>
 
 #include <memory>
 #include <utility>
@@ skipping 17 matching lines @@
 #define IntToStringType base::IntToString
 #endif
 
 using base::ProcessId;
 using std::string;
 
 namespace content {
 
 namespace {
 
-static base::LazyInstance<WebRTCInternals>::Leaky g_webrtc_internals =
+base::LazyInstance<WebRTCInternals>::Leaky g_webrtc_internals =
     LAZY_INSTANCE_INITIALIZER;
 
 // Makes sure that |dict| has a ListValue under path "log".
-static base::ListValue* EnsureLogList(base::DictionaryValue* dict) {
+base::ListValue* EnsureLogList(base::DictionaryValue* dict) {
   base::ListValue* log = NULL;
   if (!dict->GetList("log", &log)) {
     log = new base::ListValue();
-    if (log)
-      dict->Set("log", log);
+    dict->Set("log", log);
   }
   return log;
 }
 
+// Removes the log entry associated with a given record.
+void FreeLogList(base::Value* value) {
+  DCHECK(value->IsType(base::Value::Type::DICTIONARY));
+  auto* dict = static_cast<base::DictionaryValue*>(value);

[Henrik Grunell, 2017/03/03 12:40:09]

Not being particularly familiar with base::Value, is this really how one would
get the value of the right type? Just doesn't seem very elegant, but if it's the
way then fine.

[tommi (sloooow) - chröme, 2017/03/03 14:15:14]

Yes, it's a polymorphic design with an enum that points to what actual type is
used (comparable to a VARIANT).
E.g. look at static_cast<>s here:
https://cs.chromium.org/chromium/src/base/values.cc?type=cs&sq=package:chromium

[Henrik Grunell, 2017/03/06 11:49:24]

Acknowledged.

+  dict->Remove("log", nullptr);
+}
+
 }  // namespace
 
 WebRTCInternals::PendingUpdate::PendingUpdate(
     const char* command,
     std::unique_ptr<base::Value> value)
     : command_(command), value_(std::move(value)) {}
 
 WebRTCInternals::PendingUpdate::PendingUpdate(PendingUpdate&& other)
     : command_(other.command_),
       value_(std::move(other.value_)) {}
@@ skipping 54 matching lines @@
 }
 
 void WebRTCInternals::OnAddPeerConnection(int render_process_id,
                                           ProcessId pid,
                                           int lid,
                                           const string& url,
                                           const string& rtc_configuration,
                                           const string& constraints) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
+  // TODO(tommi): Consider changing this design so that webrtc-internals has
+  // minimal impact if chrome://webrtc-internals isn't open.
+
   std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
   dict->SetInteger("rid", render_process_id);
   dict->SetInteger("pid", static_cast<int>(pid));
   dict->SetInteger("lid", lid);
   dict->SetString("rtcConfiguration", rtc_configuration);
   dict->SetString("constraints", constraints);
   dict->SetString("url", url);
   dict->SetBoolean("isOpen", true);
 
   if (observers_.might_have_observers())
     SendUpdate("addPeerConnection", dict->CreateDeepCopy());
 
   peer_connection_data_.Append(std::move(dict));
   ++num_open_connections_;
   CreateOrReleasePowerSaveBlocker();
 
   if (render_process_id_set_.insert(render_process_id).second) {
     RenderProcessHost* host = RenderProcessHost::FromID(render_process_id);
     if (host)
       host->AddObserver(this);
   }
 }
 
 void WebRTCInternals::OnRemovePeerConnection(ProcessId pid, int lid) {

[Henrik Grunell, 2017/03/03 12:40:09]

Great that you cleaned up the awkward code in this function!

[tommi (sloooow) - chröme, 2017/03/03 14:15:14]

:D

   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  for (size_t i = 0; i < peer_connection_data_.GetSize(); ++i) {
-    base::DictionaryValue* dict = NULL;
-    peer_connection_data_.GetDictionary(i, &dict);
 
-    int this_pid = 0;
-    int this_lid = 0;
-    dict->GetInteger("pid", &this_pid);
-    dict->GetInteger("lid", &this_lid);
-
-    if (this_pid != static_cast<int>(pid) || this_lid != lid)
-      continue;
-
+  size_t i;

[Henrik Grunell, 2017/03/03 12:40:09]

Nit: perhaps a more descriptive variable name?

[tommi (sloooow) - chröme, 2017/03/03 14:15:14]

Done... although I only changed it to 'index'.  |i| is what was being used
before and typically stands for index anyway.

+  base::DictionaryValue* dict = FindRecord(pid, lid, &i);
+  if (dict) {
     MaybeClosePeerConnection(dict);
     peer_connection_data_.Remove(i, NULL);
+  }
 
-    if (observers_.might_have_observers()) {
-      std::unique_ptr<base::DictionaryValue> id(new base::DictionaryValue());
-      id->SetInteger("pid", static_cast<int>(pid));
-      id->SetInteger("lid", lid);
-      SendUpdate("removePeerConnection", std::move(id));
-    }
-    break;
+  if (observers_.might_have_observers()) {
+    std::unique_ptr<base::DictionaryValue> id(new base::DictionaryValue());
+    id->SetInteger("pid", static_cast<int>(pid));
+    id->SetInteger("lid", lid);
+    SendUpdate("removePeerConnection", std::move(id));
   }
 }
 
 void WebRTCInternals::OnUpdatePeerConnection(
     ProcessId pid, int lid, const string& type, const string& value) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  for (size_t i = 0; i < peer_connection_data_.GetSize(); ++i) {
-    base::DictionaryValue* record = NULL;
-    peer_connection_data_.GetDictionary(i, &record);
+  base::DictionaryValue* record = FindRecord(pid, lid);
+  if (record && type == "stop")
+    MaybeClosePeerConnection(record);
 
-    int this_pid = 0, this_lid = 0;
-    record->GetInteger("pid", &this_pid);
-    record->GetInteger("lid", &this_lid);
+  // Don't update entries if there aren't any observers.
+  if (!observers_.might_have_observers())
+    return;
 
-    if (this_pid != static_cast<int>(pid) || this_lid != lid)
-      continue;
+  std::unique_ptr<base::DictionaryValue> log_entry(new base::DictionaryValue());
 
-    if (type == "stop") {
-      MaybeClosePeerConnection(record);
-    }
+  double epoch_time = base::Time::Now().ToJsTime();
+  string time = base::DoubleToString(epoch_time);
+  log_entry->SetString("time", time);
+  log_entry->SetString("type", type);
+  log_entry->SetString("value", value);
 
+  std::unique_ptr<base::DictionaryValue> update(new base::DictionaryValue());
+  update->SetInteger("pid", static_cast<int>(pid));
+  update->SetInteger("lid", lid);
+  update->MergeDictionary(log_entry.get());
+
+  SendUpdate("updatePeerConnection", std::move(update));

[Henrik Grunell, 2017/03/03 12:40:09]

In the previous code, this would not be done if no record is found, in the new
code it is. Is that intentional? (Maybe comment on why?)

[tommi (sloooow) - chröme, 2017/03/03 14:15:14]

Not intentional. Changed the behavior back now.

+
+  if (record) {
     // Append the update to the end of the log.
-    base::ListValue* log = EnsureLogList(record);
-    if (!log)
-      return;
-
-    std::unique_ptr<base::DictionaryValue> log_entry(
-        new base::DictionaryValue());
-
-    double epoch_time = base::Time::Now().ToJsTime();
-    string time = base::DoubleToString(epoch_time);
-    log_entry->SetString("time", time);
-    log_entry->SetString("type", type);
-    log_entry->SetString("value", value);
-
-    if (observers_.might_have_observers()) {
-      std::unique_ptr<base::DictionaryValue> update(
-          new base::DictionaryValue());
-      update->SetInteger("pid", static_cast<int>(pid));
-      update->SetInteger("lid", lid);
-      update->MergeDictionary(log_entry.get());
-
-      SendUpdate("updatePeerConnection", std::move(update));
-    }
-
-    log->Append(std::move(log_entry));
-
-    return;
+    EnsureLogList(record)->Append(std::move(log_entry));
   }
 }
 
 void WebRTCInternals::OnAddStats(base::ProcessId pid, int lid,
                                  const base::ListValue& value) {
   if (!observers_.might_have_observers())
     return;
 
   std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
   dict->SetInteger("pid", static_cast<int>(pid));
@@ skipping 35 matching lines @@
 }
 
 void WebRTCInternals::AddObserver(WebRTCInternalsUIObserver* observer) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   observers_.AddObserver(observer);
 }
 
 void WebRTCInternals::RemoveObserver(WebRTCInternalsUIObserver* observer) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   observers_.RemoveObserver(observer);
+  if (observers_.might_have_observers())
+    return;
 
   // Disables event log and audio debug recordings if enabled and the last
   // webrtc-internals page is going away.
-  if (!observers_.might_have_observers()) {
-    if (audio_debug_recordings_)
-      DisableAudioDebugRecordings();
-    DisableEventLogRecordings();
-  }
+  DisableAudioDebugRecordings();
+  DisableEventLogRecordings();
+
+  // TODO(tommi): Consider removing all the peer_connection_data_.
+  for (auto& dictionary : peer_connection_data_)
+    FreeLogList(dictionary.get());

[Henrik Grunell, 2017/03/03 12:40:09]

I don't know the design of WebRTC internals, so to make sure: we don't lose any
data that we might want to display later when opening the
chrome://webrtc-internals page when doing this?

[tommi (sloooow) - chröme, 2017/03/03 14:15:14]

We do loose data but the thinking is that when chrome://webrtc-internals is
closed, that's a signal that the data isn't needed anymore.

[Henrik Grunell, 2017/03/06 11:49:24]

OK, if that's the intention that's fine then.

 }
 
 void WebRTCInternals::UpdateObserver(WebRTCInternalsUIObserver* observer) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (peer_connection_data_.GetSize() > 0)
     observer->OnUpdate("updateAllPeerConnections", &peer_connection_data_);
 
   for (const auto& request : get_user_media_requests_) {
     observer->OnUpdate("addGetUserMedia", request.get());
   }
@@ skipping 18 matching lines @@
       FILE_PATH_LITERAL(""),
       web_contents->GetTopLevelNativeWindow(),
       NULL);
 #endif
 #endif
 }
 
 void WebRTCInternals::DisableAudioDebugRecordings() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 #if BUILDFLAG(ENABLE_WEBRTC)
+  if (!audio_debug_recordings_)
+    return;
+
   audio_debug_recordings_ = false;
 
   // Tear down the dialog since the user has unchecked the audio debug
   // recordings box.
   select_file_dialog_ = nullptr;
 
   for (RenderProcessHost::iterator i(
            content::RenderProcessHost::AllHostsIterator());
        !i.IsAtEnd(); i.Advance()) {
     i.GetCurrentValue()->DisableAudioDebugRecordings();
@@ skipping 237 matching lines @@
 void WebRTCInternals::ProcessPendingUpdates() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   while (!pending_updates_.empty()) {
     const auto& update = pending_updates_.front();
     for (auto& observer : observers_)
       observer.OnUpdate(update.command(), update.value());
     pending_updates_.pop();
   }
 }
 
+base::DictionaryValue* WebRTCInternals::FindRecord(
+    ProcessId pid,
+    int lid,
+    size_t* index /*= nullptr*/) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  base::DictionaryValue* record = nullptr;
+  for (size_t i = 0; i < peer_connection_data_.GetSize(); ++i) {
+    peer_connection_data_.GetDictionary(i, &record);
+
+    int this_pid = 0, this_lid = 0;
+    record->GetInteger("pid", &this_pid);
+    record->GetInteger("lid", &this_lid);
+
+    if (this_pid == static_cast<int>(pid) && this_lid == lid) {
+      if (index)
+        *index = i;
+      return record;
+    }
+  }
+  return nullptr;
+}
 }  // namespace content