Chromecast: Android crash startup can't read path from /data/.

chromecast/crash/android/cast_crash_reporter_client_android.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromecast/crash/android/cast_crash_reporter_client_android.h"
 
 #include "base/base_paths.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/path_service.h"
 #include "chromecast/android/chromecast_config_android.h"
 #include "chromecast/base/version.h"
 #include "chromecast/common/global_descriptors.h"
 #include "chromecast/crash/cast_crash_keys.h"
 #include "content/public/common/content_switches.h"
 
 namespace chromecast {
 
-CastCrashReporterClientAndroid::CastCrashReporterClientAndroid() {
+CastCrashReporterClientAndroid::CastCrashReporterClientAndroid(bool privileged)
+    : privileged_(privileged) {

[lcwu1, 2015/06/12 01:42:01]

I feel like it is not very clear what 'privileged_' means here in the context of
CrashReportedClient. It seems that 'is_browser_process_' is clearer. In fact,
instead of using a boolean member, 'privileged_', why not simply pass in and
save 'process_type' which is used more commonly in this context (e.g. in
CastCrashReporterClient or in EnableBreakpadForProcess() method). This way, it
also gives us the flexibility/ability to do something based on different process
types.

Another benefit of passing in process_type is that we would then use
process_type_.empty() in line 51, and because of the comment in line 49-50, it's
clear that process_type_.empty() means it is a browser process. This would
address Stephen's comment.

 }
 
 CastCrashReporterClientAndroid::~CastCrashReporterClientAndroid() {
 }
 
 void CastCrashReporterClientAndroid::GetProductNameAndVersion(
     const char** product_name,
     const char** version) {
   *product_name = "media_shell";
   *version = PRODUCT_VERSION
 #if CAST_IS_DEBUG_BUILD()
       ".debug"
 #endif
       "." CAST_BUILD_REVISION;
 }
 
 base::FilePath CastCrashReporterClientAndroid::GetReporterLogFilename() {
   return base::FilePath(FILE_PATH_LITERAL("uploads.log"));
 }
 
 bool CastCrashReporterClientAndroid::GetCrashDumpLocation(
     base::FilePath* crash_dir) {
   base::FilePath crash_dir_local;
   if (!PathService::Get(base::DIR_ANDROID_APP_DATA, &crash_dir_local)) {
     return false;
   }
   crash_dir_local = crash_dir_local.Append("crashes");
 
-  if (!base::DirectoryExists(crash_dir_local)) {
-    if (!base::CreateDirectory(crash_dir_local)) {
-      return false;
+  // Only try to create the directory in a privileged process. For other
+  // processes, assume the directory exists.

[halliwell, 2015/06/10 21:23:16]

Do we need to make that assumption?  Wouldn't it be more accurate to check if
directory exists first (if not, and not privileged, then return false)?

[gunsch, 2015/06/10 21:30:16]

Renderer process is not allowed to check if file exists, even within app data
directory.

[gunsch, 2015/06/12 20:07:52]

I was hoping to abstract over the "process type" checks (which are tied to the
content model) and just have a "privileged process" flag, but I suppose this
file already checks content-specific process types (EnableBreakpadForProcess
below).

So process_type it is.

+  if (privileged_) {
+    if (!base::DirectoryExists(crash_dir_local)) {
+      if (!base::CreateDirectory(crash_dir_local)) {
+        return false;
+      }
     }
   }
 
   // Provide value to crash_dir once directory is known to be a valid path.
   *crash_dir = crash_dir_local;
   return true;
 }
 
 size_t CastCrashReporterClientAndroid::RegisterCrashKeys() {
   return crash_keys::RegisterCastCrashKeys();
 }
 
 bool CastCrashReporterClientAndroid::GetCollectStatsConsent() {
   return android::ChromecastConfigAndroid::GetInstance()->CanSendUsageStats();
 }
 
 int CastCrashReporterClientAndroid::GetAndroidMinidumpDescriptor() {
   return kAndroidMinidumpDescriptor;
 }
 
 bool CastCrashReporterClientAndroid::EnableBreakpadForProcess(
     const std::string& process_type) {
   return process_type == switches::kRendererProcess ||
          process_type == switches::kGpuProcess;
 }
 
 }  // namespace chromecast