Move the call to enable the WebRTC event log from PeerConnectionFactory to PeerConnection.

content/browser/media/webrtc/webrtc_eventlog_host.cc

Index: content/browser/media/webrtc/webrtc_eventlog_host.cc
diff --git a/content/browser/media/webrtc/webrtc_eventlog_host.cc b/content/browser/media/webrtc/webrtc_eventlog_host.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0fd64891da6dd36421410b736e4f5c2d703d91df
--- /dev/null
+++ b/content/browser/media/webrtc/webrtc_eventlog_host.cc
@@ -0,0 +1,143 @@
+// Copyright 2016 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/media/webrtc/webrtc_eventlog_host.h"
+
+#include <algorithm>
+#include <string>
+
+#include "base/files/file_util.h"
+#include "base/strings/string_number_conversions.h"
+#include "content/common/media/peer_connection_tracker_messages.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/render_process_host.h"
+#include "ipc/ipc_platform_file.h"
+
+namespace content {
+
+int WebRTCEventLogHost::number_active_log_files_ = 0;
+
+namespace {
+
+#if defined(OS_ANDROID)
+const int kMaxNumberLogFiles = 3;

[Henrik Grunell, 2016/06/01 08:31:11]

Nit: Given the much smaller storage on Android, should it be even more limited?
We talked about the max file size offline, what will it be again?

[Ivo-OOO until feb 6, 2016/06/02 14:43:38]

On android the max file size limit is set to 10MB per file (see
content/renderer/media/peer_connection_tracker.cc), so the maximum amount of
storage is 30 MB. Do you think it should be restricted further?

[Henrik Grunell, 2016/06/03 07:40:52]

Ah OK, that sounds fine.

Nit: Could you comment on that there is a file size limit and point to where
that is? Although that pointer might be stale, it's better than nothing.

[Ivo-OOO until feb 6, 2016/06/03 16:07:01]

Done.

+#else
+const int kMaxNumberLogFiles = 5;
+#endif
+
+// Appends the IDs to the RTC event log file name.
+base::FilePath GetWebRtcEventLogPath(const base::FilePath& base_file,
+                                     int render_process_id,
+                                     int connection_id) {
+  return base_file.AddExtension(base::IntToString(render_process_id))
+      .AddExtension(base::IntToString(connection_id));
+}
+
+// Opens a logfile to pass on to the renderer.
+IPC::PlatformFileForTransit CreateFileForProcess(
+    const base::FilePath& base_path,
+    int render_process_id,
+    int connection_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+  base::FilePath file_path =
+      GetWebRtcEventLogPath(base_path, render_process_id, connection_id);
+  base::File event_log_file(
+      file_path, base::File::FLAG_OPEN_ALWAYS | base::File::FLAG_APPEND);

[Henrik Grunell, 2016/06/01 08:31:11]

Should it overwrite instead perhaps?

[Ivo-OOO until feb 6, 2016/06/02 14:43:38]

Good point, I guess that makes more sense.

+  if (!event_log_file.IsValid()) {
+    PLOG(ERROR) << "Could not open WebRTC event log file, error="
+                << event_log_file.error_details();
+    return IPC::InvalidPlatformFileForTransit();
+  }
+  return IPC::TakePlatformFileForTransit(std::move(event_log_file));
+}
+
+void SendEventLogFileToRenderer(content::RenderProcessHost* rph,
+                                int local_id,
+                                IPC::PlatformFileForTransit file_for_transit) {
+  if (file_for_transit == IPC::InvalidPlatformFileForTransit())
+    return;
+
+  rph->Send(
+      new PeerConnectionTracker_StartEventLog(local_id, file_for_transit));
+}
+
+}  // namespace
+
+WebRTCEventLogHost::WebRTCEventLogHost(int render_process_id)
+    : render_process_id_(render_process_id), rtc_event_logging_enabled_(false) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+}
+
+WebRTCEventLogHost::~WebRTCEventLogHost() {}
+
+void WebRTCEventLogHost::PeerConnectionAdded(int peer_connection_local_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(std::find(active_peer_connection_local_id_.begin(),
+                   active_peer_connection_local_id_.end(),
+                   peer_connection_local_id) ==
+         active_peer_connection_local_id_.end());
+  active_peer_connection_local_id_.push_back(peer_connection_local_id);
+  if (rtc_event_logging_enabled_ &&
+      number_active_log_files_ < kMaxNumberLogFiles) {
+    content::RenderProcessHost* host =
+        content::RenderProcessHost::FromID(render_process_id_);
+    if (host) {
+      BrowserThread::PostTaskAndReplyWithResult(
+          BrowserThread::FILE, FROM_HERE,
+          base::Bind(&CreateFileForProcess, base_file_path_, render_process_id_,
+                     peer_connection_local_id),
+          base::Bind(&SendEventLogFileToRenderer, host,

[Henrik Grunell, 2016/06/01 08:31:10]

How is it guaranteed that |host| is valid when SendEventLogFileToRenderer is
executed? It seems like it could be a use after free. Since the
WebRTCEventLogHost is tied to a render process (and RPH) you could reply to a
member function and use WeakPtr. Then you can also get the host ptr in that
function.

[Ivo-OOO until feb 6, 2016/06/02 14:43:38]

Good catch, I did not consider that. I changed the code to reply to a member
function and use a WeakPtr like you suggest.

+                     peer_connection_local_id));
+      ++number_active_log_files_;

[Henrik Grunell, 2016/06/01 08:31:10]

If creating the file fails, this will be incorrectly increased. If you reply to
a member function, you can increase there.

[Ivo-OOO until feb 6, 2016/06/02 14:43:38]

Done.

+    }
+  }
+}
+
+void WebRTCEventLogHost::PeerConnectionRemoved(int peer_connection_local_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  const auto found = std::find(active_peer_connection_local_id_.begin(),
+                               active_peer_connection_local_id_.end(),
+                               peer_connection_local_id) !=
+                     active_peer_connection_local_id_.end();
+  DCHECK(found != active_peer_connection_local_id_.end());
+  active_peer_connection_local_id_.erase(found);

[Henrik Grunell, 2016/06/01 08:31:10]

This means that the log has stopped for that PC, right? Should the counter be
decreased to allow a new PC to be logged? I imagine a test case could be that
logging is enabled, and PCs come and go several times but just one is alive at a
time. After 5 PCs we wouldn't log more.

[Ivo-OOO until feb 6, 2016/06/02 14:43:38]

I guess that depends on what kind of behavior we want. I made it this way
because I liked that we can guarantee that no more than 5 logfiles will be
created no matter what happens, and the amount of storage that can be used for
logs has a hard cap (even if you forget about clicking the checkbox and leave
the tab open for months). 
Let me know if you think the other behavior makes more sense.

[Henrik Grunell, 2016/06/03 07:40:52]

That's a good point. OK, let's keep it this way. Please update the description
(on html page) that when the limit is reached, logging must be restarted.

[Ivo-OOO until feb 6, 2016/06/03 16:07:01]

Done.

+}
+
+bool WebRTCEventLogHost::StartWebRTCEventLog(const base::FilePath& file_path) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (rtc_event_logging_enabled_)
+    return false;
+  RenderProcessHost* host = RenderProcessHost::FromID(render_process_id_);
+  if (host) {
+    rtc_event_logging_enabled_ = true;
+    base_file_path_ = file_path;
+    for (int local_id : active_peer_connection_local_id_) {
+      if (number_active_log_files_ < kMaxNumberLogFiles) {
+        BrowserThread::PostTaskAndReplyWithResult(

[Henrik Grunell, 2016/06/01 08:31:10]

Nit: to make it more readable and decrease redundancy, you could put the common
code (here and PeerConnectionAdded()) in its own function.

[Ivo-OOO until feb 6, 2016/06/02 14:43:38]

Done.

+            BrowserThread::FILE, FROM_HERE,
+            base::Bind(&CreateFileForProcess, file_path, render_process_id_,
+                       local_id),
+            base::Bind(&SendEventLogFileToRenderer, host, local_id));
+        ++number_active_log_files_;
+      }
+    }
+  }
+  return true;
+}
+
+bool WebRTCEventLogHost::StopWebRTCEventLog() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (!rtc_event_logging_enabled_)
+    return false;
+  rtc_event_logging_enabled_ = false;
+  number_active_log_files_ = 0;
+  RenderProcessHost* host = RenderProcessHost::FromID(render_process_id_);
+  if (host) {
+    for (int local_id : active_peer_connection_local_id_)
+      host->Send(new PeerConnectionTracker_StopEventLog(local_id));
+  }
+  return true;
+}
+
+}  // namespace content