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

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

R=slan@chromium.org,halliwell@chromium.org,lcwu@chromium.org
BUG=internal b/21732115

Committed: https://crrev.com/b49702b06743ec66928a04e5ad8c77764456ec6c
Cr-Commit-Position: refs/heads/master@{#334278}

[gunsch, 2015-06-10 18:09:59]

slan/halliwell to review, alokp for committer stamp

[slan, 2015-06-10 18:20:54]

https://codereview.chromium.org/1176003005/diff/1/chromecast/crash/android/cr...
File chromecast/crash/android/crash_handler.cc (right):

https://codereview.chromium.org/1176003005/diff/1/chromecast/crash/android/cr...
chromecast/crash/android/crash_handler.cc:81: new
CastCrashReporterClientAndroid(process_type.empty())) {
It's unclear to me why process_type.empty() implies a privileged process. Could
you add a short comment either here or in the header?

[gunsch, 2015-06-10 18:29:05]

https://codereview.chromium.org/1176003005/diff/1/chromecast/crash/android/cr...
File chromecast/crash/android/crash_handler.cc (right):

https://codereview.chromium.org/1176003005/diff/1/chromecast/crash/android/cr...
chromecast/crash/android/crash_handler.cc:81: new
CastCrashReporterClientAndroid(process_type.empty())) {
On 2015/06/10 18:20:54, slan wrote:
> It's unclear to me why process_type.empty() implies a privileged process.
Could
> you add a short comment either here or in the header?

It's not specific to us, but rather a convention throughout all the content/
code. For example, see:

https://code.google.com/p/chromium/codesearch#chromium/src/content/common/san...

or just search for "process_type.empty()"

[slan, 2015-06-10 18:53:33]

On 2015/06/10 18:29:05, gunsch wrote:
>
https://codereview.chromium.org/1176003005/diff/1/chromecast/crash/android/cr...
> File chromecast/crash/android/crash_handler.cc (right):
> 
>
https://codereview.chromium.org/1176003005/diff/1/chromecast/crash/android/cr...
> chromecast/crash/android/crash_handler.cc:81: new
> CastCrashReporterClientAndroid(process_type.empty())) {
> On 2015/06/10 18:20:54, slan wrote:
> > It's unclear to me why process_type.empty() implies a privileged process.
> Could
> > you add a short comment either here or in the header?
> 
> It's not specific to us, but rather a convention throughout all the content/
> code. For example, see:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/san...
> 
> or just search for "process_type.empty()"

sgtm. Thanks for the info.

[slan, 2015-06-10 18:53:44]

lgtm

[halliwell, 2015-06-10 21:23:17]

https://codereview.chromium.org/1176003005/diff/1/chromecast/crash/android/ca...
File chromecast/crash/android/cast_crash_reporter_client_android.cc (right):

https://codereview.chromium.org/1176003005/diff/1/chromecast/crash/android/ca...
chromecast/crash/android/cast_crash_reporter_client_android.cc:50: // processes,
assume the directory exists.
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]

https://codereview.chromium.org/1176003005/diff/1/chromecast/crash/android/ca...
File chromecast/crash/android/cast_crash_reporter_client_android.cc (right):

https://codereview.chromium.org/1176003005/diff/1/chromecast/crash/android/ca...
chromecast/crash/android/cast_crash_reporter_client_android.cc:50: // processes,
assume the directory exists.
On 2015/06/10 21:23:16, halliwell wrote:
> 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)?

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

[halliwell, 2015-06-10 21:32:45]

On 2015/06/10 21:30:16, gunsch wrote:
>
https://codereview.chromium.org/1176003005/diff/1/chromecast/crash/android/ca...
> File chromecast/crash/android/cast_crash_reporter_client_android.cc (right):
> 
>
https://codereview.chromium.org/1176003005/diff/1/chromecast/crash/android/ca...
> chromecast/crash/android/cast_crash_reporter_client_android.cc:50: //
processes,
> assume the directory exists.
> On 2015/06/10 21:23:16, halliwell wrote:
> > 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)?
> 
> Renderer process is not allowed to check if file exists, even within app data
> directory.

Doh, got it :)  lgtm.

[gunsch, 2015-06-12 00:20:07]

gunsch@chromium.org changed reviewers:
+ lcwu@chromium.org
- alokp@chromium.org

[gunsch, 2015-06-12 00:20:16]

+lcwu for committer stamp (alokp ooo)

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

https://codereview.chromium.org/1176003005/diff/1/chromecast/crash/android/ca...
File chromecast/crash/android/cast_crash_reporter_client_android.cc (right):

https://codereview.chromium.org/1176003005/diff/1/chromecast/crash/android/ca...
chromecast/crash/android/cast_crash_reporter_client_android.cc:20: :
privileged_(privileged) {
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.

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

https://codereview.chromium.org/1176003005/diff/1/chromecast/crash/android/ca...
File chromecast/crash/android/cast_crash_reporter_client_android.cc (right):

https://codereview.chromium.org/1176003005/diff/1/chromecast/crash/android/ca...
chromecast/crash/android/cast_crash_reporter_client_android.cc:50: // processes,
assume the directory exists.
On 2015/06/10 21:30:16, gunsch wrote:
> On 2015/06/10 21:23:16, halliwell wrote:
> > 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)?
> 
> Renderer process is not allowed to check if file exists, even within app data
> directory.

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.

[lcwu1, 2015-06-12 21:05:09]

lgtm

[gunsch, 2015-06-12 23:15:51]

The CQ bit was checked by gunsch@chromium.org

[gunsch, 2015-06-12 23:15:52]

The patchset sent to the CQ was uploaded after l-g-t-m from slan@chromium.org,
halliwell@chromium.org
Link to the patchset: https://codereview.chromium.org/1176003005/#ps20001
(title: "privileged flag --> process_type")

[commit-bot: I haz the power, 2015-06-12 23:16:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1176003005/20001

[commit-bot: I haz the power, 2015-06-12 23:30:28]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-06-12 23:31:13]

Patchset 2 (id:??) landed as
https://crrev.com/b49702b06743ec66928a04e5ad8c77764456ec6c
Cr-Commit-Position: refs/heads/master@{#334278}