Fix WebRtcEventLogUploaderImpl::Cancel()

Prior to this CL, Cancel() returned a confusing boolean, which was
supposed to be used to indicate whether the `WebRtcEventLogUploaderImpl`
could be deleted directly, or if the caller to Cancel() was supposed to
wait for the completion callback to be posted back to it.

Instead, we now always wait for the completion callback to be
posted back.

Note that we also introduce a check to ensure the callback belongs
to the currently uploaded file, rather than to the previously
uploaded file.

Bug: 1092071
Change-Id: I7dd92d806bef540125bcafb49b57d763783c64eb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2260637
Commit-Queue: Elad Alon <eladalon@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#781977}

chrome/browser/media/webrtc/webrtc_event_log_manager_remote.cc

@@ -1187,18 +1187,9 @@ void WebRtcRemoteEventLogManager::MaybeCancelUpload(
     return;
   }
 
-  // Cancel the upload.
-  // * If the upload has asynchronously completed by now, the uploader would
-  //   have posted a task back to our queue to delete it and move on to the
-  //   next file; cancellation is reported as unsuccessful in that case. In that
-  //   case, we avoid resetting |uploader_| until that callback task executes.
-  // * If the upload was still underway when we cancelled it, then we can
-  //   safely reset |uploader_| and move on to the next file the next time
-  //   ManageUploadSchedule() is called.
-  const bool cancelled = uploader_->Cancel();
-  if (cancelled) {
-    uploader_.reset();
-  }
+  // Cancel the upload. `uploader_` will be released when the callback,
+  // `OnWebRtcEventLogUploadComplete`, is posted back.
+  uploader_->Cancel();
 }
 
 bool WebRtcRemoteEventLogManager::MatchesFilter(
@@ -1318,6 +1309,7 @@ void WebRtcRemoteEventLogManager::MaybeStartUploading() {
     // TODO(crbug.com/775415): Rename the file before uploading, so that we
     // would not retry the upload after restarting Chrome, if the upload is
     // interrupted.
+    currently_uploaded_file_ = pending_logs_.begin()->path;
     uploader_ =
         uploader_factory_->Create(*pending_logs_.begin(), std::move(callback));
     pending_logs_.erase(pending_logs_.begin());
@@ -1331,7 +1323,20 @@ void WebRtcRemoteEventLogManager::OnWebRtcEventLogUploadComplete(
     bool upload_successful) {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
   DCHECK(uploader_);
+
+  // Make sure this callback refers to the currently uploaded file. This might
+  // not be the case if the upload was cancelled right after succeeding, in
+  // which case we'll get two callbacks, one reporting success and one failure.
+  // It can also be that the uploader was cancelled more than once, e.g. if
+  // the user cleared cache while PrefService were changing.
+  if (!uploader_ ||
+      uploader_->GetWebRtcLogFileInfo().path != currently_uploaded_file_) {
+    return;
+  }
+
   uploader_.reset();
+  currently_uploaded_file_.clear();
+
   ManageUploadSchedule();
 }
 

chrome/browser/media/webrtc/webrtc_event_log_manager_remote.h

@@ -443,6 +443,12 @@ class WebRtcRemoteEventLogManager final
   // currently busy uploading it to a remote server.
   std::unique_ptr<WebRtcEventLogUploader> uploader_;
 
+  // The path to the file which is currently being uploaded.
+  // Used to ensure a callback from the uploader refers to the current
+  // file, rather than a second callback from the previously uploaded file,
+  // e.g. when when Cancel() is called right after the upload finishes.
+  base::FilePath currently_uploaded_file_;
+
   // Provides notifications of network changes.
   network::NetworkConnectionTracker* network_connection_tracker_;
 

chrome/browser/media/webrtc/webrtc_event_log_manager_unittest.cc

@@ -189,14 +189,9 @@ class NullWebRtcEventLogUploader : public WebRtcEventLogUploader {
     return log_file_;
   }
 
-  bool Cancel() override {
+  void Cancel() override {
     EXPECT_TRUE(cancellation_expected_);
-    if (was_cancelled_) {  // Should not be called more than once.
-      EXPECT_TRUE(false);
-      return false;
-    }
     was_cancelled_ = true;
-    return true;
   }
 
   class Factory : public WebRtcEventLogUploader::Factory {
@@ -1345,9 +1340,8 @@ class FileListExpectingWebRtcEventLogUploader : public WebRtcEventLogUploader {
     return log_file_;
   }
 
-  bool Cancel() override {
+  void Cancel() override {
     NOTREACHED() << "Incompatible with this kind of test.";
-    return true;
   }
 
  private:
@@ -4357,7 +4351,7 @@ TEST_F(WebRtcEventLogManagerTestPolicy,
 
   ASSERT_TRUE(base::PathExists(*log_file));  // Test sanity; exists before.
 
-  allow_remote_logging = !allow_remote_logging;
+  allow_remote_logging = false;
   profile->GetPrefs()->SetBoolean(prefs::kWebRtcEventLogCollectionAllowed,
                                   allow_remote_logging);
 

chrome/browser/media/webrtc/webrtc_event_log_uploader.cc

@@ -212,27 +212,25 @@ const WebRtcLogFileInfo& WebRtcEventLogUploaderImpl::GetWebRtcLogFileInfo()
   return log_file_;
 }
 
-bool WebRtcEventLogUploaderImpl::Cancel() {
+void WebRtcEventLogUploaderImpl::Cancel() {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
 
-  // The upload could already have been completed, or maybe was never properly
-  // started (due to a file read failure, etc.).
-  const bool upload_was_active = (url_loader_.get() != nullptr);
+  if (url_loader_.get() == nullptr) {
+    // Either the upload has already completed, or it never properly started.
+    return;
+  }
 
-  // Note that in this case, it might still be that the last bytes hit the
-  // wire right as we attempt to cancel the upload. OnURLFetchComplete, however,
-  // will not be called.
+  // Stop the upload.
   url_loader_.reset();
 
-  DeleteLogFile();
-  DeleteHistoryFile();
-
-  if (upload_was_active) {
-    UmaRecordWebRtcEventLoggingUpload(
-        WebRtcEventLoggingUploadUma::kUploadCancelled);
-  }
-
-  return upload_was_active;
+  // Note edge case - the upload might on very rare occasions already have
+  // finished, with the on-complete callback pending on the queue.
+  // In those very rare occasions, we will record the UMA for cancellation
+  // as well as the success/failure of the upload. Also, we'll post to replies
+  // back to the owner of this WebRtcEventLogUploaderImpl object.
+  UmaRecordWebRtcEventLoggingUpload(
+      WebRtcEventLoggingUploadUma::kUploadCancelled);
+  ReportResult(/*upload_successful=*/false, /*delete_history_file=*/true);
 }
 
 bool WebRtcEventLogUploaderImpl::PrepareUploadData(std::string* upload_data) {
@@ -344,7 +342,8 @@ void WebRtcEventLogUploaderImpl::OnURLLoadComplete(
   ReportResult(upload_successful);
 }
 
-void WebRtcEventLogUploaderImpl::ReportResult(bool result) {
+void WebRtcEventLogUploaderImpl::ReportResult(bool upload_successful,
+                                              bool delete_history_file) {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
 
   // * If the upload was successful, the file is no longer needed.
@@ -356,11 +355,16 @@ void WebRtcEventLogUploaderImpl::ReportResult(bool result) {
   // TODO(crbug.com/775415): Provide refined retrial behavior.
   DeleteLogFile();
 
+  if (delete_history_file) {
+    DeleteHistoryFile();
+  }
+
   // Release hold of history file, allowing it to be read, moved or deleted.
   history_file_writer_.reset();
 
   task_runner_->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback_), log_file_.path, result));
+      FROM_HERE,
+      base::BindOnce(std::move(callback_), log_file_.path, upload_successful));
 }
 
 void WebRtcEventLogUploaderImpl::DeleteLogFile() {

chrome/browser/media/webrtc/webrtc_event_log_uploader.h

@@ -37,12 +37,12 @@ class WebRtcEventLogUploader {
    public:
     virtual ~Factory() = default;
 
-    // Creates uploaders. The observer is passed to each call of Create,
-    // rather than be memorized by the factory's constructor, because factories
-    // created by unit tests have no visibility into the real implementation's
-    // observer (WebRtcRemoteEventLogManager).
+    // Creates uploaders.
     // This takes ownership of the file. The caller must not attempt to access
-    // the file after invoking Create().
+    // the file after invoking Create(). Even deleting the file due to
+    // the user clearing cache, is to be done through the uploader's Cancel,
+    // and not through direct access of the caller to the file. The file may
+    // be touched again only after |this| is destroyed.
     virtual std::unique_ptr<WebRtcEventLogUploader> Create(
         const WebRtcLogFileInfo& log_file,
         UploadResultCallback callback) = 0;
@@ -55,10 +55,9 @@ class WebRtcEventLogUploader {
   virtual const WebRtcLogFileInfo& GetWebRtcLogFileInfo() const = 0;
 
   // Cancels the upload, then deletes the log file and its history file.
-  // Returns true if the upload was cancelled due to this call, and false if
-  // the upload was already completed or aborted before this call.
-  // (Aborted uploads are ones where the file could not be read, etc.)
-  virtual bool Cancel() = 0;
+  // (These files are deleted even if the upload has been completed by the time
+  // Cancel is called.)
+  virtual void Cancel() = 0;
 };
 
 // Primary implementation of WebRtcEventLogUploader. Uploads log files to crash.
@@ -96,7 +95,7 @@ class WebRtcEventLogUploaderImpl : public WebRtcEventLogUploader {
 
   const WebRtcLogFileInfo& GetWebRtcLogFileInfo() const override;
 
-  bool Cancel() override;
+  void Cancel() override;
 
  private:
   friend class WebRtcEventLogUploaderImplTest;
@@ -116,7 +115,7 @@ class WebRtcEventLogUploaderImpl : public WebRtcEventLogUploader {
   void OnURLLoadComplete(std::unique_ptr<std::string> response_body);
 
   // Cleanup and posting of the result callback.
-  void ReportResult(bool result);
+  void ReportResult(bool upload_successful, bool delete_history_file = false);
 
   // Remove the log file which is owned by |this|.
   void DeleteLogFile();
@@ -148,7 +147,7 @@ class WebRtcEventLogUploaderImpl : public WebRtcEventLogUploader {
   // This object is in charge of the actual upload.
   std::unique_ptr<network::SimpleURLLoader> url_loader_;
 
-  // Allows releasing `this` while a task from `url_loader_` is still pending.
+  // Allows releasing |this| while a task from |url_loader_| is still pending.
   base::WeakPtrFactory<WebRtcEventLogUploaderImpl> weak_ptr_factory_{this};
 };
 

chrome/browser/media/webrtc/webrtc_event_log_uploader_impl_unittest.cc

@@ -28,6 +28,7 @@
 
 namespace webrtc_event_logging {
 
+using ::testing::_;
 using ::testing::StrictMock;
 using BrowserContextId = WebRtcEventLogPeerConnectionKey::BrowserContextId;
 
@@ -277,31 +278,34 @@ TEST_F(WebRtcEventLogUploaderImplTest, ExcessivelyLargeFilesNotUploaded) {
 }
 
 TEST_F(WebRtcEventLogUploaderImplTest,
-       CancelBeforeUploadCompletionReturnsTrue) {
+       CancelBeforeUploadCompletionCallsCallbackWithFalse) {
   const base::Time last_modified = base::Time::Now();
   StartUploadThatWillNotTerminate(browser_context_id_, last_modified);
-
-  EXPECT_TRUE(uploader_->Cancel());
+  EXPECT_CALL(observer_, CompletionCallback(log_file_, false)).Times(1);
+  uploader_->Cancel();
 }
 
-TEST_F(WebRtcEventLogUploaderImplTest, CancelOnCancelledUploadReturnsFalse) {
+TEST_F(WebRtcEventLogUploaderImplTest, SecondCallToCancelHasNoEffect) {
   const base::Time last_modified = base::Time::Now();
   StartUploadThatWillNotTerminate(browser_context_id_, last_modified);
 
-  ASSERT_TRUE(uploader_->Cancel());
-  EXPECT_FALSE(uploader_->Cancel());
+  EXPECT_CALL(observer_, CompletionCallback(log_file_, _)).Times(1);
+
+  uploader_->Cancel();
+  uploader_->Cancel();
 }
 
 TEST_F(WebRtcEventLogUploaderImplTest,
-       CancelAfterUploadCompletionReturnsFalse) {
+       CancelAfterUploadCompletionCallbackWasCalledHasNoEffect) {
   SetURLLoaderResponse(net::HTTP_OK, net::OK);
   EXPECT_CALL(observer_, CompletionCallback(log_file_, true)).Times(1);
   StartAndWaitForUpload();
 
-  EXPECT_FALSE(uploader_->Cancel());
+  EXPECT_CALL(observer_, CompletionCallback(_, _)).Times(0);
+  uploader_->Cancel();
 }
 
-TEST_F(WebRtcEventLogUploaderImplTest, CancelOnAbortedUploadReturnsFalse) {
+TEST_F(WebRtcEventLogUploaderImplTest, CancelOnAbortedUploadHasNoEffect) {
   // Show the failure was independent of the URLLoaderFactory's primed return
   // value.
   SetURLLoaderResponse(net::HTTP_OK, net::OK);
@@ -310,13 +314,17 @@ TEST_F(WebRtcEventLogUploaderImplTest, CancelOnAbortedUploadReturnsFalse) {
   EXPECT_CALL(observer_, CompletionCallback(log_file_, false)).Times(1);
   StartAndWaitForUpload();
 
-  EXPECT_FALSE(uploader_->Cancel());
+  EXPECT_CALL(observer_, CompletionCallback(_, _)).Times(0);
+  uploader_->Cancel();
 }
 
 TEST_F(WebRtcEventLogUploaderImplTest, CancelOnOngoingUploadDeletesFile) {
   const base::Time last_modified = base::Time::Now();
   StartUploadThatWillNotTerminate(browser_context_id_, last_modified);
-  ASSERT_TRUE(uploader_->Cancel());
+
+  EXPECT_CALL(observer_, CompletionCallback(log_file_, false)).Times(1);
+  uploader_->Cancel();
+  observer_run_loop_.Run();
 
   EXPECT_FALSE(base::PathExists(log_file_));
 }
@@ -332,7 +340,8 @@ TEST_F(WebRtcEventLogUploaderImplTest,
   EXPECT_EQ(info.last_modified, last_modified);
 
   // Test tear-down.
-  ASSERT_TRUE(uploader_->Cancel());
+  EXPECT_CALL(observer_, CompletionCallback(log_file_, false)).Times(1);
+  uploader_->Cancel();
 }
 
 TEST_F(WebRtcEventLogUploaderImplTest,
@@ -353,7 +362,8 @@ TEST_F(WebRtcEventLogUploaderImplTest,
        GetWebRtcLogFileInfoReturnsCorrectInfoWhenCalledOnCancelledUpload) {
   const base::Time last_modified = base::Time::Now();
   StartUploadThatWillNotTerminate(browser_context_id_, last_modified);
-  ASSERT_TRUE(uploader_->Cancel());
+  EXPECT_CALL(observer_, CompletionCallback(log_file_, false)).Times(1);
+  uploader_->Cancel();
 
   const WebRtcLogFileInfo info = uploader_->GetWebRtcLogFileInfo();
   EXPECT_EQ(info.browser_context_id, browser_context_id_);