Fix feedback uploader crash with raw pointer misuse

This fixes a bug in the uploader where it would post a task with a
callback that had a raw pointer, and if it was during shutdown then the
referenced object could be destructed before the call on said pointer
was invoked.  The simple part of the change is using a WeakPtr in that
location instead of a raw pointer, so the callback will be skipped.
However, the way the code worked was that it would read the pre-existing
feedback report from disk to a string, then it would pass that into the
callback which asynchronously generated a new feedback report in a new
disk location and then it would delete the old feedback report file.
However, if that callback is suppressed (which it would be in the case
the new WeakPtr becomes invalidated) then we would end up deleting that
feedback report from disk without sending it. So this code also changes
things to not recreate the report on disk but instead reuses that
existing file for the feedback report.

Bug: 994846
Test: components_unittests pass and manually verified report upload
Change-Id: Ib64ce1abbfea23e94e45387fa5a296817bf19a30
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1766766
Commit-Queue: Jeffrey Kardatzke <jkardatzke@google.com>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#691752}

chrome/browser/feedback/feedback_profile_observer.cc

@@ -51,11 +51,11 @@ void FeedbackProfileObserver::Observe(
 }
 
 void FeedbackProfileObserver::QueueSingleReport(
-    feedback::FeedbackUploader* uploader,
-    std::unique_ptr<std::string> data) {
+    base::WeakPtr<feedback::FeedbackUploader> uploader,
+    scoped_refptr<FeedbackReport> report) {
   base::PostTask(FROM_HERE, {BrowserThread::UI},
-                 base::BindOnce(&FeedbackUploaderChrome::QueueReport,
-                                uploader->AsWeakPtr(), std::move(data)));
+                 base::BindOnce(&FeedbackUploaderChrome::RequeueReport,
+                                std::move(uploader), std::move(report)));
 }
 
 void FeedbackProfileObserver::QueueUnsentReports(
@@ -67,7 +67,7 @@ void FeedbackProfileObserver::QueueUnsentReports(
                                 uploader->feedback_reports_path(),
                                 base::BindRepeating(
                                     &FeedbackProfileObserver::QueueSingleReport,
-                                    uploader)));
+                                    uploader->AsWeakPtr())));
 }
 
 }  // namespace feedback

chrome/browser/feedback/feedback_profile_observer.h

@@ -5,8 +5,10 @@
 #ifndef CHROME_BROWSER_FEEDBACK_FEEDBACK_PROFILE_OBSERVER_H_
 #define CHROME_BROWSER_FEEDBACK_FEEDBACK_PROFILE_OBSERVER_H_
 
+#include "base/files/file_path.h"
 #include "base/lazy_instance.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 
@@ -16,6 +18,7 @@ class BrowserContext;
 
 namespace feedback {
 
+class FeedbackReport;
 class FeedbackUploader;
 
 // FeedbackProfileObserver waits on profile creation notifications to check
@@ -40,8 +43,9 @@ class FeedbackProfileObserver : public content::NotificationObserver {
   // the given browser context.
   void QueueUnsentReports(content::BrowserContext* context);
 
-  static void QueueSingleReport(feedback::FeedbackUploader* uploader,
-                                std::unique_ptr<std::string> data);
+  static void QueueSingleReport(
+      base::WeakPtr<feedback::FeedbackUploader> uploader,
+      scoped_refptr<FeedbackReport> report);
 
   // Used to track creation of profiles so we can load any unsent reports
   // for that profile.

components/feedback/feedback_report.cc

@@ -12,6 +12,7 @@
 #include "base/guid.h"
 #include "base/sequenced_task_runner.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/threading/thread_task_runner_handle.h"
 
 namespace feedback {
 
@@ -57,6 +58,12 @@ FeedbackReport::FeedbackReport(
                      base::WrapRefCounted<FeedbackReport>(this)));
 }
 
+FeedbackReport::FeedbackReport(
+    base::FilePath path,
+    std::unique_ptr<std::string> data,
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+    : file_(path), data_(std::move(data)), reports_task_runner_(task_runner) {}
+
 // static
 const char FeedbackReport::kCrashReportIdsKey[]  = "crash_report_ids";
 
@@ -77,9 +84,11 @@ void FeedbackReport::LoadReportsAndQueue(const base::FilePath& user_dir,
        !name.empty();
        name = enumerator.Next()) {
     auto data = std::make_unique<std::string>();
-    if (ReadFileToString(name, data.get()))
-      callback.Run(std::move(data));
-    base::DeleteFile(name, false);
+    if (ReadFileToString(name, data.get())) {
+      callback.Run(base::MakeRefCounted<FeedbackReport>(
+          std::move(name), std::move(data),
+          base::ThreadTaskRunnerHandle::Get()));
+    }
   }
 }
 

components/feedback/feedback_report.h

@@ -11,29 +11,35 @@
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/sequenced_task_runner.h"
 #include "base/time/time.h"
 
-namespace base {
-class SequencedTaskRunner;
-}
-
 namespace feedback {
 
+class FeedbackReport;
+
 // Repeating since for every feedback report file on disk, the callback to
 // queue it in the uploader needs to be invoked.
 using QueueCallback =
-    base::RepeatingCallback<void(std::unique_ptr<std::string>)>;
+    base::RepeatingCallback<void(scoped_refptr<FeedbackReport>)>;
 
 // This class holds a feedback report. Once a report is created, a disk backup
 // for it is created automatically. This backup needs to explicitly be
 // deleted by calling DeleteReportOnDisk.
 class FeedbackReport : public base::RefCountedThreadSafe<FeedbackReport> {
  public:
+  // Creates a new feedback report with the contents of |data|.
   FeedbackReport(const base::FilePath& path,
                  const base::Time& upload_at,
                  std::unique_ptr<std::string> data,
                  scoped_refptr<base::SequencedTaskRunner> task_runner);
 
+  // Creates a feedback report from an existing one on-disk at |path|, the
+  // |upload_at| time should be set after construction.
+  FeedbackReport(base::FilePath path,
+                 std::unique_ptr<std::string> data,
+                 scoped_refptr<base::SequencedTaskRunner> task_runner);
+
   // The ID of the product specific data for the crash report IDs as stored by
   // the feedback server.
   static const char kCrashReportIdsKey[];
@@ -54,6 +60,9 @@ class FeedbackReport : public base::RefCountedThreadSafe<FeedbackReport> {
   const base::Time& upload_at() const { return upload_at_; }
   void set_upload_at(const base::Time& time) { upload_at_ = time; }
   const std::string& data() const { return *data_; }
+  scoped_refptr<base::SequencedTaskRunner> reports_task_runner() const {
+    return reports_task_runner_;
+  }
 
  private:
   friend class base::RefCountedThreadSafe<FeedbackReport>;

components/feedback/feedback_uploader.cc

@@ -82,7 +82,17 @@ void FeedbackUploader::SetMinimumRetryDelayForTesting(base::TimeDelta delay) {
 }
 
 void FeedbackUploader::QueueReport(std::unique_ptr<std::string> data) {
-  QueueReportWithDelay(std::move(data), base::TimeDelta());
+  reports_queue_.emplace(base::MakeRefCounted<FeedbackReport>(
+      feedback_reports_path_, base::Time::Now(), std::move(data),
+      task_runner_));
+  UpdateUploadTimer();
+}
+
+void FeedbackUploader::RequeueReport(scoped_refptr<FeedbackReport> report) {
+  DCHECK_EQ(task_runner_, report->reports_task_runner());
+  report->set_upload_at(base::Time::Now());
+  reports_queue_.emplace(std::move(report));
+  UpdateUploadTimer();
 }
 
 void FeedbackUploader::StartDispatchingReport() {
@@ -249,12 +259,4 @@ void FeedbackUploader::UpdateUploadTimer() {
   }
 }
 
-void FeedbackUploader::QueueReportWithDelay(std::unique_ptr<std::string> data,
-                                            base::TimeDelta delay) {
-  reports_queue_.emplace(base::MakeRefCounted<FeedbackReport>(
-      feedback_reports_path_, base::Time::Now() + delay, std::move(data),
-      task_runner_));
-  UpdateUploadTimer();
-}
-
 }  // namespace feedback

components/feedback/feedback_uploader.h

@@ -50,6 +50,9 @@ class FeedbackUploader : public KeyedService,
   // Queues a report for uploading.
   void QueueReport(std::unique_ptr<std::string> data);
 
+  // Re-queues an existing report from disk for uploading.
+  void RequeueReport(scoped_refptr<FeedbackReport> report);
+
   bool QueueEmpty() const { return reports_queue_.empty(); }
 
   content::BrowserContext* context() { return context_; }
@@ -113,9 +116,6 @@ class FeedbackUploader : public KeyedService,
   // Update our timer for uploading the next report.
   void UpdateUploadTimer();
 
-  void QueueReportWithDelay(std::unique_ptr<std::string> data,
-                            base::TimeDelta delay);
-
   // URLLoaderFactory used for network requests.
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
 

components/feedback/feedback_uploader_unittest.cc

@@ -59,7 +59,8 @@ class MockFeedbackUploader : public FeedbackUploader {
         base::BindOnce(
             &FeedbackReport::LoadReportsAndQueue, feedback_reports_path(),
             base::BindRepeating(&MockFeedbackUploader::QueueSingleReport,
-                                base::SequencedTaskRunnerHandle::Get(), this)));
+                                base::SequencedTaskRunnerHandle::Get(),
+                                AsWeakPtr())));
   }
 
   const std::map<std::string, unsigned int>& dispatched_reports() const {
@@ -71,11 +72,11 @@ class MockFeedbackUploader : public FeedbackUploader {
  private:
   static void QueueSingleReport(
       scoped_refptr<base::SequencedTaskRunner> main_task_runner,
-      MockFeedbackUploader* uploader,
-      std::unique_ptr<std::string> data) {
+      base::WeakPtr<FeedbackUploader> uploader,
+      scoped_refptr<FeedbackReport> report) {
     main_task_runner->PostTask(
-        FROM_HERE, base::BindOnce(&MockFeedbackUploader::QueueReport,
-                                  uploader->AsWeakPtr(), std::move(data)));
+        FROM_HERE, base::BindOnce(&MockFeedbackUploader::RequeueReport,
+                                  std::move(uploader), std::move(report)));
   }
 
   // FeedbackUploaderChrome: