[chromecast] Pass crash_pid to crash_reporter_client.

The existing code of GetProcessType() never works,
because it's always running in browser process.

Bug: internal b/128680778
Change-Id: I8ad7b2ccfdef036c05cbb4aeaf791b334c41d3ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1589010Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Luke Halliwell <halliwell@chromium.org>
Commit-Queue: Gaofeng Huang <gfhuang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#656429}

chromecast/app/linux/cast_crash_reporter_client.cc

@@ -15,7 +15,6 @@ namespace chromecast {
 
 namespace {
 
-std::string* g_process_type = nullptr;
 uint64_t g_process_start_time_ms = 0u;
 
 }  // namespace
@@ -23,31 +22,22 @@ uint64_t g_process_start_time_ms = 0u;
 // static
 void CastCrashReporterClient::InitCrashReporter(
     const std::string& process_type) {
-  DCHECK(!g_process_type);
+  DCHECK(!g_process_start_time_ms);
   g_process_start_time_ms =
       (base::TimeTicks::Now() - base::TimeTicks()).InMilliseconds();
 
-  // Save the process type (leaked).
-  g_process_type = new std::string(process_type);
-
   // Start up breakpad for this process, if applicable.
   breakpad::InitCrashReporter(process_type);
 }
 
-// static
-const char* CastCrashReporterClient::GetProcessType() {
-  return g_process_type ? g_process_type->c_str() : nullptr;
-}
-
 // static
 uint64_t CastCrashReporterClient::GetProcessStartTime() {
   return g_process_start_time_ms;
 }
 
-CastCrashReporterClient::CastCrashReporterClient() {
-}
-CastCrashReporterClient::~CastCrashReporterClient() {
-}
+CastCrashReporterClient::CastCrashReporterClient() {}
+
+CastCrashReporterClient::~CastCrashReporterClient() {}
 
 bool CastCrashReporterClient::EnableBreakpadForProcess(
     const std::string& process_type) {
@@ -57,18 +47,17 @@ bool CastCrashReporterClient::EnableBreakpadForProcess(
          process_type == switches::kUtilityProcess;
 }
 
-bool CastCrashReporterClient::HandleCrashDump(const char* crashdump_filename) {
+bool CastCrashReporterClient::HandleCrashDump(const char* crashdump_filename,
+                                              uint64_t crash_pid) {
   // Set the initial error code to ERROR_WEB_CONTENT_RENDER_VIEW_GONE to show
   // an error message on next cast_shell run. Though the error code is for
   // renderer process crash, the actual messages can be used for browser process
   // as well.
-  if (!GetProcessType() || !strcmp(GetProcessType(), ""))
-    SetInitialErrorCode(ERROR_WEB_CONTENT_RENDER_VIEW_GONE);
+  SetInitialErrorCode(ERROR_WEB_CONTENT_RENDER_VIEW_GONE);
 
   // Upload crash dump. If user didn't opt-in crash report, minidump writer
   // instantiated within CrashUtil::RequestUploadCrashDump() does nothing.
-  CrashUtil::RequestUploadCrashDump(crashdump_filename,
-                                    GetProcessType() ? GetProcessType() : "",
+  CrashUtil::RequestUploadCrashDump(crashdump_filename, crash_pid,
                                     GetProcessStartTime());
 
   // Always return true to indicate that this crash dump has been processed,

chromecast/app/linux/cast_crash_reporter_client.h

@@ -23,10 +23,10 @@ class CastCrashReporterClient : public crash_reporter::CrashReporterClient {
 
   // crash_reporter::CrashReporterClient implementation:
   bool EnableBreakpadForProcess(const std::string& process_type) override;
-  bool HandleCrashDump(const char* crashdump_filename) override;
+  bool HandleCrashDump(const char* crashdump_filename,
+                       uint64_t crash_pid) override;
 
  private:
-  static const char* GetProcessType();
   static uint64_t GetProcessStartTime();
 
   DISALLOW_COPY_AND_ASSIGN(CastCrashReporterClient);

chromecast/app/linux/cast_crash_reporter_client_unittest.cc

@@ -131,7 +131,7 @@ TEST_F(CastCrashReporterClientTest, EndToEndTestOnIORestrictedThread) {
   // Handle a "crash" on an IO restricted thread.
   base::ThreadRestrictions::SetIOAllowed(false);
   CastCrashReporterClient client;
-  ASSERT_TRUE(client.HandleCrashDump(minidump_path().value().c_str()));
+  ASSERT_TRUE(client.HandleCrashDump(minidump_path().value().c_str(), 0));
 
   // Assert that the thread is IO restricted when the function exits.
   // Note that SetIOAllowed returns the previous value.
@@ -143,7 +143,7 @@ TEST_F(CastCrashReporterClientTest, EndToEndTestOnNonIORestrictedThread) {
   // Handle a crash on a non-IO restricted thread.
   base::ThreadRestrictions::SetIOAllowed(true);
   CastCrashReporterClient client;
-  ASSERT_TRUE(client.HandleCrashDump(minidump_path().value().c_str()));
+  ASSERT_TRUE(client.HandleCrashDump(minidump_path().value().c_str(), 0));
 
   // Assert that the thread is not IO restricted when the function exits.
   // Note that SetIOAllowed returns the previous value.

chromecast/crash/linux/crash_util.cc

@@ -35,25 +35,22 @@ uint64_t CrashUtil::GetCurrentTimeMs() {
 // static
 bool CrashUtil::RequestUploadCrashDump(
     const std::string& existing_minidump_path,
-    const std::string& crashed_process_name,
+    uint64_t crashed_pid,
     uint64_t crashed_process_start_time_ms) {
   // Remove IO restrictions from this thread. Chromium IO functions must be used
   // to access the file system and upload information to the crash server.
   const bool io_allowed = base::ThreadRestrictions::SetIOAllowed(true);
 
   LOG(INFO) << "Request to upload crash dump " << existing_minidump_path
-            << " for process " << crashed_process_name;
+            << " for process " << crashed_pid;
 
   uint64_t uptime_ms = GetCurrentTimeMs() - crashed_process_start_time_ms;
-  MinidumpParams params(crashed_process_name,
-                        uptime_ms,
-                        "",  // suffix
-                        AppStateTracker::GetPreviousApp(),
-                        AppStateTracker::GetCurrentApp(),
-                        AppStateTracker::GetLastLaunchedApp(),
-                        CAST_BUILD_RELEASE,
-                        CAST_BUILD_INCREMENTAL,
-                        ""  /* reason */);
+  MinidumpParams params(
+      uptime_ms,
+      "",  // suffix
+      AppStateTracker::GetPreviousApp(), AppStateTracker::GetCurrentApp(),
+      AppStateTracker::GetLastLaunchedApp(), CAST_BUILD_RELEASE,
+      CAST_BUILD_INCREMENTAL, "" /* reason */);
   DummyMinidumpGenerator minidump_generator(existing_minidump_path);
 
   base::FilePath filename = base::FilePath(existing_minidump_path).BaseName();

chromecast/crash/linux/crash_util.h

@@ -18,7 +18,7 @@ class CrashUtil {
   // Helper function to request upload an existing minidump file. Returns true
   // on success, false otherwise.
   static bool RequestUploadCrashDump(const std::string& existing_minidump_path,
-                                     const std::string& crashed_process_name,
+                                     uint64_t crashed_pid,
                                      uint64_t crashed_process_start_time_ms);
 
   // Util function to get current time in ms. This is used to record

chromecast/crash/linux/dump_info.cc

@@ -18,7 +18,7 @@ namespace {
 // "%Y-%m-%d %H:%M:%S";
 const char kDumpTimeFormat[] = "%04d-%02d-%02d %02d:%02d:%02d";
 
-const int kNumRequiredParams = 5;
+const int kNumRequiredParams = 4;
 
 const char kNameKey[] = "name";
 const char kDumpTimeKey[] = "dump_time";
@@ -56,7 +56,6 @@ std::unique_ptr<base::Value> DumpInfo::GetAsValue() const {
       std::make_unique<base::DictionaryValue>();
   base::DictionaryValue* entry;
   result->GetAsDictionary(&entry);
-  entry->SetString(kNameKey, params_.process_name);
 
   base::Time::Exploded ex;
   dump_time_.LocalExplode(&ex);
@@ -91,9 +90,6 @@ bool DumpInfo::ParseEntry(const base::Value* entry) {
     return false;
 
   // Extract required fields.
-  if (!dict->GetString(kNameKey, &params_.process_name))
-    return false;
-
   std::string dump_time;
   if (!dict->GetString(kDumpTimeKey, &dump_time))
     return false;
@@ -116,6 +112,9 @@ bool DumpInfo::ParseEntry(const base::Value* entry) {
   size_t num_params = kNumRequiredParams;
 
   // Extract all other optional fields.
+  std::string unused_process_name;
+  if (dict->GetString(kNameKey, &unused_process_name))
+    ++num_params;
   if (dict->GetString(kSuffixKey, &params_.suffix))
     ++num_params;
   if (dict->GetString(kPrevAppNameKey, &params_.previous_app_name))

chromecast/crash/linux/dump_info_unittest.cc

@@ -41,7 +41,6 @@ TEST(DumpInfoTest, BadTimeStringIsNotValid) {
 TEST(DumpInfoTest, AllRequiredFieldsIsValid) {
   std::unique_ptr<DumpInfo> info(
       CreateDumpInfo("{"
-                     "\"name\": \"name\","
                      "\"dump_time\" : \"2001-11-12 18:31:01\","
                      "\"dump\": \"dump_string\","
                      "\"uptime\": \"123456789\","
@@ -58,7 +57,6 @@ TEST(DumpInfoTest, AllRequiredFieldsIsValid) {
   EXPECT_TRUE(base::Time::FromLocalExploded(ex, &dump_time));
 
   ASSERT_TRUE(info->valid());
-  ASSERT_EQ("name", info->params().process_name);
   ASSERT_EQ(dump_time, info->dump_time());
   ASSERT_EQ("dump_string", info->crashed_process_dump());
   ASSERT_EQ(123456789u, info->params().process_uptime);
@@ -97,7 +95,6 @@ TEST(DumpInfoTest, SomeRequiredFieldsEmptyIsValid) {
   EXPECT_TRUE(base::Time::FromLocalExploded(ex, &dump_time));
 
   ASSERT_TRUE(info->valid());
-  ASSERT_EQ("name", info->params().process_name);
   ASSERT_EQ(dump_time, info->dump_time());
   ASSERT_EQ("", info->crashed_process_dump());
   ASSERT_EQ(0u, info->params().process_uptime);
@@ -131,7 +128,6 @@ TEST(DumpInfoTest, AllOptionalFieldsIsValid) {
   EXPECT_TRUE(base::Time::FromLocalExploded(ex, &dump_time));
 
   ASSERT_TRUE(info->valid());
-  ASSERT_EQ("name", info->params().process_name);
   ASSERT_EQ(dump_time, info->dump_time());
   ASSERT_EQ("dump_string", info->crashed_process_dump());
   ASSERT_EQ(123456789u, info->params().process_uptime);
@@ -166,7 +162,6 @@ TEST(DumpInfoTest, SomeOptionalFieldsIsValid) {
   EXPECT_TRUE(base::Time::FromLocalExploded(ex, &dump_time));
 
   ASSERT_TRUE(info->valid());
-  ASSERT_EQ("name", info->params().process_name);
   ASSERT_EQ(dump_time, info->dump_time());
   ASSERT_EQ("dump_string", info->crashed_process_dump());
   ASSERT_EQ(123456789u, info->params().process_uptime);

chromecast/crash/linux/minidump_params.cc

@@ -6,8 +6,7 @@
 
 namespace chromecast {
 
-MinidumpParams::MinidumpParams(const std::string& p_process_name,
-                               const uint64_t p_process_uptime,
+MinidumpParams::MinidumpParams(const uint64_t p_process_uptime,
                                const std::string& p_suffix,
                                const std::string& p_previous_app_name,
                                const std::string& p_current_app_name,
@@ -15,19 +14,16 @@ MinidumpParams::MinidumpParams(const std::string& p_process_name,
                                const std::string& p_cast_release_version,
                                const std::string& p_cast_build_number,
                                const std::string& p_reason)
-    : process_name(p_process_name),
-      process_uptime(p_process_uptime),
+    : process_uptime(p_process_uptime),
       suffix(p_suffix),
       previous_app_name(p_previous_app_name),
       current_app_name(p_current_app_name),
       last_app_name(p_last_app_name),
       cast_release_version(p_cast_release_version),
       cast_build_number(p_cast_build_number),
-      reason(p_reason) {
-}
+      reason(p_reason) {}
 
-MinidumpParams::MinidumpParams() : process_uptime(0) {
-}
+MinidumpParams::MinidumpParams() : process_uptime(0) {}
 
 MinidumpParams::MinidumpParams(const MinidumpParams& params) = default;
 

chromecast/crash/linux/minidump_params.h

@@ -13,8 +13,7 @@ namespace chromecast {
 
 struct MinidumpParams {
   MinidumpParams();
-  MinidumpParams(const std::string& p_process_name,
-                 const uint64_t p_process_uptime,
+  MinidumpParams(const uint64_t p_process_uptime,
                  const std::string& p_suffix,
                  const std::string& p_previous_app_name,
                  const std::string& p_current_app_name,
@@ -25,7 +24,6 @@ struct MinidumpParams {
   MinidumpParams(const MinidumpParams& params);
   ~MinidumpParams();
 
-  std::string process_name;
   uint64_t process_uptime;
   std::string suffix;
   std::string previous_app_name;

chromecast/crash/linux/minidump_uploader_unittest.cc

@@ -97,7 +97,7 @@ class MinidumpUploaderTest : public testing::Test {
     // Must pass in non-empty MinidumpParams to circumvent the internal checks.
     std::unique_ptr<DumpInfo> dump(new DumpInfo(
         minidump_path.value(), logfile_path.value(), base::Time::Now(),
-        MinidumpParams("_", 0, "_", "_", "_", "_", "_", "_", "_")));
+        MinidumpParams(0, "_", "_", "_", "_", "_", "_", "_")));
 
     CHECK(AppendLockFile(lockfile_.value(), metadata_.value(), *dump));
     base::File minidump(

chromecast/crash/linux/synchronized_minidump_manager_unittest.cc

@@ -265,7 +265,6 @@ TEST_F(SynchronizedMinidumpManagerTest,
   // Sample parameters.
   base::Time now = base::Time::Now();
   MinidumpParams params;
-  params.process_name = "process";
 
   // Write the first entry.
   SynchronizedMinidumpManagerSimple manager;
@@ -294,7 +293,6 @@ TEST_F(SynchronizedMinidumpManagerTest, AcquireLockFile_WaitsForOtherThread) {
   // Create some parameters for a minidump.
   base::Time now = base::Time::Now();
   MinidumpParams params;
-  params.process_name = "process";
 
   // Create a manager that grabs the lock then sleeps. Post a DoWork task to
   // another thread. |sleepy_manager| will grab the lock and hold it for
@@ -344,7 +342,6 @@ TEST_F(SynchronizedMinidumpManagerTest,
   // Create some parameters for a minidump.
   base::Time now = base::Time::Now();
   MinidumpParams params;
-  params.process_name = "process";
 
   // Fork the process.
   pid_t pid = base::ForkWithFlags(0u, nullptr, nullptr);
@@ -388,7 +385,6 @@ TEST_F(SynchronizedMinidumpManagerTest,
   // Sample parameters.
   base::Time now = base::Time::Now();
   MinidumpParams params;
-  params.process_name = "process";
 
   FakeSynchronizedMinidumpUploader uploader;
   SynchronizedMinidumpManagerSimple producer;
@@ -404,7 +400,6 @@ TEST_F(SynchronizedMinidumpManagerTest, Upload_FailsWhenTooManyRecentDumps) {
   // Sample parameters.
   base::Time now = base::Time::Now();
   MinidumpParams params;
-  params.process_name = "process";
 
   FakeSynchronizedMinidumpUploader uploader;
   SynchronizedMinidumpManagerSimple producer;
@@ -424,7 +419,6 @@ TEST_F(SynchronizedMinidumpManagerTest, UploadSucceedsAfterRateLimitPeriodEnd) {
   // Sample parameters.
   base::Time now = base::Time::Now();
   MinidumpParams params;
-  params.process_name = "process";
 
   FakeSynchronizedMinidumpUploader uploader;
   SynchronizedMinidumpManagerSimple producer;
@@ -469,7 +463,6 @@ TEST_F(SynchronizedMinidumpManagerTest, HasDumpsWithDumps) {
   // Sample parameters.
   base::Time now = base::Time::Now();
   MinidumpParams params;
-  params.process_name = "process";
 
   SynchronizedMinidumpManagerSimple producer;
   FakeSynchronizedMinidumpUploader uploader;

components/crash/content/app/breakpad_linux.cc

@@ -1540,7 +1540,7 @@ void HandleCrashDump(const BreakpadInfo& info) {
   google_breakpad::PageAllocator allocator;
   const char* exe_buf = nullptr;
 
-  if (GetCrashReporterClient()->HandleCrashDump(info.filename)) {
+  if (GetCrashReporterClient()->HandleCrashDump(info.filename, info.pid)) {
     return;
   }
 

components/crash/content/app/crash_reporter_client.cc

@@ -101,7 +101,8 @@ base::FilePath CrashReporterClient::GetReporterLogFilename() {
   return base::FilePath();
 }
 
-bool CrashReporterClient::HandleCrashDump(const char* crashdump_filename) {
+bool CrashReporterClient::HandleCrashDump(const char* crashdump_filename,
+                                          uint64_t crash_pid) {
   return false;
 }
 #endif

components/crash/content/app/crash_reporter_client.h

@@ -109,7 +109,8 @@ class CrashReporterClient {
   // to fallback to default handler.
   // WARNING: this handler runs in a compromised context. It may not call into
   // libc nor allocate memory normally.
-  virtual bool HandleCrashDump(const char* crashdump_filename);
+  virtual bool HandleCrashDump(const char* crashdump_filename,
+                               uint64_t crash_pid);
 #endif
 
   // The location where minidump files should be written. Returns true if