Use posix version of stack_trace on iOS

Use posix version of stack_trace on iOS

The iOS specific version was not async-safe, and the posix version
works correctly on iOS (manually tested). Scrap the iOS version
and always use the posix one.

BUG=334640
R=mark@chromium.org

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

[sdefresne, 2014-01-15 16:27:13]

Please have a look

[Mark Mentovai, 2014-01-15 16:49:44]

Using streams in a signal handler is a step backwards. Streams are not safe
to use in a signal handler, and the TODO here calls this out. See RAW_LOG
(base/logging.h). The Objective-C in here is also unsafe.

This is the most important comment, but since the change its short,
additional comments:

Blank line between C system headers and C++ system headers, before
<ostream>.

No need to end lines with "\n" when using our log streams.
On Jan 15, 2014 11:27 AM, <sdefresne@chromium.org> wrote:

> Reviewers: Mark Mentovai,
>
> Message:
> Please have a look
>
> Description:
> Make callstack more visible in stderr and flush the stream
>
> Some unittests fails on ios bots with stderr.log ending a message
> from stack_trace_ios.mm. Surroung the callstack with markers and
> use "LOG(ERROR)" instead of directly writing to "stderr" to pass
> the presubmit tests (and also ensure that the stream is flushed).
>
> Sample of one such crashes:
>  INFO: >>>> Dumping contents of stderr.log <<<<
>  2014-01-15 07:21:14.453 net_unittests[501:60b] Application windows are
> expected
> to have a root view controller at the end of application launch
>  [501:1547:0115/072145:46247512209:ERROR:simple_version_upgrade.cc(161)]
> Inconsistent cache version.
>  [501:1547:0115/072149:46351508341:ERROR:stack_trace_ios.mm(21)] Received
> signal
> 10
>  INFO: >>>> End contents of stderr.log <<<<
>
> BUG=334640
> R=mark@chromium.org
>
> Please review this at https://codereview.chromium.org/136033004/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+4, -2 lines):
>   base/debug/stack_trace_ios.mm
>
>
> Index: base/debug/stack_trace_ios.mm
> diff --git a/base/debug/stack_trace_ios.mm b/base/debug/stack_trace_ios.mm
> index 998dd0dfc693b0baf73de8c874d999bc3a6a6ea7..
> a8a6674208ab62696a366fbb81754acc85c35848 100644
> --- a/base/debug/stack_trace_ios.mm
> +++ b/base/debug/stack_trace_ios.mm
> @@ -4,7 +4,7 @@
>
>  #import <Foundation/Foundation.h>
>  #include <mach/task.h>
> -#include <stdio.h>
> +#include <ostream>
>
>  #include "base/logging.h"
>
> @@ -19,10 +19,12 @@ namespace {
>  void StackDumpSignalHandler(int signal) {
>    // TODO(phajdan.jr): Fix async-signal unsafety.
>    LOG(ERROR) << "Received signal " << signal;
> +  LOG(ERROR) << ">> Callstack:\n";
>    NSArray *stack_symbols = [NSThread callStackSymbols];
>    for (NSString* stack_symbol in stack_symbols) {
> -    fprintf(stderr, "\t%s\n", [stack_symbol UTF8String]);
> +    LOG(ERROR) << "\t%s\n" << [stack_symbol UTF8String];
>    }
> +  LOG(ERROR) << "<<\n" << std::flush;
>    _exit(1);
>  }
>
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sdefresne, 2014-01-15 19:08:47]

OK, so I tried to use the posix stack_tracer on iOS and it works. So I changed
my CL to always use the posix stack_tracer (without using the info pointer that
is not properly set on iOS).

Please have another look.

[Mark Mentovai, 2014-01-15 19:14:06]

Much better.

https://codereview.chromium.org/136033004/diff/120001/base/debug/stack_trace_...
File base/debug/stack_trace_posix.cc (right):

https://codereview.chromium.org/136033004/diff/120001/base/debug/stack_trace_...
base/debug/stack_trace_posix.cc:198: // NOTE: on iOS, info and void_context are
incorrect. Ignore them.
If you’ve got a reference (link?) to point to here, it’d be helpful. Also, if
you can determine whether it’s just broken for ARM and OK in the simulator, you
can refine the #ifdef.

https://codereview.chromium.org/136033004/diff/120001/base/debug/stack_trace_...
base/debug/stack_trace_posix.cc:334: #elif defined(OS_MACOSX) &&
!defined(OS_IOS)
You probably don’t need !defined(OS_IOS) here, because everything inside this is
limited to ARCH_CPU_X86_FAMILY && ARCH_CPU_32_BITS. You could change the next
line to “port to x86_64 and ARM (for iOS).”

Unless this won’t work at all in the simulator.

[sdefresne, 2014-01-15 19:50:21]

After more testing, I found that info and context are correct, so removed the
corresponding defined(OS_IOS) and updated the comment.

PTAL

[Mark Mentovai, 2014-01-15 19:51:27]

LGTM otherwise.

https://codereview.chromium.org/136033004/diff/160001/base/debug/stack_trace_...
File base/debug/stack_trace_posix.cc (right):

https://codereview.chromium.org/136033004/diff/160001/base/debug/stack_trace_...
base/debug/stack_trace_posix.cc:197: // NOTE: on iOS, info and void_context are
incorrect. Ignore them.
Remove this line.

[Mark Mentovai, 2014-01-15 19:51:56]

Nit: in the CL description, scrape → scrap.

[sdefresne, 2014-02-10 09:11:52]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-02-10 09:12:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sdefresne@chromium.org/136033004/220001

[commit-bot: I haz the power, 2014-02-10 10:48:31]

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

[commit-bot: I haz the power, 2014-02-10 10:48:32]

Retried try job too often on win_rel for step(s) telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[sdefresne, 2014-02-10 17:22:50]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-02-10 17:23:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sdefresne@chromium.org/136033004/220001

[commit-bot: I haz the power, 2014-02-10 18:18:34]

Change committed as 250154