Add JavaScript error reports to chrome://crashes view

Ensure that JavaScript error reports are added to chrome://crashes when
running on Linux.

NOTE: This only works on Breakpad at the moment. The Crashpad database
doesn't allow Chrome to add entries to the database.

BUG=chromium:1162356,chromium:1121816

Change-Id: If89f56526b0b1f6c2c33c399b37d4c14cf1d0756
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2613927Reviewed-by: Miriam Zimmerman <mutexlox@chromium.org>
Commit-Queue: Ian Barkley-Yeung <iby@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841242}

chrome/browser/error_reporting/BUILD.gn

@@ -14,10 +14,12 @@ static_library("error_reporting") {
   deps = [
     "//base",
     "//build:chromeos_buildflags",
+    "//chrome/common:constants",
     "//components/crash/content/browser/error_reporting",
     "//components/crash/core/app",
     "//components/feedback",
     "//components/startup_metric_utils/browser",
+    "//components/upload_list",
     "//content/public/browser",
     "//net",
     "//services/network:network_service",
@@ -34,6 +36,9 @@ source_set("test_support") {
   deps = [
     ":error_reporting",
     "//base",
+    "//base/test:test_support",
+    "//build:chromeos_buildflags",
+    "//chrome/common:constants",
     "//components/crash/content/browser/error_reporting:mock_crash_endpoint",
   ]
 }
@@ -49,6 +54,7 @@ source_set("unit_test") {
     "//components/crash/content/browser/error_reporting",
     "//components/crash/content/browser/error_reporting:mock_crash_endpoint",
     "//components/crash/core/app",
+    "//components/upload_list",
     "//content/test:test_support",
     "//net:test_support",
     "//testing/gtest",

chrome/browser/error_reporting/chrome_js_error_report_processor.cc

@@ -10,8 +10,11 @@
 
 #include "base/callback.h"
 #include "base/callback_helpers.h"
+#include "base/files/file.h"
+#include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/memory/scoped_refptr.h"
+#include "base/path_service.h"
 #include "base/strings/strcat.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
@@ -20,11 +23,13 @@
 #include "base/task/thread_pool.h"
 #include "base/time/default_clock.h"
 #include "build/build_config.h"
-#include "build/chromeos_buildflags.h"
+#include "chrome/common/chrome_paths.h"
 #include "components/crash/content/browser/error_reporting/javascript_error_report.h"
 #include "components/crash/core/app/client_upload_info.h"
+#include "components/crash/core/app/crashpad.h"
 #include "components/feedback/redaction_tool.h"
 #include "components/startup_metric_utils/browser/startup_metric_utils.h"
+#include "components/upload_list/crash_upload_list.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
@@ -100,17 +105,58 @@ ChromeJsErrorReportProcessor::ChromeJsErrorReportProcessor()
     : clock_(base::DefaultClock::GetInstance()) {}
 ChromeJsErrorReportProcessor::~ChromeJsErrorReportProcessor() = default;
 
+#if !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_CHROMEOS_LACROS)
+void ChromeJsErrorReportProcessor::UpdateReportDatabase(
+    std::string remote_report_id,
+    base::Time report_time) {
+  // Uploads.log format is "seconds_since_epoch,crash_id\n"
+  base::FilePath crash_dir_path;
+  if (!base::PathService::Get(chrome::DIR_CRASH_DUMPS, &crash_dir_path)) {
+    VLOG(1) << "Nowhere to write uploads.log";
+    return;
+  }
+  base::FilePath upload_log_path =
+      crash_dir_path.AppendASCII(CrashUploadList::kReporterLogFilename);
+  base::File upload_log(upload_log_path,
+                        base::File::FLAG_OPEN_ALWAYS | base::File::FLAG_APPEND);
+  if (!upload_log.IsValid()) {
+    VLOG(1) << "Could not open upload.log: "
+            << base::File::ErrorToString(upload_log.error_details());
+    return;
+  }
+  std::string line = base::StrCat({base::NumberToString(report_time.ToTimeT()),
+                                   ",", remote_report_id, "\n"});
+  // WriteAtCurrentPos because O_APPEND.
+  if (upload_log.WriteAtCurrentPos(line.c_str(), line.length()) !=
+      static_cast<int>(line.length())) {
+    VLOG(1) << "Could not write to upload.log";
+    return;
+  }
+}
+#endif  // !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_CHROMEOS_LACROS)
+
 void ChromeJsErrorReportProcessor::OnRequestComplete(
     std::unique_ptr<network::SimpleURLLoader> url_loader,
     base::ScopedClosureRunner callback_runner,
+    base::Time report_time,
     std::unique_ptr<std::string> response_body) {
   if (response_body) {
-    // TODO(iby): Update the crash log (uploads.log)
     VLOG(1) << "Uploaded crash report. ID: " << *response_body;
+    // On Chrome OS, we use a different format than other platforms. Since we
+    // will soon not call this function at all on Chrome OS (crbug.com/986166),
+    // don't bother writing code to write to that format.
+#if !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_CHROMEOS_LACROS)
+    base::ThreadPool::PostTaskAndReply(
+        FROM_HERE, {base::MayBlock()},
+        base::BindOnce(&ChromeJsErrorReportProcessor::UpdateReportDatabase,
+                       this, *response_body, report_time),
+        callback_runner.Release());
+#endif  // !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_CHROMEOS_LACROS)
   } else {
     LOG(ERROR) << "Failed to upload crash report";
   }
-  // callback_runner will implicitly run the callback when we reach this line.
+  // callback_runner may implicitly run the callback when we reach this line if
+  // we didn't add a task to update the report database.
 }
 
 // Returns the redacted, fixed-up error report if the user consented to have it
@@ -166,6 +212,7 @@ void ChromeJsErrorReportProcessor::SendReport(
     const GURL& url,
     const std::string& body,
     base::ScopedClosureRunner callback_runner,
+    base::Time report_time,
     network::SharedURLLoaderFactory* loader_factory) {
   auto resource_request = std::make_unique<network::ResourceRequest>();
   resource_request->method = "POST";
@@ -220,7 +267,8 @@ void ChromeJsErrorReportProcessor::SendReport(
   loader->DownloadToString(
       loader_factory,
       base::BindOnce(&ChromeJsErrorReportProcessor::OnRequestComplete, this,
-                     std::move(url_loader), std::move(callback_runner)),
+                     std::move(url_loader), std::move(callback_runner),
+                     report_time),
       kCrashEndpointResponseMaxSizeInBytes);
 }
 
@@ -229,6 +277,7 @@ void ChromeJsErrorReportProcessor::OnConsentCheckCompleted(
     base::ScopedClosureRunner callback_runner,
     scoped_refptr<network::SharedURLLoaderFactory> loader_factory,
     base::TimeDelta browser_process_uptime,
+    base::Time report_time,
     base::Optional<JavaScriptErrorReport> error_report) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (!error_report) {
@@ -292,7 +341,8 @@ void ChromeJsErrorReportProcessor::OnConsentCheckCompleted(
     body = std::move(*error_report->stack_trace);
   }
 
-  SendReport(url, body, std::move(callback_runner), loader_factory.get());
+  SendReport(url, body, std::move(callback_runner), report_time,
+             loader_factory.get());
 }
 
 void ChromeJsErrorReportProcessor::CheckAndUpdateRecentErrorReports(
@@ -417,7 +467,8 @@ void ChromeJsErrorReportProcessor::SendErrorReport(
                      std::move(error_report)),
       base::BindOnce(&ChromeJsErrorReportProcessor::OnConsentCheckCompleted,
                      this, std::move(callback_runner),
-                     std::move(loader_factory), browser_process_uptime));
+                     std::move(loader_factory), browser_process_uptime,
+                     clock_->Now()));
 }
 
 std::string ChromeJsErrorReportProcessor::GetCrashEndpoint() {

chrome/browser/error_reporting/chrome_js_error_report_processor.h

@@ -15,6 +15,7 @@
 #include "base/containers/flat_map.h"
 #include "base/time/clock.h"
 #include "base/time/time.h"
+#include "build/chromeos_buildflags.h"
 #include "components/crash/content/browser/error_reporting/js_error_report_processor.h"
 
 namespace content {
@@ -68,11 +69,20 @@ class ChromeJsErrorReportProcessor : public JsErrorReportProcessor {
                             int32_t& os_minor_version,
                             int32_t& os_bugfix_version);
 
+#if !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_CHROMEOS_LACROS)
+  // Update the uploads.log file with a record of this error report. This
+  // ensures that the error appears on chrome://crashes and is listed in the
+  // feedback reports.
+  virtual void UpdateReportDatabase(std::string remote_report_id,
+                                    base::Time report_time);
+#endif  // !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_CHROMEOS_LACROS)
+
  private:
   struct PlatformInfo;
 
   void OnRequestComplete(std::unique_ptr<network::SimpleURLLoader> url_loader,
                          base::ScopedClosureRunner callback_runner,
+                         base::Time report_time,
                          std::unique_ptr<std::string> response_body);
 
   base::Optional<JavaScriptErrorReport> CheckConsentAndRedact(
@@ -83,12 +93,14 @@ class ChromeJsErrorReportProcessor : public JsErrorReportProcessor {
   void SendReport(const GURL& url,
                   const std::string& body,
                   base::ScopedClosureRunner callback_runner,
+                  base::Time report_time,
                   network::SharedURLLoaderFactory* loader_factory);
 
   void OnConsentCheckCompleted(
       base::ScopedClosureRunner callback_runner,
       scoped_refptr<network::SharedURLLoaderFactory> loader_factory,
       base::TimeDelta browser_process_uptime,
+      base::Time report_time,
       base::Optional<JavaScriptErrorReport> error_report);
 
   // To avoid spamming the error collection system, do not send duplicate

chrome/browser/error_reporting/chrome_js_error_report_processor_unittest.cc

@@ -9,12 +9,18 @@
 
 #include "base/callback.h"
 #include "base/callback_helpers.h"
+#include "base/strings/strcat.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/test/simple_test_clock.h"
 #include "build/build_config.h"
+#include "chrome/browser/crash_upload_list/crash_upload_list.h"
 #include "chrome/browser/error_reporting/mock_chrome_js_error_report_processor.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/crash/content/browser/error_reporting/javascript_error_report.h"
 #include "components/crash/content/browser/error_reporting/mock_crash_endpoint.h"
+#include "components/crash/core/app/crashpad.h"
+#include "components/upload_list/upload_list.h"
 #include "content/public/test/browser_task_environment.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -38,7 +44,7 @@ class ChromeJsErrorReportProcessorTest : public ::testing::Test {
 
   void SetUp() override {
     // Set clock to something arbitrary which is not the null value.
-    test_clock_.SetNow(base::Time::FromTimeT(1586581472));
+    test_clock_.SetNow(base::Time::FromTimeT(kFakeNow));
     test_server_ = std::make_unique<net::test_server::EmbeddedTestServer>();
     endpoint_ = std::make_unique<MockCrashEndpoint>(test_server_.get());
     processor_->SetCrashEndpoint(endpoint_->GetCrashEndpointURL());
@@ -73,6 +79,7 @@ class ChromeJsErrorReportProcessorTest : public ::testing::Test {
   bool finish_callback_was_called_ = false;
   scoped_refptr<MockChromeJsErrorReportProcessor> processor_;
 
+  static constexpr time_t kFakeNow = 1586581472;
   static constexpr char kFirstMessage[] = "An Error Is Me";
   static constexpr char kFirstMessageQuery[] =
       "error_message=An%20Error%20Is%20Me";
@@ -85,6 +92,7 @@ class ChromeJsErrorReportProcessorTest : public ::testing::Test {
   static constexpr char kSecondProduct[] = "Chrome_Linux";
 };
 
+constexpr time_t ChromeJsErrorReportProcessorTest::kFakeNow;
 constexpr char ChromeJsErrorReportProcessorTest::kFirstMessage[];
 constexpr char ChromeJsErrorReportProcessorTest::kFirstMessageQuery[];
 constexpr char ChromeJsErrorReportProcessorTest::kSecondMessage[];
@@ -421,3 +429,82 @@ TEST_F(ChromeJsErrorReportProcessorTest, DifferentColumnNumbersAreDistinct) {
   SendErrorReport(std::move(report3));
   EXPECT_EQ(endpoint_->report_count(), 3);
 }
+
+#if !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_CHROMEOS_LACROS)
+static std::string UploadInfoStateToString(
+    UploadList::UploadInfo::State state) {
+  switch (state) {
+    case UploadList::UploadInfo::State::NotUploaded:
+      return "NotUploaded";
+    case UploadList::UploadInfo::State::Pending:
+      return "Pending";
+    case UploadList::UploadInfo::State::Pending_UserRequested:
+      return "Pending_UserRequested";
+    case UploadList::UploadInfo::State::Uploaded:
+      return "Uploaded";
+    default:
+      return base::StrCat({"Unknown upload state ",
+                           base::NumberToString(static_cast<int>(state))});
+  }
+}
+
+static std::string UploadInfoVectorToString(
+    const std::vector<UploadList::UploadInfo>& uploads) {
+  std::string result = "[";
+  bool first = true;
+  for (const UploadList::UploadInfo& upload : uploads) {
+    if (first) {
+      first = false;
+    } else {
+      result += ", ";
+    }
+    base::StrAppend(&result,
+                    {"{state ", UploadInfoStateToString(upload.state),
+                     ", upload_id ", upload.upload_id, ", upload_time ",
+                     base::NumberToString(upload.upload_time.ToTimeT()),
+                     ", local_id ", upload.local_id, ", capture_time ",
+                     base::NumberToString(upload.capture_time.ToTimeT()),
+                     ", source ", upload.source, ", file size ",
+                     base::UTF16ToUTF8(upload.file_size), "}"});
+  }
+  result += "]";
+  return result;
+}
+
+TEST_F(ChromeJsErrorReportProcessorTest, UpdatesUploadsLog) {
+  if (crash_reporter::IsCrashpadEnabled()) {
+    // TODO(crbug.com/1162356): Combine uploads.log with Crashpad database when
+    // getting list of crashes.
+    GTEST_SKIP();
+  }
+
+  base::ScopedPathOverride crash_dir_override(chrome::DIR_CRASH_DUMPS);
+  processor_->set_update_report_database(true);
+
+  constexpr char kCrashId[] = "123abc456def";
+  endpoint_->set_response(net::HTTP_OK, kCrashId);
+
+  SendErrorReport(MakeErrorReport(kFirstMessage));
+  EXPECT_EQ(endpoint_->report_count(), 1);
+
+  auto upload_list = CreateCrashUploadList();
+  base::RunLoop run_loop;
+  upload_list->Load(run_loop.QuitClosure());
+  run_loop.Run();
+  std::vector<UploadList::UploadInfo> uploads;
+  upload_list->GetUploads(50, &uploads);
+  EXPECT_EQ(uploads.size(), 1U) << UploadInfoVectorToString(uploads);
+
+  bool found = false;
+  for (const UploadList::UploadInfo& upload : uploads) {
+    if (upload.state == UploadList::UploadInfo::State::Uploaded &&
+        upload.upload_id == kCrashId) {
+      EXPECT_FALSE(found) << "Found twice";
+      found = true;
+      EXPECT_EQ(upload.upload_time.ToTimeT(), kFakeNow);
+    }
+  }
+  EXPECT_TRUE(found) << "Didn't find upload record in "
+                     << UploadInfoVectorToString(uploads);
+}
+#endif  // !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_CHROMEOS_LACROS)

chrome/browser/error_reporting/mock_chrome_js_error_report_processor.cc

@@ -9,6 +9,7 @@
 #include "components/crash/content/browser/error_reporting/mock_crash_endpoint.h"
 
 MockChromeJsErrorReportProcessor::MockChromeJsErrorReportProcessor() = default;
+
 MockChromeJsErrorReportProcessor::~MockChromeJsErrorReportProcessor() = default;
 
 void MockChromeJsErrorReportProcessor::SetAsDefault() {
@@ -50,6 +51,17 @@ void MockChromeJsErrorReportProcessor::GetOsVersion(
   os_bugfix_version = 1;
 }
 
+#if !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_CHROMEOS_LACROS)
+void MockChromeJsErrorReportProcessor::UpdateReportDatabase(
+    std::string remote_report_id,
+    base::Time report_time) {
+  if (update_report_database_) {
+    ChromeJsErrorReportProcessor::UpdateReportDatabase(
+        std::move(remote_report_id), report_time);
+  }
+}
+#endif  //  !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_CHROMEOS_LACROS)
+
 ScopedMockChromeJsErrorReportProcessor::ScopedMockChromeJsErrorReportProcessor(
     const MockCrashEndpoint& endpoint)
     : processor_(base::MakeRefCounted<MockChromeJsErrorReportProcessor>()),

chrome/browser/error_reporting/mock_chrome_js_error_report_processor.h

@@ -7,9 +7,12 @@
 
 #include <stdint.h>
 
+#include <memory>
 #include <string>
 
 #include "base/memory/scoped_refptr.h"
+#include "base/test/scoped_path_override.h"
+#include "build/chromeos_buildflags.h"
 #include "chrome/browser/error_reporting/chrome_js_error_report_processor.h"
 
 class MockCrashEndpoint;
@@ -31,6 +34,17 @@ class MockChromeJsErrorReportProcessor : public ChromeJsErrorReportProcessor {
   // return the given (other) JsErrorReportProcessor.
   static void SetDefaultTo(scoped_refptr<JsErrorReportProcessor> new_default);
 
+#if !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_CHROMEOS_LACROS)
+  // By default, a MockChromeJsErrorReportProcessor will suppress the updating
+  // of the crash database (a.k.a. uploads.log) to avoid contaminating the real
+  // database with test uploads. Set |update_report_database| to true to have
+  // ChromeJsErrorReportProcessor::UpdateReportDatabase called like it normally
+  // would be.
+  void set_update_report_database(bool update_report_database) {
+    update_report_database_ = update_report_database;
+  }
+#endif
+
  protected:
   std::string GetCrashEndpoint() override;
   std::string GetCrashEndpointStaging() override;
@@ -40,10 +54,18 @@ class MockChromeJsErrorReportProcessor : public ChromeJsErrorReportProcessor {
                     int32_t& os_minor_version,
                     int32_t& os_bugfix_version) override;
 
+#if !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_CHROMEOS_LACROS)
+  void UpdateReportDatabase(std::string remote_report_id,
+                            base::Time report_time) override;
+#endif
+
  private:
   ~MockChromeJsErrorReportProcessor() override;
   std::string crash_endpoint_;
   std::string crash_endpoint_staging_;
+#if !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_CHROMEOS_LACROS)
+  bool update_report_database_ = false;
+#endif
 };
 
 // Wrapper for MockChromeJsErrorReportProcessor. Will automatically create, set
@@ -63,6 +85,8 @@ class ScopedMockChromeJsErrorReportProcessor {
   // JsErrorReportProcessor::Get() will return nullptr.
   ~ScopedMockChromeJsErrorReportProcessor();
 
+  MockChromeJsErrorReportProcessor& processor() const { return *processor_; }
+
  private:
   scoped_refptr<MockChromeJsErrorReportProcessor> processor_;
   scoped_refptr<JsErrorReportProcessor> previous_;

components/crash/content/browser/error_reporting/mock_crash_endpoint.cc

@@ -89,8 +89,8 @@ MockCrashEndpoint::HandleRequest(const net::test_server::HttpRequest& request) {
   ++report_count_;
   last_report_ = Report(absolute_url.query(), request.content);
   auto http_response = std::make_unique<net::test_server::BasicHttpResponse>();
-  http_response->set_code(net::HTTP_OK);
-  http_response->set_content("123");
+  http_response->set_code(response_code_);
+  http_response->set_content(response_content_);
   http_response->set_content_type("text/plain");
   if (on_report_) {
     on_report_.Run();

components/crash/content/browser/error_reporting/mock_crash_endpoint.h

@@ -10,6 +10,7 @@
 
 #include "base/callback.h"
 #include "base/optional.h"
+#include "net/http/http_status_code.h"
 #include "url/gurl.h"
 
 namespace net {
@@ -51,6 +52,12 @@ class MockCrashEndpoint {
   // submitting crash reports.
   void set_consented(bool consented) { consented_ = consented; }
 
+  // Set the response that the server will return.
+  void set_response(net::HttpStatusCode code, const std::string& content) {
+    response_code_ = code;
+    response_content_ = content;
+  }
+
   // Returns the URL that tests should send crash reports to.
   std::string GetCrashEndpointURL() const;
 
@@ -66,6 +73,8 @@ class MockCrashEndpoint {
   int report_count_ = 0;
   bool consented_ = true;
   base::RepeatingClosure on_report_;
+  net::HttpStatusCode response_code_ = net::HTTP_OK;
+  std::string response_content_ = "123";
 };
 
 #endif  // COMPONENTS_CRASH_CONTENT_BROWSER_ERROR_REPORTING_MOCK_CRASH_ENDPOINT_H_