Upstream Android native crash handling changes.

Upstream Android native crash handling changes.

This is actulaly the second time.

Revert "Revert 131404 - Upstream native crash handling changes for Android."

This reverts commit 421df96aab6267e963ddff16c9f738aa903a2cba.

Conflicts:

	chrome/browser/chrome_browser_main.cc

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=131593

[Lei Zhang, 2012-04-09 22:26:54]

(still looking)

Can you refer to http://codereview.chromium.org/9838033/ in the CL description?

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/app/breakpad_...
File chrome/app/breakpad_linuxish.cc (right):

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/app/breakpad_...
chrome/app/breakpad_linuxish.cc:346: size_t log(const char* buf, size_t nbytes)
{
Do you mind calling this something else to avoid confusion with log() from
math.h?

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/browser/crash...
File chrome/browser/crash_handler_host_linuxish.cc (right):

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/browser/crash...
chrome/browser/crash_handler_host_linuxish.cc:370: unsigned
minidump_filename_str_len = minidump_filename.length() + 1;
You don't need |minidump_filename_str_len| anymore. Put it back the way it was?

[Peter Kasting, 2012-04-09 22:39:04]

chroem/browser/ui/gtk OWNERS LGTM

[Lei Zhang, 2012-04-09 22:54:57]

https://chromiumcodereview.appspot.com/9967017/diff/1026/base/android/build_i...
File base/android/build_info.cc (right):

https://chromiumcodereview.appspot.com/9967017/diff/1026/base/android/build_i...
base/android/build_info.cc:6: 
nit: I think you forgot to add <string.h> for strdup().

https://chromiumcodereview.appspot.com/9967017/diff/1026/base/android/build_i...
File base/android/build_info.h (right):

https://chromiumcodereview.appspot.com/9967017/diff/1026/base/android/build_i...
base/android/build_info.h:80: 
nit: remove blank line.

https://chromiumcodereview.appspot.com/9967017/diff/1026/base/android/build_i...
base/android/build_info.h:83: bool RegisterBuildInfo(JNIEnv* env);
Can you add a comment to explain what this does / when to call it? I don't see
it getting used anywhere.

https://chromiumcodereview.appspot.com/9967017/diff/1026/breakpad/breakpad.gyp
File breakpad/breakpad.gyp (right):

https://chromiumcodereview.appspot.com/9967017/diff/1026/breakpad/breakpad.gy...
breakpad/breakpad.gyp:236: 'variables': {
(Apologies if this has already been discussed previously)

Isn't |host_arch| already set in src/build/common.gypi?

Same with the blocks below.

https://chromiumcodereview.appspot.com/9967017/diff/1026/breakpad/breakpad.gy...
breakpad/breakpad.gyp:241: 'cflags!': ['-m32', '-march=pentium4', '-msse2',
Is this needed? IIRC, Mark fixed minidump_stackwalk so that the 64-bit version
can read 32-bit dump files.

Same with the blocks below.

https://chromiumcodereview.appspot.com/9967017/diff/1026/breakpad/breakpad.gy...
breakpad/breakpad.gyp:254: 'src/processor/minidump_stackwalk.cc',
nit: sort the list.

Same with the blocks below.

https://chromiumcodereview.appspot.com/9967017/diff/1026/build/filename_rules...
File build/filename_rules.gypi (right):

https://chromiumcodereview.appspot.com/9967017/diff/1026/build/filename_rules...
build/filename_rules.gypi:38: ['OS!="android" and OS!="linux" and OS!="openbsd"
and OS!="freebsd"', {
nit: move this above the 'OS!="android"' block, since it's related to the block
on line 26?

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/app/breakpad_...
File chrome/app/breakpad_linuxish.cc (right):

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/app/breakpad_...
chrome/app/breakpad_linuxish.cc:730: char* pid_buf =
reinterpret_cast<char*>(allocator.Alloc(pid_str_len));
Use: "char pid_buf[kUint64StringSize];" instead?

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/app/breakpad_...
chrome/app/breakpad_linuxish.cc:738: size_t done_filename_len = filename_len -1
+ pid_str_len;
nit: space before '1'.

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/app/breakpad_...
chrome/app/breakpad_linuxish.cc:741: // Rename the file such that the pid is the
suffix in order signal to other
in order signal to -> in order to signal

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/app/breakpad_...
chrome/app/breakpad_linuxish.cc:752: __android_log_write(ANDROID_LOG_WARN,
"chromium", "Failed to rename:");
Use log() here?

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/app/breakpad_...
chrome/app/breakpad_linuxish.cc:985: #if defined(OS_ANDROID)
Can't you just do this here? (unconst |unattended|)

#if defined(OS_ANDROID)
// In Android we never upload from native code.
unattended = true;
#endif

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/app/breakpad_...
chrome/app/breakpad_linuxish.cc:1142:
child_process_logging::SetClientId("Android");
Why do you need this for non-browser processes, but not the browser process?

[Mark Mentovai, 2012-04-09 22:55:58]

Please carefully check your change to ensure that no unrelated pieces have
migrated in. I may not have flagged all files that don’t belong.

https://chromiumcodereview.appspot.com/9967017/diff/6032/chrome/browser/resou...
File chrome/browser/resources/uber/uber_frame.html (right):

https://chromiumcodereview.appspot.com/9967017/diff/6032/chrome/browser/resou...
chrome/browser/resources/uber/uber_frame.html:18:
i18n-values="controls:historyHost;override:overridesHistory">
Does this belong?

https://chromiumcodereview.appspot.com/9967017/diff/6032/chrome/browser/resou...
File chrome/browser/resources/uber/uber_frame.js (right):

https://chromiumcodereview.appspot.com/9967017/diff/6032/chrome/browser/resou...
chrome/browser/resources/uber/uber_frame.js:42: // Extensions can override Uber
content (e.g., if the user has a history
This too.

https://chromiumcodereview.appspot.com/9967017/diff/6032/chrome/browser/ui/we...
File chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc (right):

https://chromiumcodereview.appspot.com/9967017/diff/6032/chrome/browser/ui/we...
chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc:747: state_ !=
PLAN_ACTIVATION_RECONNECTING_OTASP) return false;
This too.

https://chromiumcodereview.appspot.com/9967017/diff/6032/chrome/browser/ui/we...
File chrome/browser/ui/webui/uber/uber_ui.cc (right):

https://chromiumcodereview.appspot.com/9967017/diff/6032/chrome/browser/ui/we...
chrome/browser/ui/webui/uber/uber_ui.cc:9: #include
"chrome/browser/extensions/extension_service.h"
This too.

https://chromiumcodereview.appspot.com/9967017/diff/6032/chrome/browser/ui/we...
File chrome/browser/ui/webui/uber/uber_ui.h (right):

https://chromiumcodereview.appspot.com/9967017/diff/6032/chrome/browser/ui/we...
chrome/browser/ui/webui/uber/uber_ui.h:13: #include
"chrome/browser/profiles/profile.h"
This too.

[carlosvaldivia, 2012-04-09 23:01:01]

I suspect part of the problem is that I keep syncing which brings in synced
changes. The problem is that I'm trying to get the try bot to accept my change.


On 2012/04/09 22:56:33, Mark Mentovai wrote:

[carlosvaldivia, 2012-04-09 23:36:23]

https://chromiumcodereview.appspot.com/9967017/diff/1026/base/android/build_i...
File base/android/build_info.cc (right):

https://chromiumcodereview.appspot.com/9967017/diff/1026/base/android/build_i...
base/android/build_info.cc:6: 
This works on android.

On 2012/04/09 22:54:58, Lei Zhang wrote:
> nit: I think you forgot to add <string.h> for strdup().

https://chromiumcodereview.appspot.com/9967017/diff/1026/base/android/build_i...
File base/android/build_info.h (right):

https://chromiumcodereview.appspot.com/9967017/diff/1026/base/android/build_i...
base/android/build_info.h:80: 
On 2012/04/09 22:54:58, Lei Zhang wrote:
> nit: remove blank line.

Done.

https://chromiumcodereview.appspot.com/9967017/diff/1026/base/android/build_i...
base/android/build_info.h:83: bool RegisterBuildInfo(JNIEnv* env);
On 2012/04/09 22:54:58, Lei Zhang wrote:
> Can you add a comment to explain what this does / when to call it? I don't see
> it getting used anywhere.

Done.

https://chromiumcodereview.appspot.com/9967017/diff/1026/breakpad/breakpad.gyp
File breakpad/breakpad.gyp (right):

https://chromiumcodereview.appspot.com/9967017/diff/1026/breakpad/breakpad.gy...
breakpad/breakpad.gyp:236: 'variables': {
Lets just stick with it as is. I don't want to do another build/test cycle.

On 2012/04/09 22:54:58, Lei Zhang wrote:
> (Apologies if this has already been discussed previously)
> 
> Isn't |host_arch| already set in src/build/common.gypi?
> 
> Same with the blocks below.

https://chromiumcodereview.appspot.com/9967017/diff/1026/breakpad/breakpad.gy...
breakpad/breakpad.gyp:241: 'cflags!': ['-m32', '-march=pentium4', '-msse2',
I'm hesitant to change this because it is already working. Changing this would
require another build test cycle and it is harmless as is.

On 2012/04/09 22:54:58, Lei Zhang wrote:
> Is this needed? IIRC, Mark fixed minidump_stackwalk so that the 64-bit version
> can read 32-bit dump files.
> 
> Same with the blocks below.

https://chromiumcodereview.appspot.com/9967017/diff/1026/breakpad/breakpad.gy...
breakpad/breakpad.gyp:254: 'src/processor/minidump_stackwalk.cc',
On 2012/04/09 22:54:58, Lei Zhang wrote:
> nit: sort the list.
> 
> Same with the blocks below.

Done.

https://chromiumcodereview.appspot.com/9967017/diff/1026/build/filename_rules...
File build/filename_rules.gypi (right):

https://chromiumcodereview.appspot.com/9967017/diff/1026/build/filename_rules...
build/filename_rules.gypi:38: ['OS!="android" and OS!="linux" and OS!="openbsd"
and OS!="freebsd"', {
On 2012/04/09 22:54:58, Lei Zhang wrote:
> nit: move this above the 'OS!="android"' block, since it's related to the
block
> on line 26?

Done.

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/app/breakpad_...
File chrome/app/breakpad_linuxish.cc (right):

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/app/breakpad_...
chrome/app/breakpad_linuxish.cc:346: size_t log(const char* buf, size_t nbytes)
{
On 2012/04/09 22:26:54, Lei Zhang wrote:
> Do you mind calling this something else to avoid confusion with log() from
> math.h?

Done.

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/app/breakpad_...
chrome/app/breakpad_linuxish.cc:730: char* pid_buf =
reinterpret_cast<char*>(allocator.Alloc(pid_str_len));
That sounds sketchy. Lets stick with what we know and have tested on android.

On 2012/04/09 22:54:58, Lei Zhang wrote:
> Use: "char pid_buf[kUint64StringSize];" instead?

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/app/breakpad_...
chrome/app/breakpad_linuxish.cc:738: size_t done_filename_len = filename_len -1
+ pid_str_len;
On 2012/04/09 22:54:58, Lei Zhang wrote:
> nit: space before '1'.

Done.

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/app/breakpad_...
chrome/app/breakpad_linuxish.cc:741: // Rename the file such that the pid is the
suffix in order signal to other
On 2012/04/09 22:54:58, Lei Zhang wrote:
> in order signal to -> in order to signal

Done.

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/app/breakpad_...
chrome/app/breakpad_linuxish.cc:752: __android_log_write(ANDROID_LOG_WARN,
"chromium", "Failed to rename:");
It is an aesthetic choice. This is android specific code and I'd have to pass in
a no op strlen which would be confusing.

On 2012/04/09 22:54:58, Lei Zhang wrote:
> Use log() here?

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/app/breakpad_...
chrome/app/breakpad_linuxish.cc:985: #if defined(OS_ANDROID)
On 2012/04/09 22:54:58, Lei Zhang wrote:
> Can't you just do this here? (unconst |unattended|)
> 
> #if defined(OS_ANDROID)
> // In Android we never upload from native code.
> unattended = true;
> #endif

Done.

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/app/breakpad_...
chrome/app/breakpad_linuxish.cc:1142:
child_process_logging::SetClientId("Android");
It gets set correctly on the browser process. The issue is that non-browser
processes are created differently in android.

On 2012/04/09 22:54:58, Lei Zhang wrote:
> Why do you need this for non-browser processes, but not the browser process?

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/browser/crash...
File chrome/browser/crash_handler_host_linuxish.cc (right):

https://chromiumcodereview.appspot.com/9967017/diff/1026/chrome/browser/crash...
chrome/browser/crash_handler_host_linuxish.cc:370: unsigned
minidump_filename_str_len = minidump_filename.length() + 1;
On 2012/04/09 22:26:54, Lei Zhang wrote:
> You don't need |minidump_filename_str_len| anymore. Put it back the way it
was?

Done.

[Lei Zhang, 2012-04-09 23:38:39]

LGTM

per gchat conversation, we'll land this and I'll work on the remaining issues
that did not get addressed.

[carlosvaldivia, 2012-04-09 23:41:23]

On 2012/04/09 22:55:58, Mark Mentovai wrote:
> Please carefully check your change to ensure that no unrelated pieces have
> migrated in. I may not have flagged all files that don’t belong.
> 
>
https://chromiumcodereview.appspot.com/9967017/diff/6032/chrome/browser/resou...
> File chrome/browser/resources/uber/uber_frame.html (right):
> 
>
https://chromiumcodereview.appspot.com/9967017/diff/6032/chrome/browser/resou...
> chrome/browser/resources/uber/uber_frame.html:18:
> i18n-values="controls:historyHost;override:overridesHistory">
> Does this belong?
> 
>
https://chromiumcodereview.appspot.com/9967017/diff/6032/chrome/browser/resou...
> File chrome/browser/resources/uber/uber_frame.js (right):
> 
>
https://chromiumcodereview.appspot.com/9967017/diff/6032/chrome/browser/resou...
> chrome/browser/resources/uber/uber_frame.js:42: // Extensions can override
Uber
> content (e.g., if the user has a history
> This too.
> 
>
https://chromiumcodereview.appspot.com/9967017/diff/6032/chrome/browser/ui/we...
> File chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9967017/diff/6032/chrome/browser/ui/we...
> chrome/browser/ui/webui/chromeos/mobile_setup_ui.cc:747: state_ !=
> PLAN_ACTIVATION_RECONNECTING_OTASP) return false;
> This too.
> 
> 
Hey mark, I had issues with sync. All those changes are side effects of syncing
my code. I have no other changes but those in patch sets 2 and 6. Basically to
the build fix and Lei's comments.

https://chromiumcodereview.appspot.com/9967017/diff/6032/chrome/browser/ui/we...
> File chrome/browser/ui/webui/uber/uber_ui.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9967017/diff/6032/chrome/browser/ui/we...
> chrome/browser/ui/webui/uber/uber_ui.cc:9: #include
> "chrome/browser/extensions/extension_service.h"
> This too.
> 
>
https://chromiumcodereview.appspot.com/9967017/diff/6032/chrome/browser/ui/we...
> File chrome/browser/ui/webui/uber/uber_ui.h (right):
> 
>
https://chromiumcodereview.appspot.com/9967017/diff/6032/chrome/browser/ui/we...
> chrome/browser/ui/webui/uber/uber_ui.h:13: #include
> "chrome/browser/profiles/profile.h"
> This too.

[Mark Mentovai, 2012-04-10 16:53:19]

LGTM

https://chromiumcodereview.appspot.com/9967017/diff/1072/base/android/build_i...
File base/android/build_info.cc (right):

https://chromiumcodereview.appspot.com/9967017/diff/1072/base/android/build_i...
base/android/build_info.cc:20: 
It’s abnormal to start a function with a blank line.

https://chromiumcodereview.appspot.com/9967017/diff/1072/base/android/build_i...
base/android/build_info.cc:55: 
It’s abnormal to end with one too.

[carlosvaldivia, 2012-04-10 16:59:56]

Thanks. I'm going to try to submit.

https://chromiumcodereview.appspot.com/9967017/diff/1072/base/android/build_i...
File base/android/build_info.cc (right):

https://chromiumcodereview.appspot.com/9967017/diff/1072/base/android/build_i...
base/android/build_info.cc:20: 
On 2012/04/10 16:53:19, Mark Mentovai wrote:
> It’s abnormal to start a function with a blank line.

Done.

https://chromiumcodereview.appspot.com/9967017/diff/1072/base/android/build_i...
base/android/build_info.cc:55: 
On 2012/04/10 16:53:19, Mark Mentovai wrote:
> It’s abnormal to end with one too.

Done.

[commit-bot: I haz the power, 2012-04-10 17:00:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/carlosvaldivia@google.com/9967017/2016

[commit-bot: I haz the power, 2012-04-10 18:34:47]

Change committed as 131593