[Mac] Modify the ObjC exception preprocessor to make some exceptions fatal.

chrome/browser/mac/exception_processor.mm

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import "chrome/browser/mac/exception_processor.h"
+
+#import <Foundation/Foundation.h>
+#include <libunwind.h>
+#include <objc/objc-exception.h>
+
+#include "base/debug/crash_logging.h"
+#include "base/debug/stack_trace.h"
+#include "base/logging.h"
+#include "base/macros.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/strings/sys_string_conversions.h"
+#include "chrome/common/crash_keys.h"
+
+namespace chrome {
+
+// Maximum number of known named exceptions we'll support.  There is
+// no central registration, but I only find about 75 possibilities in
+// the system frameworks, and many of them are probably not
+// interesting to track in aggregate (those relating to distributed
+// objects, for instance).
+const size_t kKnownNSExceptionCount = 25;

[Mark Mentovai, 2016/12/01 19:02:33]

Can constexpr these two also.

[Robert Sesek, 2016/12/06 23:18:09]

Did this one, but not the latter (per comment in other file).

+
+const size_t kUnknownNSException = kKnownNSExceptionCount;
+
+size_t BinForException(NSException* exception) {
+  // A list of common known exceptions.  The list position will
+  // determine where they live in the histogram, so never move them
+  // around, only add to the end.
+  static NSString* const kKnownNSExceptionNames[] = {

[Mark Mentovai, 2016/12/01 19:02:33]

static doesn’t contribute to anything here.

[Robert Sesek, 2016/12/06 23:18:09]

Done.

+    // Grab-bag exception, not very common.  CFArray (or other
+    // container) mutated while being enumerated is one case seen in
+    // production.
+    NSGenericException,
+
+    // Out-of-range on NSString or NSArray.  Quite common.
+    NSRangeException,
+
+    // Invalid arg to method, unrecognized selector.  Quite common.
+    NSInvalidArgumentException,
+
+    // malloc() returned null in object creation, I think.  Turns out
+    // to be very uncommon in production, because of the OOM killer.
+    NSMallocException,
+
+    // This contains things like windowserver errors, trying to draw
+    // views which aren't in windows, unable to read nib files.  By
+    // far the most common exception seen on the crash server.
+    NSInternalInconsistencyException,
+
+    nil

[Mark Mentovai, 2016/12/01 19:02:33]

Instead of a nil-terminated list, just use arraysize in the loop below. Remember
to use _LT or < for the check that follows, too.

[Robert Sesek, 2016/12/06 23:18:09]

Done.

+  };
+
+  // Make sure our array hasn't outgrown our abilities to track it.
+  DCHECK_LE(arraysize(kKnownNSExceptionNames), kKnownNSExceptionCount);

[Mark Mentovai, 2016/12/01 19:02:33]

constexpr benefit: static_cast instead of DCHECK!

[Robert Sesek, 2016/12/06 23:18:09]

Done.

+
+  NSString* name = [exception name];
+  for (int i = 0; kKnownNSExceptionNames[i]; ++i) {
+    if (name == kKnownNSExceptionNames[i]) {
+      return i;
+    }
+  }
+  return kUnknownNSException;
+}
+
+void RecordExceptionWithUma(NSException* exception) {
+  UMA_HISTOGRAM_ENUMERATION("OSX.NSException",
+      BinForException(exception), kUnknownNSException);
+}
+
+static objc_exception_preprocessor g_next_preprocessor = nullptr;
+
+static const char* kExceptionSinkholes[] = {

[Mark Mentovai, 2016/12/01 19:02:33]

const the pointers too.

[Robert Sesek, 2016/12/06 23:18:09]

Done.

+  "CFRunLoopRunSpecific",
+  "_dispatch_client_callout",
+};
+
+id ObjcExceptionPreprocessor(id exception) {

[Mark Mentovai, 2016/12/01 19:02:32]

Should be static.

Or introduce an unnamed namespace and group all of the other static and
non-extern const things into it.

[Robert Sesek, 2016/12/06 23:18:09]

Made static because the anon namespace adds extra junk to the crash signatures.

+  static bool seen_first_exception = false;
+
+  // Record UMA and crash keys about the exception.
+  RecordExceptionWithUma(exception);
+
+  const char* const kExceptionKey =
+      seen_first_exception ? crash_keys::mac::kLastNSException
+                           : crash_keys::mac::kFirstNSException;
+  NSString* value = [NSString stringWithFormat:@"%@ reason %@",
+      [exception name], [exception reason]];
+  base::debug::SetCrashKeyValue(kExceptionKey, base::SysNSStringToUTF8(value));
+
+  const char* const kExceptionTraceKey =
+      seen_first_exception ? crash_keys::mac::kLastNSExceptionTrace
+                           : crash_keys::mac::kFirstNSExceptionTrace;
+  // This exception preprocessor runs prior to the one in libobjc, which sets
+  // the -[NSException callStackReturnAddresses].
+  base::debug::SetCrashKeyToStackTrace(kExceptionTraceKey,
+                                       base::debug::StackTrace());
+
+  seen_first_exception = true;
+
+  //////////////////////////////////////////////////////////////////////////////
+
+  // Unwind the stack looking for any exception handlers. If an exception

[Mark Mentovai, 2016/12/01 19:02:33]

Oh good, new stuff!

+  // handler is encountered, test to see if it is a function known to catch-
+  // and-rethrow as a "top-level" exception handler. Various routines in
+  // Cocoa do this, and it obscures the crashing stack, since the original
+  // throw location is no longer present on the stack (just the re-throw) when
+  // Crashpad captures the crash report.
+  unw_context_t context;
+  unw_getcontext(&context);
+
+  unw_cursor_t cursor;
+  unw_init_local(&cursor, &context);
+
+  for (;;) {
+    int result = unw_step(&cursor);

[Mark Mentovai, 2016/12/01 19:02:33]

Why not fold this into the loop condition?: while (unw_step(&cursor) > 0)

[Robert Sesek, 2016/12/06 23:18:09]

Done.

+    if (result <= 0) {
+      break;
+    }
+
+    unw_proc_info_t frame_info;
+    if (unw_get_proc_info(&cursor, &frame_info) != UNW_ESUCCESS) {
+      continue;
+    }
+
+    // This frame has an exception handler.
+    if (frame_info.handler != 0) {
+      char proc_name[64];
+      unw_word_t name_len;

[Mark Mentovai, 2016/12/01 19:02:33]

This is not name_len, it’s the offset of unw_get_reg(cursor, UNW_REG_IP)
relative to the address of the symbol whose name is returned.

[Robert Sesek, 2016/12/06 23:18:09]

Oops, done.

+      if (unw_get_proc_name(&cursor, proc_name, sizeof(proc_name),
+                            &name_len) != UNW_ESUCCESS) {
+        continue;
+      }
+
+      // Check if the function is one that is known to obscure (by way of
+      // catch-and-rethrow) exception stack traces. If it is, sinkhole it
+      // by crashing at the point of throw.

[Mark Mentovai, 2016/12/01 19:02:33]

by crashing here at the point of throw

[Robert Sesek, 2016/12/06 23:18:09]

Done.

+      for (const auto& sinkhole : kExceptionSinkholes) {
+        if (strcmp(sinkhole, proc_name) == 0) {
+          LOG(FATAL) << "Terminating from Objective-C exception: "

[Mark Mentovai, 2016/12/01 19:02:33]

It might be neat if we could do something to get the actual exception thrower’s
context to appear at the top of the stack instead of this preprocessor function.

Barring that, what do you think about calling a function named for what’s
actually happening, like TerminateOnObjectiveCException(), and have death happen
there? You know, Apple-style. That’d at least make the top of the stack in
symbolized crash reports look like the Objective-C exception was the problem and
less like ObjcExceptionPreprocessor() has a crash bug. Because you know
someone’s gonna think that…

[Robert Sesek, 2016/12/06 23:18:09]

I considered this but it may make things less clear, and it's more complicated
to do correctly.
Done.

+                     << base::SysNSStringToUTF8([exception name])
+                     << ": " << base::SysNSStringToUTF8([exception reason]);
+        }
+      }
+
+      break;
+    }
+  }
+
+  // Forward to the next preprocessor.
+  if (g_next_preprocessor)
+    return g_next_preprocessor(exception);
+
+  return exception;
+}
+
+void InstallObjcExceptionPreprocessor() {
+  if (g_next_preprocessor)
+    return;
+
+  g_next_preprocessor =
+      objc_setExceptionPreprocessor(&ObjcExceptionPreprocessor);
+}
+
+void UninstallObjcExceptionPreprocessor() {
+  if (!g_next_preprocessor)

[Mark Mentovai, 2016/12/01 19:02:33]

If there was no previous preprocessor, this won’t restore the no-preprocessor
state. I expect that no previous preprocessor is legitimate because you test for
it on line 155.

[Robert Sesek, 2016/12/06 23:18:09]

Done.

+    return;
+
+  objc_setExceptionPreprocessor(g_next_preprocessor);
+  g_next_preprocessor = nullptr;
+}
+
+}  // namespace chrome