Clear pending WebRTC remote-bound logs if policy disabled at start

When a profile is loaded for the first time, if the policy
controlling remote-bound WebRTC event logs is disabled, clear
any pending logs from previous times the profile was loaded, when
the policy might have been enabled.

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

chrome/browser/media/webrtc/webrtc_event_log_manager.cc

@@ -130,8 +130,6 @@ WebRtcEventLogManager::~WebRtcEventLogManager() {
   g_webrtc_event_log_manager = nullptr;
 }
 
-// TODO(crbug.com/775415): If a BrowserContext had the policy as active in
-// the past, but no longer does, purge pending log files from before.
 void WebRtcEventLogManager::EnableForBrowserContext(
     BrowserContext* browser_context,
     base::OnceClosure reply) {
@@ -147,7 +145,18 @@ void WebRtcEventLogManager::EnableForBrowserContext(
   StartListeningForPrefChangeForBrowserContext(browser_context);
 
   if (!IsRemoteLoggingAllowedForBrowserContext(browser_context)) {
-    MaybeReply(FROM_HERE, std::move(reply));
+    // If remote-bound logging was enabled during a previous Chrome session,
+    // it might have produced some pending log files, which we will now
+    // wish to remove.
+    // |this| is destroyed by ~BrowserProcessImpl(), so base::Unretained(this)
+    // will not be dereferenced after destruction.
+    task_runner_->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            &WebRtcEventLogManager::
+                RemovePendingRemoteBoundLogsForNotEnabledBrowserContext,
+            base::Unretained(this), GetBrowserContextId(browser_context),
+            browser_context->GetPath(), std::move(reply)));
     return;
   }
 
@@ -674,6 +683,19 @@ void WebRtcEventLogManager::DisableRemoteBoundLoggingForBrowserContext(
   MaybeReply(FROM_HERE, std::move(reply));
 }
 
+void WebRtcEventLogManager::
+    RemovePendingRemoteBoundLogsForNotEnabledBrowserContext(
+        BrowserContextId browser_context_id,
+        const base::FilePath& browser_context_dir,
+        base::OnceClosure reply) {
+  DCHECK(task_runner_->RunsTasksInCurrentSequence());
+
+  remote_logs_manager_.RemovePendingLogsForNotEnabledBrowserContext(
+      browser_context_id, browser_context_dir);
+
+  MaybeReply(FROM_HERE, std::move(reply));
+}
+
 void WebRtcEventLogManager::PeerConnectionAddedInternal(
     PeerConnectionKey key,
     const std::string& peer_connection_id,

chrome/browser/media/webrtc/webrtc_event_log_manager.h

@@ -229,6 +229,11 @@ class WebRtcEventLogManager final : public content::RenderProcessHostObserver,
       BrowserContextId browser_context_id,
       base::OnceClosure reply);
 
+  void RemovePendingRemoteBoundLogsForNotEnabledBrowserContext(
+      BrowserContextId browser_context_id,
+      const base::FilePath& browser_context_dir,
+      base::OnceClosure reply);
+
   void PeerConnectionAddedInternal(PeerConnectionKey key,
                                    const std::string& peer_connection_id,
                                    base::OnceCallback<void(bool)> reply);

chrome/browser/media/webrtc/webrtc_event_log_manager_remote.cc

@@ -402,6 +402,18 @@ void WebRtcRemoteEventLogManager::ClearCacheForBrowserContext(
   MaybeCancelUpload(delete_begin, delete_end, browser_context_id);
 }
 
+void WebRtcRemoteEventLogManager::RemovePendingLogsForNotEnabledBrowserContext(
+    BrowserContextId browser_context_id,
+    const base::FilePath& browser_context_dir) {
+  DCHECK(task_runner_->RunsTasksInCurrentSequence());
+  DCHECK(!BrowserContextEnabled(browser_context_id));
+  const base::FilePath remote_bound_logs_dir =
+      GetRemoteBoundWebRtcEventLogsDir(browser_context_dir);
+  if (!base::DeleteFile(remote_bound_logs_dir, /*recursive=*/true)) {
+    LOG(ERROR) << "Failed to delete  `" << remote_bound_logs_dir << ".";
+  }
+}
+
 void WebRtcRemoteEventLogManager::RenderProcessHostExitedDestroyed(
     int render_process_id) {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());

chrome/browser/media/webrtc/webrtc_event_log_manager_remote.h

@@ -139,6 +139,13 @@ class WebRtcRemoteEventLogManager final
                                    const base::Time& delete_begin,
                                    const base::Time& delete_end);
 
+  // Works on not-enabled BrowserContext-s, which means the logs are never made
+  // eligible for upload. Useful when a BrowserContext is loaded which in
+  // the past had remote-logging enabled, but no longer does.
+  void RemovePendingLogsForNotEnabledBrowserContext(
+      BrowserContextId browser_context_id,
+      const base::FilePath& browser_context_dir);
+
   // An implicit PeerConnectionRemoved() on all of the peer connections that
   // were associated with the renderer process.
   void RenderProcessHostExitedDestroyed(int render_process_id);

chrome/browser/media/webrtc/webrtc_event_log_manager_unittest.cc

@@ -118,6 +118,20 @@ PeerConnectionKey GetPeerConnectionKey(const RenderProcessHost* rph, int lid) {
   return PeerConnectionKey(rph->GetID(), lid, browser_context_id);
 }
 
+bool CreateRemoteBoundLogFile(const base::FilePath& dir,
+                              const base::FilePath::StringPieceType& extension,
+                              base::FilePath* file_path,
+                              base::File* file) {
+  *file_path =
+      dir.Append(kRemoteBoundWebRtcEventLogFileNamePrefix)
+          .InsertBeforeExtensionASCII("01234567890123456789012345678901")
+          .AddExtension(extension);
+  constexpr int file_flags = base::File::FLAG_CREATE | base::File::FLAG_WRITE |
+                             base::File::FLAG_EXCLUSIVE_WRITE;
+  file->Initialize(*file_path, file_flags);
+  return (file->IsValid() && file->created());
+}
+
 // This implementation does not upload files, nor pretends to have finished an
 // upload. Most importantly, it does not get rid of the locally-stored log file
 // after finishing a simulated upload; this is useful because it keeps the file
@@ -581,7 +595,12 @@ class WebRtcEventLogManagerTestBase : public ::testing::Test {
 
   base::FilePath RemoteBoundLogsDir(
       const BrowserContext* browser_context) const {
-    return GetRemoteBoundWebRtcEventLogsDir(browser_context->GetPath());
+    return RemoteBoundLogsDir(browser_context->GetPath());
+  }
+
+  base::FilePath RemoteBoundLogsDir(
+      const base::FilePath& browser_context_base_dir) const {
+    return GetRemoteBoundWebRtcEventLogsDir(browser_context_base_dir);
   }
 
   // Initiate an arbitrary synchronous operation, allowing any tasks pending
@@ -898,8 +917,19 @@ class WebRtcEventLogManagerTestWithRemoteLoggingDisabled
 
 class WebRtcEventLogManagerTestPolicy : public WebRtcEventLogManagerTestBase {
  public:
-  WebRtcEventLogManagerTestPolicy() {
-    scoped_feature_list_.InitAndEnableFeature(features::kWebRtcRemoteEventLog);
+  ~WebRtcEventLogManagerTestPolicy() override = default;
+
+  // Defer to setup from the body.
+  void SetUp() override {}
+
+  void SetUp(bool feature_enabled) {
+    if (feature_enabled) {
+      scoped_feature_list_.InitAndEnableFeature(
+          features::kWebRtcRemoteEventLog);
+    } else {
+      scoped_feature_list_.InitAndDisableFeature(
+          features::kWebRtcRemoteEventLog);
+    }
 
     // Avoid proactive pruning; it has the potential to mess up tests, as well
     // as keep pendings tasks around with a dangling reference to the unit
@@ -907,10 +937,10 @@ class WebRtcEventLogManagerTestPolicy : public WebRtcEventLogManagerTestBase {
     scoped_command_line_.GetProcessCommandLine()->AppendSwitchASCII(
         ::switches::kWebRtcRemoteEventLogUploadDelayMs, "0");
 
-    event_log_manager_ = WebRtcEventLogManager::CreateSingletonInstance();
-  }
+    CreateWebRtcEventLogManager(Compression::GZIP_PERFECT_ESTIMATION);
 
-  ~WebRtcEventLogManagerTestPolicy() override = default;
+    WebRtcEventLogManagerTestBase::SetUp();
+  }
 };
 
 class WebRtcEventLogManagerTestUploadSuppressionDisablingFlag
@@ -972,15 +1002,10 @@ class WebRtcEventLogManagerTestForNetworkConnectivity
     // creation of logs in a previous session.
     ASSERT_TRUE(CreateDirectory(remote_logs_dir));
 
-    const base::FilePath file_path =
-        remote_logs_dir.Append(kRemoteBoundWebRtcEventLogFileNamePrefix)
-            .InsertBeforeExtensionASCII("01234567890123456789012345678901")
-            .AddExtension(remote_log_extension_);
-    constexpr int file_flags = base::File::FLAG_CREATE |
-                               base::File::FLAG_WRITE |
-                               base::File::FLAG_EXCLUSIVE_WRITE;
-    file_ = base::File(file_path, file_flags);
-    ASSERT_TRUE(file_.IsValid() && file_.created());
+    base::FilePath file_path;
+    ASSERT_TRUE(CreateRemoteBoundLogFile(remote_logs_dir, remote_log_extension_,
+                                         &file_path, &file_));
+
     expected_files_.emplace_back(browser_context_id_, file_path,
                                  GetLastModificationTime(file_path));
   }
@@ -3586,6 +3611,8 @@ INSTANTIATE_TEST_CASE_P(,
 
 // This test is redundant; it is provided for completeness; see following tests.
 TEST_F(WebRtcEventLogManagerTestPolicy, StartsEnabledAllowsRemoteLogging) {
+  SetUp(true);  // Feature generally enabled (kill-switch not engaged).
+
   const bool allow_remote_logging = true;
   auto browser_context = CreateBrowserContext("name", allow_remote_logging);
 
@@ -3598,6 +3625,8 @@ TEST_F(WebRtcEventLogManagerTestPolicy, StartsEnabledAllowsRemoteLogging) {
 
 // This test is redundant; it is provided for completeness; see following tests.
 TEST_F(WebRtcEventLogManagerTestPolicy, StartsDisabledRejectsRemoteLogging) {
+  SetUp(true);  // Feature generally enabled (kill-switch not engaged).
+
   const bool allow_remote_logging = false;
   auto browser_context = CreateBrowserContext("name", allow_remote_logging);
 
@@ -3612,6 +3641,8 @@ TEST_F(WebRtcEventLogManagerTestPolicy, StartsDisabledRejectsRemoteLogging) {
 // the pref value.
 TEST_F(WebRtcEventLogManagerTestPolicy,
        StartsEnabledThenDisabledRejectsRemoteLogging1) {
+  SetUp(true);  // Feature generally enabled (kill-switch not engaged).
+
   bool allow_remote_logging = true;
   auto profile = CreateBrowserContext("name", allow_remote_logging);
 
@@ -3631,6 +3662,8 @@ TEST_F(WebRtcEventLogManagerTestPolicy,
 // the pref value.
 TEST_F(WebRtcEventLogManagerTestPolicy,
        StartsEnabledThenDisabledRejectsRemoteLogging2) {
+  SetUp(true);  // Feature generally enabled (kill-switch not engaged).
+
   bool allow_remote_logging = true;
   auto profile = CreateBrowserContext("name", allow_remote_logging);
 
@@ -3650,6 +3683,8 @@ TEST_F(WebRtcEventLogManagerTestPolicy,
 // the pref value.
 TEST_F(WebRtcEventLogManagerTestPolicy,
        StartsDisabledThenEnabledAllowsRemoteLogging1) {
+  SetUp(true);  // Feature generally enabled (kill-switch not engaged).
+
   bool allow_remote_logging = false;
   auto profile = CreateBrowserContext("name", allow_remote_logging);
 
@@ -3669,6 +3704,8 @@ TEST_F(WebRtcEventLogManagerTestPolicy,
 // the pref value.
 TEST_F(WebRtcEventLogManagerTestPolicy,
        StartsDisabledThenEnabledAllowsRemoteLogging2) {
+  SetUp(true);  // Feature generally enabled (kill-switch not engaged).
+
   bool allow_remote_logging = false;
   auto profile = CreateBrowserContext("name", allow_remote_logging);
 
@@ -3686,6 +3723,8 @@ TEST_F(WebRtcEventLogManagerTestPolicy,
 
 TEST_F(WebRtcEventLogManagerTestPolicy,
        StartsDisabledThenEnabledUploadsPendingLogFiles) {
+  SetUp(true);  // Feature generally enabled (kill-switch not engaged).
+
   bool allow_remote_logging = false;
   auto profile = CreateBrowserContext("name", allow_remote_logging);
 
@@ -3719,6 +3758,8 @@ TEST_F(WebRtcEventLogManagerTestPolicy,
 
 TEST_F(WebRtcEventLogManagerTestPolicy,
        StartsEnabledThenDisabledDoesNotUploadPendingLogFiles) {
+  SetUp(true);  // Feature generally enabled (kill-switch not engaged).
+
   SuppressUploading();
 
   std::list<WebRtcLogFileInfo> empty_list;
@@ -3750,6 +3791,8 @@ TEST_F(WebRtcEventLogManagerTestPolicy,
 
 TEST_F(WebRtcEventLogManagerTestPolicy,
        StartsEnabledThenDisabledDeletesPendingLogFiles) {
+  SetUp(true);  // Feature generally enabled (kill-switch not engaged).
+
   SuppressUploading();
 
   std::list<WebRtcLogFileInfo> empty_list;
@@ -3793,6 +3836,8 @@ TEST_F(WebRtcEventLogManagerTestPolicy,
 
 TEST_F(WebRtcEventLogManagerTestPolicy,
        StartsEnabledThenDisabledCancelsAndDeletesCurrentlyUploadedLogFile) {
+  SetUp(true);  // Feature generally enabled (kill-switch not engaged).
+
   // This factory expects exactly one log to be created, then cancelled.
   SetWebRtcEventLogUploaderFactoryForTesting(
       std::make_unique<NullWebRtcEventLogUploader::Factory>(true, 1));
@@ -3831,6 +3876,84 @@ TEST_F(WebRtcEventLogManagerTestPolicy,
   // WebRtcEventLogUploaderImplTest.CancelOnOngoingUploadDeletesFile tests that.
 }
 
+// This test makes sure that if the policy was enabled in the past, but was
+// disabled while Chrome was not running, pending logs created during the
+// earlier session will be deleted from disk.
+TEST_F(WebRtcEventLogManagerTestPolicy,
+       PendingLogsFromPreviousSessionRemovedIfPolicyDisabledAtNewSessionStart) {
+  SetUp(true);  // Feature generally enabled (kill-switch not engaged).
+
+  SuppressUploading();
+
+  SetWebRtcEventLogUploaderFactoryForTesting(
+      std::make_unique<NullWebRtcEventLogUploader::Factory>(true, 0));
+
+  bool allow_remote_logging = true;
+  auto browser_context = CreateBrowserContext("name", allow_remote_logging);
+
+  const base::FilePath browser_context_dir =
+      RemoteBoundLogsDir(browser_context.get());
+  ASSERT_TRUE(base::DirectoryExists(browser_context_dir));
+
+  auto rph = std::make_unique<MockRenderProcessHost>(browser_context.get());
+  const auto key = GetPeerConnectionKey(rph.get(), kLid);
+
+  // Produce an empty log file in the BrowserContext. It's not uploaded
+  // because uploading is suppressed.
+  ASSERT_TRUE(PeerConnectionAdded(key));
+  ASSERT_TRUE(allow_remote_logging)
+      << "Must turn off after StartRemoteLogging, to test the right thing.";
+  ASSERT_EQ(StartRemoteLogging(key), allow_remote_logging);
+  ASSERT_TRUE(PeerConnectionRemoved(key));
+
+  // Reload the BrowserContext, but this time with the policy disabling
+  // the feature.
+  rph.reset();
+  browser_context.reset();
+  ASSERT_TRUE(base::DirectoryExists(browser_context_dir));  // Test sanity
+  allow_remote_logging = false;
+  browser_context = CreateBrowserContext("name", allow_remote_logging);
+
+  // Test focus - pending log files removed, as well as any potential metadata
+  // associated with remote-bound logging for |browser_context|.
+  ASSERT_FALSE(base::DirectoryExists(browser_context_dir));
+
+  // When NullWebRtcEventLogUploader::Factory is destroyed, it will show that
+  // the deleted log file was never uploaded.
+  UnsuppressUploading();
+  WaitForPendingTasks();
+}
+
+TEST_F(WebRtcEventLogManagerTestPolicy,
+       PendingLogsFromPreviousSessionRemovedIfRemoteLoggingKillSwitchEngaged) {
+  SetUp(false);  // Feature generally disabled (kill-switch engaged).
+
+  SetWebRtcEventLogUploaderFactoryForTesting(
+      std::make_unique<NullWebRtcEventLogUploader::Factory>(true, 0));
+
+  const std::string name = "name";
+  const base::FilePath browser_context_dir =
+      profiles_dir_.GetPath().AppendASCII(name);
+  const base::FilePath remote_bound_dir =
+      RemoteBoundLogsDir(browser_context_dir);
+  ASSERT_FALSE(base::PathExists(remote_bound_dir));
+
+  base::FilePath file_path;
+  base::File file;
+  ASSERT_TRUE(base::CreateDirectory(remote_bound_dir));
+  ASSERT_TRUE(CreateRemoteBoundLogFile(remote_bound_dir, remote_log_extension_,
+                                       &file_path, &file));
+  file.Close();
+
+  const bool allow_remote_logging = true;
+  auto browser_context = CreateBrowserContext(name, allow_remote_logging);
+  ASSERT_EQ(browser_context->GetPath(), browser_context_dir);  // Test sanity
+
+  WaitForPendingTasks();
+
+  EXPECT_FALSE(base::PathExists(remote_bound_dir));
+}
+
 TEST_F(WebRtcEventLogManagerTestUploadSuppressionDisablingFlag,
        UploadingNotSuppressedByActivePeerConnections) {
   SuppressUploading();