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

chromecast/crash/android/cast_crash_reporter_client_android.cc

Index: chromecast/crash/android/cast_crash_reporter_client_android.cc
diff --git a/chromecast/crash/android/cast_crash_reporter_client_android.cc b/chromecast/crash/android/cast_crash_reporter_client_android.cc
index d914767e8501f8bd13c02325218f198b1aec24cf..3212174364d2cc34c2b99ea7cf07bab9c3fd4f82 100644
--- a/chromecast/crash/android/cast_crash_reporter_client_android.cc
+++ b/chromecast/crash/android/cast_crash_reporter_client_android.cc
@@ -16,7 +16,8 @@
 
 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() {
@@ -45,9 +46,13 @@ bool CastCrashReporterClientAndroid::GetCrashDumpLocation(
   }
   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;
+      }
     }
   }