Always search TIDs for the crashing processes.

Always search TIDs for the crashing processes.

Kernels newer than 2.6.32 support TID and PID namespacing where
processes' view of their TIDs and PIDs are not globally unique or
externally meaningful.  We have workarounds to find the TID and PID of
the crashing process from outside in the browser process.  However, we
were only assuming TID namespacing was happening if PID namespacing
was enabled and the kernel had a bug that was fixed since 2.6.38.
This change causes us to always treat the TID as subject to
namespacing.  Our trick to find the TID relies on a procfs feature
added in 2008.  We assume if that feature is not yet present that
the TID translation is not necessary.

This fixes the bug where all crashes of non-browser processes on Linux
2.6.38+ (Chrome OS r13+) are unusable (result in
UnspecifiedStackSignature).

BUG=chromium-os:15462
TEST=Do about:crash on 2.6.38 kernel and verify proper tid listed in
MDException block 

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

[oshima, 2011-06-17 01:50:34]

I don't think i'm qualified to review this as i'm not familiar with this code,
and will let lei review this. Thank you for fixing this anyway!

I'm sorry that I didn't have time to do for what you asked yesterday. I assume
you no longer need it, but please let me know if you want me to test.

On 2011/06/17 00:30:49, kmixter1 wrote:

[kmixter1, 2011-06-17 17:10:46]

No worries.  I eventually found out why my "brand" wasn't set to
Chrome and got a build with crash reporting and found that it was
failing as suspected.

Lei - will you be able to review this or is there someone else better to?

On Thu, Jun 16, 2011 at 6:50 PM,  <oshima@chromium.org> wrote:
> I don't think i'm qualified to review this as i'm not familiar with this
> code,
> and will let lei review this. Thank you for fixing this anyway!
>
> I'm sorry that I didn't have time to do for what you asked yesterday. I
> assume
> you no longer need it, but please let me know if you want me to test.
>
> On 2011/06/17 00:30:49, kmixter1 wrote:
>
>
>
> http://codereview.chromium.org/7190019/
>

[Lei Zhang, 2011-06-17 21:30:24]

I'll take a look today / over the weekend. I just upgraded my CR48 to r13. I
still need to do a build and test out this CL.

On 2011/06/17 17:10:46, kmixter1 wrote:
> No worries.  I eventually found out why my "brand" wasn't set to
> Chrome and got a build with crash reporting and found that it was
> failing as suspected.
> 
> Lei - will you be able to review this or is there someone else better to?
> 
> On Thu, Jun 16, 2011 at 6:50 PM,  <mailto:oshima@chromium.org> wrote:
> > I don't think i'm qualified to review this as i'm not familiar with this
> > code,
> > and will let lei review this. Thank you for fixing this anyway!
> >
> > I'm sorry that I didn't have time to do for what you asked yesterday. I
> > assume
> > you no longer need it, but please let me know if you want me to test.
> >
> > On 2011/06/17 00:30:49, kmixter1 wrote:
> >
> >
> >
> > http://codereview.chromium.org/7190019/
> >

[Lei Zhang, 2011-06-20 21:37:10]

LGTM, but please update the comments.

http://codereview.chromium.org/7190019/diff/1/chrome/browser/crash_handler_ho...
File chrome/browser/crash_handler_host_linux.cc (right):

http://codereview.chromium.org/7190019/diff/1/chrome/browser/crash_handler_ho...
chrome/browser/crash_handler_host_linux.cc:224: // Kernel bug workaround (broken
in 2.6.30 at least):
Can we update this comment block to:

// Kernel bug workaround (broken in 2.6.30 and 2.6.32, working in 2.6.38)
// The kernel doesn't translate PIDs in SCM_CREDENTIALS across PID
// namespaces. Thus |crashing_pid| might be garbage from our point of view.
// In the future we can remove this workaround, but we have to wait a couple
// of years to be sure that it's worked its way out into the world.
// TODO(thestig) Remove the workaround when Ubuntu Lucid is deprecated.

http://codereview.chromium.org/7190019/diff/1/chrome/browser/crash_handler_ho...
chrome/browser/crash_handler_host_linux.cc:254: // in
ExceptionHandler::HandleSignal is also wrong and needs to be
s/is also/might be/

[kmixter1, 2011-06-20 23:31:01]

I've updated with Lei's comments, and tested the changes (which were just to
comments but just to be safe).

Lei/Oshima, can one of you submit this for me since I'm not a committer?

On 2011/06/20 21:37:10, Lei Zhang wrote:
> LGTM, but please update the comments.
> 
>
http://codereview.chromium.org/7190019/diff/1/chrome/browser/crash_handler_ho...
> File chrome/browser/crash_handler_host_linux.cc (right):
> 
>
http://codereview.chromium.org/7190019/diff/1/chrome/browser/crash_handler_ho...
> chrome/browser/crash_handler_host_linux.cc:224: // Kernel bug workaround
(broken
> in 2.6.30 at least):
> Can we update this comment block to:
> 
> // Kernel bug workaround (broken in 2.6.30 and 2.6.32, working in 2.6.38)
> // The kernel doesn't translate PIDs in SCM_CREDENTIALS across PID
> // namespaces. Thus |crashing_pid| might be garbage from our point of view.
> // In the future we can remove this workaround, but we have to wait a couple
> // of years to be sure that it's worked its way out into the world.
> // TODO(thestig) Remove the workaround when Ubuntu Lucid is deprecated.
> 
>
http://codereview.chromium.org/7190019/diff/1/chrome/browser/crash_handler_ho...
> chrome/browser/crash_handler_host_linux.cc:254: // in
> ExceptionHandler::HandleSignal is also wrong and needs to be
> s/is also/might be/

[Lei Zhang, 2011-06-20 23:37:02]

The commit bot can do it, but we probably need some OWNERs approvals. I'll click
the checkbox and see what commit-bot says.

[commit-bot: I haz the power, 2011-06-20 23:37:21]

Presubmit check for 7190019-9001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: base/linux_util.h,base/linux_util.cc

Presubmit checks took 2.3s to calculate.

[kmixter1, 2011-06-20 23:49:17]

I still need a reviewer from base OWNERS for the small change in linux_utils.cc.
 This is for an M13 bug fix.

Brett/Mark/Will - could any of you do a review?

http://codereview.chromium.org/7190019/diff/1/chrome/browser/crash_handler_ho...
File chrome/browser/crash_handler_host_linux.cc (right):

http://codereview.chromium.org/7190019/diff/1/chrome/browser/crash_handler_ho...
chrome/browser/crash_handler_host_linux.cc:224: // Kernel bug workaround (broken
in 2.6.30 at least):
On 2011/06/20 21:37:10, Lei Zhang wrote:
> Can we update this comment block to:
> 
> // Kernel bug workaround (broken in 2.6.30 and 2.6.32, working in 2.6.38)
> // The kernel doesn't translate PIDs in SCM_CREDENTIALS across PID
> // namespaces. Thus |crashing_pid| might be garbage from our point of view.
> // In the future we can remove this workaround, but we have to wait a couple
> // of years to be sure that it's worked its way out into the world.
> // TODO(thestig) Remove the workaround when Ubuntu Lucid is deprecated.

Done.

http://codereview.chromium.org/7190019/diff/1/chrome/browser/crash_handler_ho...
chrome/browser/crash_handler_host_linux.cc:254: // in
ExceptionHandler::HandleSignal is also wrong and needs to be
On 2011/06/20 21:37:10, Lei Zhang wrote:
> s/is also/might be/

Done.

[Mark Mentovai, 2011-06-21 00:11:24]

LGTM

http://codereview.chromium.org/7190019/diff/9001/base/linux_util.cc
File base/linux_util.cc (right):

http://codereview.chromium.org/7190019/diff/9001/base/linux_util.cc#newcode277
base/linux_util.cc:277: *syscall_supported = false;
Can you do this at the very beginning of the function so that it’s always set
regardless of whether this function returns early? Right now, there’s one
early-return (-1) above where it won’t be set, and a return -1 below with this
already set.

http://codereview.chromium.org/7190019/diff/9001/base/linux_util.h
File base/linux_util.h (right):

http://codereview.chromium.org/7190019/diff/9001/base/linux_util.h#newcode41
base/linux_util.h:41: // set to false the kernel does not support syscall in
procfs.
You should comment that syscall_supported must be non-NULL, or you should make
the implementation in the .cc file tolerate a NULL argument.

[kmixter1, 2011-06-21 00:55:33]

Updated with changes per Mark's request.  Tested on local device.

http://codereview.chromium.org/7190019/diff/9001/base/linux_util.cc
File base/linux_util.cc (right):

http://codereview.chromium.org/7190019/diff/9001/base/linux_util.cc#newcode277
base/linux_util.cc:277: *syscall_supported = false;
On 2011/06/21 00:11:24, Mark Mentovai wrote:
> Can you do this at the very beginning of the function so that it’s always set
> regardless of whether this function returns early? Right now, there’s one
> early-return (-1) above where it won’t be set, and a return -1 below with this
> already set.

Good catch.  Moved it up so that it's always set to something on all return
paths.  It's ok for the result to be -1 and *syscall_supported==true.  This
means we know for certain that no thread for this pid was running the expected
syscall (compared to -1 and *syscall_supported==false) when we just don't know.

http://codereview.chromium.org/7190019/diff/9001/base/linux_util.h
File base/linux_util.h (right):

http://codereview.chromium.org/7190019/diff/9001/base/linux_util.h#newcode41
base/linux_util.h:41: // set to false the kernel does not support syscall in
procfs.
On 2011/06/21 00:11:24, Mark Mentovai wrote:
> You should comment that syscall_supported must be non-NULL, or you should make
> the implementation in the .cc file tolerate a NULL argument.

Done.  Handling when it's NULL.

[Mark Mentovai, 2011-06-21 01:54:21]

My LG was valid with those changes made, but just to be explicit:

LGTM

[commit-bot: I haz the power, 2011-06-21 04:21:09]

Change committed as 89795