RenderProcessHost exiting is an implicit PeerConnection removal

When a RenderProcessHost exits clearly or crashes, all of its
PeerConnections should be considered as removed.

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

content/browser/webrtc/webrtc_event_log_manager.cc

@@ -96,6 +96,11 @@ WebRtcEventLogManager::WebRtcEventLogManager()
 
 WebRtcEventLogManager::~WebRtcEventLogManager() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  for (RenderProcessHost* host : observed_render_process_hosts_) {
+    host->RemoveObserver(this);
+  }
+
   DCHECK(g_webrtc_event_log_manager);
   g_webrtc_event_log_manager = nullptr;
 }
@@ -105,6 +110,7 @@ void WebRtcEventLogManager::EnableForBrowserContext(
     base::OnceClosure reply) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   CHECK(!browser_context->IsOffTheRecord());
+  // Posting to task queue owned by the unretained object - unretained is safe.
   task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&WebRtcEventLogManager::EnableForBrowserContextInternal,
@@ -117,6 +123,7 @@ void WebRtcEventLogManager::DisableForBrowserContext(
     BrowserContextId browser_context_id,
     base::OnceClosure reply) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  // Posting to task queue owned by the unretained object - unretained is safe.
   task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&WebRtcEventLogManager::DisableForBrowserContextInternal,
@@ -129,6 +136,17 @@ void WebRtcEventLogManager::PeerConnectionAdded(
     int lid,
     base::OnceCallback<void(bool)> reply) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  RenderProcessHost* host = RenderProcessHost::FromID(render_process_id);
+  auto it = observed_render_process_hosts_.find(host);
+  if (it == observed_render_process_hosts_.end()) {
+    // This is the first PeerConnection which we see that's associated
+    // with this RPH.
+    host->AddObserver(this);
+    observed_render_process_hosts_.insert(host);
+  }
+
+  // Posting to task queue owned by the unretained object - unretained is safe.
   task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&WebRtcEventLogManager::PeerConnectionAddedInternal,
@@ -156,6 +174,7 @@ void WebRtcEventLogManager::PeerConnectionRemoved(
   }
   const BrowserContext* browser_context = host->GetBrowserContext();
 
+  // Posting to task queue owned by the unretained object - unretained is safe.
   task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&WebRtcEventLogManager::PeerConnectionRemovedInternal,
@@ -169,6 +188,7 @@ void WebRtcEventLogManager::EnableLocalLogging(
     base::OnceCallback<void(bool)> reply) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(!base_path.empty());
+  // Posting to task queue owned by the unretained object - unretained is safe.
   task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&WebRtcEventLogManager::EnableLocalLoggingInternal,
@@ -179,6 +199,7 @@ void WebRtcEventLogManager::EnableLocalLogging(
 void WebRtcEventLogManager::DisableLocalLogging(
     base::OnceCallback<void(bool)> reply) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  // Posting to task queue owned by the unretained object - unretained is safe.
   task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&WebRtcEventLogManager::DisableLocalLoggingInternal,
@@ -198,6 +219,7 @@ void WebRtcEventLogManager::StartRemoteLogging(
     browser_context_id = GetBrowserContextId(browser_context);
   }
 
+  // Posting to task queue owned by the unretained object - unretained is safe.
   task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&WebRtcEventLogManager::StartRemoteLoggingInternal,
@@ -213,6 +235,7 @@ void WebRtcEventLogManager::OnWebRtcEventLogWrite(
     base::OnceCallback<void(std::pair<bool, bool>)> reply) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   const BrowserContext* browser_context = GetBrowserContext(render_process_id);
+  // Posting to task queue owned by the unretained object - unretained is safe.
   task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&WebRtcEventLogManager::OnWebRtcEventLogWriteInternal,
@@ -225,6 +248,7 @@ void WebRtcEventLogManager::SetLocalLogsObserver(
     WebRtcLocalEventLogsObserver* observer,
     base::OnceClosure reply) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  // Posting to task queue owned by the unretained object - unretained is safe.
   task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&WebRtcEventLogManager::SetLocalLogsObserverInternal,
@@ -235,6 +259,7 @@ void WebRtcEventLogManager::SetRemoteLogsObserver(
     WebRtcRemoteEventLogsObserver* observer,
     base::OnceClosure reply) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  // Posting to task queue owned by the unretained object - unretained is safe.
   task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&WebRtcEventLogManager::SetRemoteLogsObserverInternal,
@@ -253,6 +278,38 @@ WebRtcEventLogManager::GetTaskRunnerForTesting() {
   return task_runner_;
 }
 
+void WebRtcEventLogManager::RenderProcessExited(RenderProcessHost* host,
+                                                base::TerminationStatus status,
+                                                int exit_code) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  RenderProcessHostExitedDestroyed(host);
+}
+
+void WebRtcEventLogManager::RenderProcessHostDestroyed(
+    RenderProcessHost* host) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  RenderProcessHostExitedDestroyed(host);
+}
+
+void WebRtcEventLogManager::RenderProcessHostExitedDestroyed(
+    RenderProcessHost* host) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(host);
+
+  auto it = observed_render_process_hosts_.find(host);
+  if (it == observed_render_process_hosts_.end()) {
+    return;  // We've never seen PeerConnections associated with this RPH.
+  }
+  host->RemoveObserver(this);
+  observed_render_process_hosts_.erase(host);
+
+  // Posting to task queue owned by the unretained object - unretained is safe.
+  task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&WebRtcEventLogManager::RenderProcessExitedInternal,
+                     base::Unretained(this), host->GetID()));
+}
+
 void WebRtcEventLogManager::OnLocalLogStarted(PeerConnectionKey peer_connection,
                                               const base::FilePath& file_path) {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
@@ -450,6 +507,12 @@ void WebRtcEventLogManager::OnWebRtcEventLogWriteInternal(
   }
 }
 
+void WebRtcEventLogManager::RenderProcessExitedInternal(int render_process_id) {
+  DCHECK(task_runner_->RunsTasksInCurrentSequence());
+  local_logs_manager_.RenderProcessHostExitedDestroyed(render_process_id);
+  remote_logs_manager_.RenderProcessHostExitedDestroyed(render_process_id);
+}
+
 void WebRtcEventLogManager::SetLocalLogsObserverInternal(
     WebRtcLocalEventLogsObserver* observer,
     base::OnceClosure reply) {

content/browser/webrtc/webrtc_event_log_manager.h

@@ -11,6 +11,7 @@
 #include <utility>
 
 #include "base/callback.h"
+#include "base/containers/flat_set.h"
 #include "base/files/file_path.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/optional.h"
@@ -20,6 +21,7 @@
 #include "content/browser/webrtc/webrtc_local_event_log_manager.h"
 #include "content/browser/webrtc/webrtc_remote_event_log_manager.h"
 #include "content/common/content_export.h"
+#include "content/public/browser/render_process_host_observer.h"
 
 namespace content {
 
@@ -32,7 +34,8 @@ class BrowserContext;
 // user from the WebRTCIntenals. (A log may simulatenously be written to both,
 // either, or none.)
 class CONTENT_EXPORT WebRtcEventLogManager
-    : public WebRtcLocalEventLogsObserver,
+    : public RenderProcessHostObserver,
+      public WebRtcLocalEventLogsObserver,
       public WebRtcRemoteEventLogsObserver {
  public:
   using BrowserContextId = WebRtcRemoteEventLogManager::BrowserContextId;
@@ -209,6 +212,16 @@ class CONTENT_EXPORT WebRtcEventLogManager
 
   static WebRtcEventLogManager* g_webrtc_event_log_manager;
 
+  // RenderProcessHostObserver implementation.
+  void RenderProcessExited(RenderProcessHost* host,
+                           base::TerminationStatus status,
+                           int exit_code) override;
+  void RenderProcessHostDestroyed(RenderProcessHost* host) override;
+
+  // RenderProcessExited() and RenderProcessHostDestroyed() treated similarly
+  // by this function.
+  void RenderProcessHostExitedDestroyed(RenderProcessHost* host);
+
   // WebRtcLocalEventLogsObserver implementation:
   void OnLocalLogStarted(PeerConnectionKey peer_connection,
                          const base::FilePath& file_path) override;
@@ -257,6 +270,8 @@ class CONTENT_EXPORT WebRtcEventLogManager
       const std::string& message,
       base::OnceCallback<void(std::pair<bool, bool>)> reply);
 
+  void RenderProcessExitedInternal(int render_process_id);
+
   void SetLocalLogsObserverInternal(WebRtcLocalEventLogsObserver* observer,
                                     base::OnceClosure reply);
 
@@ -294,6 +309,12 @@ class CONTENT_EXPORT WebRtcEventLogManager
   std::map<PeerConnectionKey, LoggingTargetBitmap>
       peer_connections_with_event_logging_enabled_;
 
+  // The set of RenderProcessHosts with which the manager is registered for
+  // observation. Allows us to register for each RPH only once, and get notified
+  // when it exits (cleanly or due to a crash).
+  // This object is only to be accessed on the UI thread.
+  base::flat_set<RenderProcessHost*> observed_render_process_hosts_;
+
   // In production, this holds a small object that just tells WebRTC (via
   // PeerConnectionTracker) to start/stop producing event logs for a specific
   // peer connection. In (relevant) unit tests, a mock will be injected.

content/browser/webrtc/webrtc_local_event_log_manager.cc

@@ -121,6 +121,30 @@ bool WebRtcLocalEventLogManager::EventLogWrite(int render_process_id,
   return WriteToLogFile(it, message);
 }
 
+void WebRtcLocalEventLogManager::RenderProcessHostExitedDestroyed(
+    int render_process_id) {
+  // Remove all of the peer connections associated with this render process.
+  auto pc_it = active_peer_connections_.begin();
+  while (pc_it != active_peer_connections_.end()) {
+    if (pc_it->render_process_id == render_process_id) {
+      pc_it = active_peer_connections_.erase(pc_it);
+    } else {
+      ++pc_it;
+    }
+  }
+
+  // Close all of the files that were associated with peer connections which
+  // belonged to this render process.
+  auto log_it = log_files_.begin();
+  while (log_it != log_files_.end()) {
+    if (log_it->first.render_process_id == render_process_id) {
+      log_it = CloseLogFile(log_it);
+    } else {
+      ++log_it;
+    }
+  }
+}
+
 void WebRtcLocalEventLogManager::SetClockForTesting(base::Clock* clock) {
   clock_for_testing_ = clock;
 }

content/browser/webrtc/webrtc_local_event_log_manager.h

@@ -32,6 +32,8 @@ class WebRtcLocalEventLogManager final : public LogFileWriter {
                      int lid,
                      const std::string& message);
 
+  void RenderProcessHostExitedDestroyed(int render_process_id);
+
   // This function is public, but this entire class is a protected
   // implementation detail of WebRtcEventLogManager, which hides this
   // function from everybody except its own unit tests.

content/browser/webrtc/webrtc_remote_event_log_manager.cc

@@ -158,6 +158,41 @@ bool WebRtcRemoteEventLogManager::EventLogWrite(int render_process_id,
   return WriteToLogFile(it, message);
 }
 
+void WebRtcRemoteEventLogManager::RenderProcessHostExitedDestroyed(
+    int render_process_id) {
+  // Closing files will call MaybeStartUploading(). Avoid letting that upload
+  // any recently expired files.
+  PrunePendingLogs();
+
+  // Remove all of the peer connections associated with this render process.
+  // It's important to do this before closing the actual files, because closing
+  // files can trigger a new upload if no active peer connections are present.
+  auto pc_it = active_peer_connections_.begin();
+  while (pc_it != active_peer_connections_.end()) {
+    if (pc_it->render_process_id == render_process_id) {
+      pc_it = active_peer_connections_.erase(pc_it);
+    } else {
+      ++pc_it;
+    }
+  }
+
+  // Close all of the files that were associated with peer connections which
+  // belonged to this render process.
+  auto log_it = active_logs_.begin();
+  while (log_it != active_logs_.end()) {
+    if (log_it->first.render_process_id == render_process_id) {
+      log_it = CloseLogFile(log_it);
+    } else {
+      ++log_it;
+    }
+  }
+
+  // Though CloseLogFile() calls this, it's important to also do this
+  // explicitly, since it could be that no files were closed, but some
+  // active PeerConnections that were suppressing uploading are now gone.
+  MaybeStartUploading();
+}
+
 void WebRtcRemoteEventLogManager::OnWebRtcEventLogUploadComplete(
     const base::FilePath& file_path,
     bool upload_successful) {

content/browser/webrtc/webrtc_remote_event_log_manager.h

@@ -83,6 +83,10 @@ class CONTENT_EXPORT WebRtcRemoteEventLogManager final
                      int lid,
                      const std::string& message);
 
+  // An implicit PeerConnectionRemoved() on all of the peer connections that
+  // were associated with the renderer process.
+  void RenderProcessHostExitedDestroyed(int render_process_id);
+
   // WebRtcEventLogUploaderObserver implementation.
   void OnWebRtcEventLogUploadComplete(const base::FilePath& file_path,
                                       bool upload_successful) override;