Enable IO before reading /proc/self/maps.

Enable IO before reading /proc/self/maps.

This prevents a recursive loop that causes a stack overflow when printing
stack traces on android from the UI thread.

BUG=258530
R=mark@chromium.org, scherkus@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210896

[Joao da Silva, 2013-07-10 09:55:01]

An android build today was repeatedly hitting a NOTREACHED on webui_impl.cc
(different issue), and this always crashed the browser. gdb showed that this was
a stack overflow because the NOTREACHED() prints a stack trace, which reads
/proc/self/maps from the UI thread, which triggers a LOG(FATAL), which tries to
print a stack trace, etc.

I'm not sure if this is the right fix, please have a look. Thanks!

[Mark Mentovai, 2013-07-10 15:04:35]

I think it’s useful to put a comment at the point of use to explain why you
think it’s OK to allow IO here. Something like ”this is debug-only code” is fine
because it captures the doesn’t-run-in-production essence.

[scherkus (not reviewing), 2013-07-10 15:18:19]

+1 to comment

change BUG= to point to 258530

FYI we may end up removing this restriction to use mmap()+read() versus
base::ReadFile(), see http://code.google.com/p/chromium/issues/detail?id=258451

[Joao da Silva, 2013-07-10 15:26:35]

Added a comment and updated the BUG line, PTAL.

Avoiding the need for this altogether by using mmap is much better. Let's remove
this once it's not needed anymore.

[scherkus (not reviewing), 2013-07-10 15:35:30]

https://codereview.chromium.org/18293007/diff/6001/base/debug/stack_trace_and...
File base/debug/stack_trace_android.cc (right):

https://codereview.chromium.org/18293007/diff/6001/base/debug/stack_trace_and...
base/debug/stack_trace_android.cc:108: if (!ReadProcMaps(&proc_maps)) {
Q: thoughts on putting the ScopedAllowIO inside ReadProcMaps() itself?

[Joao da Silva, 2013-07-10 15:36:44]

https://codereview.chromium.org/18293007/diff/6001/base/debug/stack_trace_and...
File base/debug/stack_trace_android.cc (right):

https://codereview.chromium.org/18293007/diff/6001/base/debug/stack_trace_and...
base/debug/stack_trace_android.cc:108: if (!ReadProcMaps(&proc_maps)) {
On 2013/07/10 15:35:30, scherkus wrote:
> Q: thoughts on putting the ScopedAllowIO inside ReadProcMaps() itself?

Seems like a better place for this to me. Mark, what do you think?

[scherkus (not reviewing), 2013-07-10 15:43:11]

On 2013/07/10 15:36:44, Joao da Silva wrote:
>
https://codereview.chromium.org/18293007/diff/6001/base/debug/stack_trace_and...
> File base/debug/stack_trace_android.cc (right):
> 
>
https://codereview.chromium.org/18293007/diff/6001/base/debug/stack_trace_and...
> base/debug/stack_trace_android.cc:108: if (!ReadProcMaps(&proc_maps)) {
> On 2013/07/10 15:35:30, scherkus wrote:
> > Q: thoughts on putting the ScopedAllowIO inside ReadProcMaps() itself?
> 
> Seems like a better place for this to me. Mark, what do you think?

I'm LGTM either way, with preference for inside ReadProcMaps() as the path is
hardcoded inside that function to read from /read/proc/maps + the scope of the
allow IO is tighter

[Mark Mentovai, 2013-07-10 15:43:47]

I figured it was intentionally placed in OutputToStream because that’s the one
that you’re definitely going to use for debugging purposes. ReadProcMaps is
“quiet” and could potentially be called for non-debugging purposes. We still
might want that to scream, until proven otherwise.

Unless we decide that it’s OK because reading from /proc isn’t I/O in the “disk
I/O” sense. Then you can move the ScopedAllowIO. But then the comment needs to
change too. :)

[scherkus (not reviewing), 2013-07-10 15:49:37]

On 2013/07/10 15:43:47, Mark Mentovai wrote:
> I figured it was intentionally placed in OutputToStream because that’s the one
> that you’re definitely going to use for debugging purposes. ReadProcMaps is
> “quiet” and could potentially be called for non-debugging purposes. We still
> might want that to scream, until proven otherwise.
> 
> Unless we decide that it’s OK because reading from /proc isn’t I/O in the
“disk
> I/O” sense. Then you can move the ScopedAllowIO. But then the comment needs to
> change too. :)

Yeah I guess you can justify the ScopedAllowIO on either the "this is only for
debugging" grounds or the "/proc isn't disk IO" grounds.

I'm OK leaving as is and reevaluating if/when another ReadProcMaps() call makes
it into production code.

[Joao da Silva, 2013-07-10 15:52:27]

I'd leave as is then, with the understanding that it's meant to be removed
later.

Mark, ready to stamp? :-)


On Wed, Jul 10, 2013 at 5:49 PM, <scherkus@chromium.org> wrote:

> On 2013/07/10 15:43:47, Mark Mentovai wrote:
>
>> I figured it was intentionally placed in OutputToStream because that’s
>> the one
>> that you’re definitely going to use for debugging purposes. ReadProcMaps
>> is
>> “quiet” and could potentially be called for non-debugging purposes. We
>> still
>> might want that to scream, until proven otherwise.
>>
>
>  Unless we decide that it’s OK because reading from /proc isn’t I/O in the
>>
> “disk
>
>> I/O” sense. Then you can move the ScopedAllowIO. But then the comment
>> needs to
>> change too. :)
>>
>
> Yeah I guess you can justify the ScopedAllowIO on either the "this is only
> for
> debugging" grounds or the "/proc isn't disk IO" grounds.
>
> I'm OK leaving as is and reevaluating if/when another ReadProcMaps() call
> makes
> it into production code.
>
>
https://codereview.chromium.**org/18293007/<https://codereview.chromium.org/1...
>

[Mark Mentovai, 2013-07-10 16:13:06]

LGTM

[commit-bot: I haz the power, 2013-07-10 16:16:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/18293007/6001

[commit-bot: I haz the power, 2013-07-10 16:25:29]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Joao da Silva, 2013-07-10 18:37:54]

Committed patchset #3 manually as r210896.