Fix removal of old entries from chrome://webrtc-logs

When WebRTC textual log files are deleted, either because of
expiration or because the user clears browsing history,
entries from the logs' index are now also removed.

As a drive-by:
* Fixed the unbounded reading of a file into memory.
* Marked with TODOs (and filed bugs) for several other
  problems found.

Bug: 825977, 827131
Change-Id: If34acf6290ec90c3ce99c0704c58bc8a8188f92d
Reviewed-on: https://chromium-review.googlesource.com/982055Reviewed-by: Henrik Grunell <grunell@chromium.org>
Commit-Queue: Elad Alon <eladalon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548455}

components/webrtc_logging/browser/log_cleanup.cc

@@ -25,16 +25,21 @@ const int kDaysToKeepLogs = 5;
 // Remove any empty entries from the log list. One line is one log entry, see
 // WebRtcLogUploader::AddLocallyStoredLogInfoToUploadListFile for more
 // information about the format.
-void RemoveEmptyEntriesInLogList(std::string* log_list) {
-  static const char kEmptyLine[] = ",,\n";
+void RemoveEmptyEntriesFromLogList(std::string* log_list) {
+  // TODO(crbug.com/826253): Make this more robust to errors; corrupt entries
+  // should also be removed. (Better to move away from a .csv altogether.)
+  static const char kEmptyLineStart[] = ",,,";  // And a timestamp after it.
   size_t pos = 0;
   do {
-    pos = log_list->find(kEmptyLine, pos);
+    pos = log_list->find(kEmptyLineStart, pos);
     if (pos == std::string::npos)
       break;
-    DCHECK(pos == 0 || (*log_list)[pos - 1] == '\n');
-    log_list->erase(pos, arraysize(kEmptyLine) - 1);
-  } while (true);
+    const size_t line_end = log_list->find("\n", pos);
+    DCHECK(line_end == std::string::npos || pos < line_end);
+    const size_t delete_len =
+        line_end == std::string::npos ? std::string::npos : line_end - pos + 1;
+    log_list->erase(pos, delete_len);
+  } while (pos < log_list->size());
 }
 
 }  // namespace
@@ -62,10 +67,18 @@ void DeleteOldAndRecentWebRtcLogFiles(const base::FilePath& log_dir,
   std::string log_list;
   const bool update_log_list = base::PathExists(log_list_path);
   if (update_log_list) {
-    bool read_ok = base::ReadFileToString(log_list_path, &log_list);
-    DPCHECK(read_ok);
+    constexpr size_t kMaxIndexSizeBytes = 1000000;  // Intentional overshot.
+    const bool read_ok = base::ReadFileToStringWithMaxSize(
+        log_list_path, &log_list, kMaxIndexSizeBytes);
+    if (!read_ok) {
+      // If the maximum size was exceeded, updating it will corrupt it. However,
+      // the size would not be exceeded unless the user edits it manually.
+      LOG(ERROR) << "Couldn't read WebRTC textual logs list (" << log_list_path
+                 << ").";
+    }
   }
 
+  // Delete relevant logs files (and their associated entries in the index).
   base::FileEnumerator log_files(log_dir, false, base::FileEnumerator::FILES);
   bool delete_ok = true;
   for (base::FilePath name = log_files.Next(); !name.empty();
@@ -73,6 +86,8 @@ void DeleteOldAndRecentWebRtcLogFiles(const base::FilePath& log_dir,
     if (name == log_list_path)
       continue;
     base::FileEnumerator::FileInfo file_info(log_files.GetInfo());
+    // TODO(crbug.com/827167): Handle mismatch between timestamps of the .gz
+    // file and the .meta file, as well as with the index.
     base::TimeDelta file_age = now - file_info.GetLastModifiedTime();
     if (file_age > time_to_keep_logs ||
         (!delete_begin_time.is_max() &&
@@ -93,7 +108,11 @@ void DeleteOldAndRecentWebRtcLogFiles(const base::FilePath& log_dir,
   if (!delete_ok)
     LOG(WARNING) << "Could not delete all old WebRTC logs.";
 
-  RemoveEmptyEntriesInLogList(&log_list);
+  // TODO(crbug.com/826254): Purge index file separately, too, to ensure
+  // entries for logs whose files were manually removed, are also subject
+  // to expiry and browsing data clearing.
+
+  RemoveEmptyEntriesFromLogList(&log_list);
 
   if (update_log_list) {
     int written = base::WriteFile(log_list_path, &log_list[0], log_list.size());

components/webrtc_logging/browser/log_cleanup.h

@@ -12,14 +12,19 @@ class Time;
 
 namespace webrtc_logging {
 
-// Deletes logs files older that 5 days. Updates the log file list. Must be
-// called on the FILE thread.
+// Deletes logs files older that 5 days. Updates the log file list.
+// Must be called on a task runner that's allowed to block.
+// TODO(crbug.com/826221): Only call on the same task runner as where writing
+// is done.
 void DeleteOldWebRtcLogFiles(const base::FilePath& log_dir);
 
 // Deletes logs files older that 5 days and logs younger than
 // |delete_begin_time|. Updates the log file list. If |delete_begin_time| is
 // base::time::Max(), no recent logs will be deleted, and the function is
-// equal to DeleteOldWebRtcLogFiles(). Must be called on the FILE thread.
+// equal to DeleteOldWebRtcLogFiles().
+// Must be called on a task runner that's allowed to block.
+// TODO(crbug.com/826221): Only call on the same task runner as where writing
+// is done.
 void DeleteOldAndRecentWebRtcLogFiles(const base::FilePath& log_dir,
                                       const base::Time& delete_begin_time);
 

components/webrtc_logging/browser/log_cleanup_unittest.cc

@@ -73,4 +73,6 @@ TEST_F(WebRtcLogCleanupTest, DeleteOldAndRecentWebRtcLogFiles) {
   VerifyFiles(1);
 }
 
+// TODO(crbug.com/826251): Write tests for the index file.
+
 }  // namespace webrtc_logging