If an extension feature isn't found, write the feature name to the stack before crashing.

This will help debug crashes like the one in the bug, by looking at the
minidump which contains a snapshot of the stack.

BUG=461915
R=rockot@chromium.org

Review URL: https://codereview.chromium.org/1240423003

Cr-Commit-Position: refs/heads/master@{#342006}

3 files changed, 41 insertions, 15 deletions

diff --git a/extensions/common/features/feature_provider.cc b/extensions/common/features/feature_provider.cc
index b7f6f48..79ecf73 100644
--- a/extensions/common/features/feature_provider.cc
+++ b/extensions/common/features/feature_provider.cc
@@ -14,6 +14,7 @@
 #include "base/trace_event/trace_event.h"
 #include "content/public/common/content_switches.h"
 #include "extensions/common/extensions_client.h"
+#include "extensions/common/features/feature_util.h"
 #include "extensions/common/switches.h"
 
 namespace extensions {
@@ -24,7 +25,8 @@ class Static {
  public:
   FeatureProvider* GetFeatures(const std::string& name) const {
     FeatureProviderMap::const_iterator it = feature_providers_.find(name);
-    CHECK(it != feature_providers_.end());
+    if (it == feature_providers_.end())
+      CRASH_WITH_MINIDUMP("FeatureProvider \"" + name + "\" not found");
     return it->second.get();
   }
 
@@ -72,8 +74,10 @@ const Feature* GetFeatureFromProviderByName(const std::string& provider_name,
                                             const std::string& feature_name) {
   const Feature* feature =
       FeatureProvider::GetByName(provider_name)->GetFeature(feature_name);
-  CHECK(feature) << "FeatureProvider '" << provider_name
-                 << "' does not contain Feature '" << feature_name << "'";
+  if (!feature) {
+    CRASH_WITH_MINIDUMP("Feature \"" + feature_name + "\" not found in " +
+                        "FeatureProvider \"" + provider_name + "\"");
+  }
   return feature;
 }
 
@@ -81,9 +85,7 @@ const Feature* GetFeatureFromProviderByName(const std::string& provider_name,
 
 // static
 const FeatureProvider* FeatureProvider::GetByName(const std::string& name) {
-  const FeatureProvider* feature_provider = g_static.Get().GetFeatures(name);
-  CHECK(feature_provider) << "FeatureProvider '" << name << "' not found";
-  return feature_provider;
+  return g_static.Get().GetFeatures(name);
 }
 
 // static
diff --git a/extensions/common/features/feature_util.h b/extensions/common/features/feature_util.h
new file mode 100644
index 0000000..b8d04fe
--- /dev/null
+++ b/extensions/common/features/feature_util.h
@@ -0,0 +1,30 @@
+// Copyright 2013 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.
+
+#ifndef EXTENSIONS_COMMON_FEATURES_FEATURE_UTIL_H_
+#define EXTENSIONS_COMMON_FEATURES_FEATURE_UTIL_H_
+
+#include "base/debug/alias.h"
+#include "base/logging.h"
+#include "base/strings/string_util.h"
+
+// Writes |message| to the stack so that it shows up in the minidump, then
+// crashes the current process.
+//
+// The prefix "e::" is used so that the crash can be quickly located.
+//
+// This is provided in feature_util because for some reason features are prone
+// to mysterious crashes in named map lookups. For example see crbug.com/365192
+// and crbug.com/461915.
+#define CRASH_WITH_MINIDUMP(message)                                           \
+  {                                                                            \
+    std::string message_copy(message);                                         \
+    char minidump[BUFSIZ];                                                     \
+    base::debug::Alias(&minidump);                                             \
+    base::snprintf(minidump, arraysize(minidump), "e::%s:%d:\"%s\"", __FILE__, \
+                   __LINE__, message_copy.c_str());                            \
+    LOG(FATAL) << message_copy;                                                \
+  }
+
+#endif  // EXTENSIONS_COMMON_FEATURES_FEATURE_UTIL_H_
diff --git a/extensions/common/features/simple_feature.cc b/extensions/common/features/simple_feature.cc
index e7e6844..57d9233 100644
--- a/extensions/common/features/simple_feature.cc
+++ b/extensions/common/features/simple_feature.cc
@@ -10,7 +10,6 @@
 
 #include "base/bind.h"
 #include "base/command_line.h"
-#include "base/debug/alias.h"
 #include "base/macros.h"
 #include "base/sha1.h"
 #include "base/stl_util.h"
@@ -20,6 +19,7 @@
 #include "components/crx_file/id_util.h"
 #include "extensions/common/extension_api.h"
 #include "extensions/common/features/feature_provider.h"
+#include "extensions/common/features/feature_util.h"
 #include "extensions/common/switches.h"
 
 using crx_file::id_util::HashedIdInHex;
@@ -75,14 +75,8 @@ void ParseEnum(const std::string& string_value,
                T* enum_value,
                const std::map<std::string, T>& mapping) {
   const auto& iter = mapping.find(string_value);
-  if (iter == mapping.end()) {
-    // For http://crbug.com/365192.
-    char minidump[256];
-    base::debug::Alias(&minidump);
-    base::snprintf(minidump, arraysize(minidump),
-        "e::simple_feature.cc:%d:\"%s\"", __LINE__, string_value.c_str());
-    CHECK(false) << string_value;
-  }
+  if (iter == mapping.end())
+    CRASH_WITH_MINIDUMP("Enum value not found: " + string_value);
   *enum_value = iter->second;
 }