Chromecast: adds crash handling for Android build.

Chromecast: adds crash handling for Android build.

Cast for Android TV behavior includes the following:
* On crash, immediately attempt to upload crash dump + logs from
  current process, then delete the dump file.
* On startup, attempt to upload any crash dump files that are
  still left on the device.

R=thestig@chromium.org,byungchul@chromium.org
BUG=400876
Committed: https://crrev.com/168c44ce90012516dcfa91ac80a6d0a200513531
Cr-Commit-Position: refs/heads/master@{#298767}

[gunsch, 2014-09-30 21:33:21]

FYI:
* Byungchul to review; already reviewed internally, just reworking for TOT.
Largest change is "components/breakpad" -> "components/crash" and relevant
renames.
* Lei to review Chromecast's use of breakpad and components/crash code.

Thanks.

[byungchul, 2014-09-30 21:51:27]

Enable breakpad for android first?

[gunsch, 2014-09-30 22:00:23]

On 2014/09/30 21:51:27, byungchul wrote:
> Enable breakpad for android first?

Yes, is that a problem?

There is essentially no shared code between our Linux and Android breakpad use;
I spent some time looking into it but the functionality is fairly distinct, due
primarily to:
* Non-trivial differences in the Android/Linux breakpad component code,
particularly how Android tries really hard not to upload immediately, and
* The Chromecast implementation handling crash uploading out-of-process

[byungchul, 2014-09-30 22:46:43]

lgtm

[Lei Zhang, 2014-10-06 22:44:25]

I didn't review the Java code.

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
File chromecast/crash/android/cast_crash_reporter_client_android.cc (right):

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/cast_crash_reporter_client_android.cc:8: #include
"base/command_line.h"
nit: not used.

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/cast_crash_reporter_client_android.cc:12: #include
"base/strings/string16.h"
not used?

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/cast_crash_reporter_client_android.cc:13: #include
"base/strings/utf_string_conversions.h"
nit: not used.

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/cast_crash_reporter_client_android.cc:15: #include
"chromecast/common/chromecast_switches.h"
also not needed?

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/cast_crash_reporter_client_android.cc:45:
PathService::Get(base::DIR_ANDROID_APP_DATA, crash_dir);
Can't this or CreateDirectory() below fail?

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/cast_crash_reporter_client_android.cc:66: process_type
== switches::kZygoteProcess ||
You're saying yes to the zygote process here, but there's an "if (process_type
!= switches::kZygoteProcess)" condition in CrashHandler::CrashHandler() that
skips initialization for the zygote.

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
File chromecast/crash/android/cast_crash_reporter_client_android.h (right):

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/cast_crash_reporter_client_android.h:22: std::string*
version) OVERRIDE;
I think we are in the middle of switching from OVERRIDE to override. Please
don't add more of the obsolete version.

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
File chromecast/crash/android/crash_handler.cc (right):

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/crash_handler.cc:13: #include "base/lazy_instance.h"
nit: not used

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/crash_handler.cc:30: bool HandleCrash(const void*,
size_t, void*) {
nit: you can write in the parameter names but comment them out:
bool HandleCrash(const void* /* foo */, size_t /* bar */, void* /* qux */) {

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/crash_handler.cc:50:
CHECK(crash_reporter_client_->GetCrashDumpLocation(&crash_dump_path_));
Do you have to CHECK() here?

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/crash_handler.cc:99: LOG(INFO) << "Attempting to upload
current process crash";
No LOG(INFO) please. Use VLOG() if you want to keep this.

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/crash_handler.cc:126: LOG(INFO) << "Removing crash
dumps instead of uploading";
ditto

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
File chromecast/crash/android/crash_handler.h (right):

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/crash_handler.h:46: static bool
RegisterCastCrashJni(JNIEnv* env);
nit: move this up with the other static methods.

https://codereview.chromium.org/620673003/diff/20001/chromecast/shell/app/cas...
File chromecast/shell/app/cast_main_delegate.cc (right):

https://codereview.chromium.org/620673003/diff/20001/chromecast/shell/app/cas...
chromecast/shell/app/cast_main_delegate.cc:71:
chromecast::CrashHandler::Initialize(process_type, log_file);
Can CrashHandler handle an empty |log_file| ?

https://codereview.chromium.org/620673003/diff/20001/chromecast/shell/browser...
File chromecast/shell/browser/cast_browser_process.h (right):

https://codereview.chromium.org/620673003/diff/20001/chromecast/shell/browser...
chromecast/shell/browser/cast_browser_process.h:39: #if defined(OS_ANDROID)
nit: maybe move this to after the other setters?

[Lei Zhang, 2014-10-07 18:57:14]

BTW, https://codereview.chromium.org/633833002/ conflicts slightly.

[gunsch, 2014-10-09 00:20:56]

Lei: thanks for the thorough review. PTAL.

I rebased past https://codereview.chromium.org/633833002/ in patch set 3, and
then made addressed your comments in patch set 4.

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
File chromecast/crash/android/cast_crash_reporter_client_android.cc (right):

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/cast_crash_reporter_client_android.cc:8: #include
"base/command_line.h"
On 2014/10/06 22:44:25, Lei Zhang wrote:
> nit: not used.

Done.

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/cast_crash_reporter_client_android.cc:12: #include
"base/strings/string16.h"
On 2014/10/06 22:44:24, Lei Zhang wrote:
> not used?

Done.

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/cast_crash_reporter_client_android.cc:13: #include
"base/strings/utf_string_conversions.h"
On 2014/10/06 22:44:24, Lei Zhang wrote:
> nit: not used.

Done.

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/cast_crash_reporter_client_android.cc:15: #include
"chromecast/common/chromecast_switches.h"
On 2014/10/06 22:44:24, Lei Zhang wrote:
> also not needed?

Done.

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/cast_crash_reporter_client_android.cc:45:
PathService::Get(base::DIR_ANDROID_APP_DATA, crash_dir);
On 2014/10/06 22:44:25, Lei Zhang wrote:
> Can't this or CreateDirectory() below fail?

Done.

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/cast_crash_reporter_client_android.cc:66: process_type
== switches::kZygoteProcess ||
On 2014/10/06 22:44:24, Lei Zhang wrote:
> You're saying yes to the zygote process here, but there's an "if (process_type
> != switches::kZygoteProcess)" condition in CrashHandler::CrashHandler() that
> skips initialization for the zygote.

Done.

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
File chromecast/crash/android/cast_crash_reporter_client_android.h (right):

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/cast_crash_reporter_client_android.h:22: std::string*
version) OVERRIDE;
On 2014/10/06 22:44:25, Lei Zhang wrote:
> I think we are in the middle of switching from OVERRIDE to override. Please
> don't add more of the obsolete version.

Done.

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
File chromecast/crash/android/crash_handler.cc (right):

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/crash_handler.cc:13: #include "base/lazy_instance.h"
On 2014/10/06 22:44:25, Lei Zhang wrote:
> nit: not used

Done.

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/crash_handler.cc:30: bool HandleCrash(const void*,
size_t, void*) {
On 2014/10/06 22:44:25, Lei Zhang wrote:
> nit: you can write in the parameter names but comment them out:
> bool HandleCrash(const void* /* foo */, size_t /* bar */, void* /* qux */) {

Done.

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/crash_handler.cc:50:
CHECK(crash_reporter_client_->GetCrashDumpLocation(&crash_dump_path_));
On 2014/10/06 22:44:25, Lei Zhang wrote:
> Do you have to CHECK() here?

I'll change it to a LOG(ERROR) --- but I don't necessarily have a good way to
handle the case when it fails.

That said, we have yet to see a crash from this location in production.

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/crash_handler.cc:99: LOG(INFO) << "Attempting to upload
current process crash";
On 2014/10/06 22:44:25, Lei Zhang wrote:
> No LOG(INFO) please. Use VLOG() if you want to keep this.

Done (this isn't terribly important), but in general chromecast/ uses a fair bit
of LOG(INFO). No code from chromecast/ is linked into the chrome binary, so our
LOG(INFO) is primarily for the Chromecast product's binary itself, and we have a
presubmit exception to the LOG(INFO) restriction for that reason.

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/crash_handler.cc:126: LOG(INFO) << "Removing crash
dumps instead of uploading";
On 2014/10/06 22:44:25, Lei Zhang wrote:
> ditto

Done.

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
File chromecast/crash/android/crash_handler.h (right):

https://codereview.chromium.org/620673003/diff/20001/chromecast/crash/android...
chromecast/crash/android/crash_handler.h:46: static bool
RegisterCastCrashJni(JNIEnv* env);
On 2014/10/06 22:44:25, Lei Zhang wrote:
> nit: move this up with the other static methods.

Done.

https://codereview.chromium.org/620673003/diff/20001/chromecast/shell/app/cas...
File chromecast/shell/app/cast_main_delegate.cc (right):

https://codereview.chromium.org/620673003/diff/20001/chromecast/shell/app/cas...
chromecast/shell/app/cast_main_delegate.cc:71:
chromecast::CrashHandler::Initialize(process_type, log_file);
On 2014/10/06 22:44:25, Lei Zhang wrote:
> Can CrashHandler handle an empty |log_file| ?

I believe that should be okay, we haven't had any problem with this code running
it in the last several months. We use the same file path when initializing
logging with it in BasicStartupComplete.

https://codereview.chromium.org/620673003/diff/20001/chromecast/shell/browser...
File chromecast/shell/browser/cast_browser_process.h (right):

https://codereview.chromium.org/620673003/diff/20001/chromecast/shell/browser...
chromecast/shell/browser/cast_browser_process.h:39: #if defined(OS_ANDROID)
On 2014/10/06 22:44:25, Lei Zhang wrote:
> nit: maybe move this to after the other setters?

Done.

[Lei Zhang, 2014-10-09 00:26:37]

lgtm

[gunsch, 2014-10-09 00:31:56]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-10-09 00:38:04]

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

[commit-bot: I haz the power, 2014-10-09 03:50:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-09 03:50:14]

Exceeded time limit waiting for builds to trigger.

[gunsch, 2014-10-09 03:51:32]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-10-09 03:53:07]

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

[commit-bot: I haz the power, 2014-10-09 04:37:14]

Committed patchset #4 (id:60001) as 8b52df73f4536866a1835ba485f3e1553fba8219

[commit-bot: I haz the power, 2014-10-09 04:38:15]

Patchset 4 (id:??) landed as
https://crrev.com/168c44ce90012516dcfa91ac80a6d0a200513531
Cr-Commit-Position: refs/heads/master@{#298767}