Improvements to fatal JavaScript extension errors: (1) include the extension

chrome/renderer/extensions/module_system.cc

Index: chrome/renderer/extensions/module_system.cc
diff --git a/chrome/renderer/extensions/module_system.cc b/chrome/renderer/extensions/module_system.cc
index 8d8a01e084b746438035336abeb375fe2069b4bd..83fbc4332cc9b08fd180f9d4c5435d09e7f88d8d 100644
--- a/chrome/renderer/extensions/module_system.cc
+++ b/chrome/renderer/extensions/module_system.cc
@@ -5,10 +5,12 @@
 #include "chrome/renderer/extensions/module_system.h"
 
 #include "base/bind.h"
+#include "base/command_line.h"
 #include "base/debug/trace_event.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension_messages.h"
 #include "chrome/renderer/extensions/chrome_v8_context.h"
 #include "chrome/renderer/extensions/console.h"
@@ -25,9 +27,40 @@ const char* kModuleName = "module_name";
 const char* kModuleField = "module_field";
 const char* kModulesField = "modules";
 
+// Prepends |extension_id| if it's non-empty to |message|.
+std::string PrependExtensionID(const std::string& message,
+                               const std::string& extension_id) {
+  if (extension_id.empty())
+    return message;
+  return "(" + extension_id + ") " + message;

[Jeffrey Yasskin, 2013/06/14 21:42:59]

This sort of expression on std::string is slow and generates lots of code (until
we switch to C++11 anyway). This is why google3 has StrCat(), but it looks like
that's not imported to Chrome yet. It's better to use a local string and +=.

[not at google - send to devlin, 2013/06/14 22:00:32]

Done.

+}
+
+void Fatal(const std::string& message, const std::string& extension_id) {

[Jeffrey Yasskin, 2013/06/14 21:42:59]

nit: I would probably put the extension_id first since it appears first in the
message.

Wouldn't it be nice if we had a distinct type for ExtensionId so the compiler
could tell us if we passed these backwards?

[not at google - send to devlin, 2013/06/14 22:00:32]

done.
hells yes. so often.

+  // Only crash extension processes. Ideally we would crash on *any* (i.e. web
+  // too), but given we hardly have code coverage for JS we should be careful
+  // and not break the web.
+  const CommandLine* command_line = CommandLine::ForCurrentProcess();
+  bool is_extension_process =
+      command_line->HasSwitch(switches::kExtensionProcess) ||
+      command_line->HasSwitch(switches::kSingleProcess);

[Jeffrey Yasskin, 2013/06/14 21:42:59]

You need a comment justifying crashing the whole browser on an extension failure
in single-process mode.

[not at google - send to devlin, 2013/06/14 22:00:32]

Done.

+  std::string with_extension_id = PrependExtensionID(message, extension_id);
+  if (is_extension_process)

[Jeffrey Yasskin, 2013/06/14 21:42:59]

Maybe restrict this to the dev or even canary channel for now. I'm not sure we
want this to land in Beta in a month, although it'll probably be a good idea for
M30.

[not at google - send to devlin, 2013/06/14 22:00:32]

As in - you want it to crash in web pages, or you want it to never crash at all?
We already have the former behaviour. It's a bit weird to make it change
depending on channel, I think an extension silently not working is arguably even
worse than its extension process crashing. Arguably, perhaps tenuously :)

[Jeffrey Yasskin, 2013/06/14 22:11:22]

I think I'd want it to never crash at all on stable, even if the javascript
threw an exception. I'm not sure it's correct to say that "We already have the
former behaviour" since you're adding extra checking and upgrading some of the
Errors here to Fatals, so I don't think it's the case that M29 will crash
strictly less than M28; it'll just be a different arrangement of crashes. If
you're not sick of being hounded by the release engineers, I think it's ok to
leave what you have.

[not at google - send to devlin, 2013/06/14 22:27:44]

True on the case of crashing in new situations; I remember deciding at the time
that they should be error not fatal, but I can't remember why. It seems wrong in
hindsight, since it's our error to be requiring modules that don't exist (etc).

After thinking about this more, I think I'll leave it in. It's quite unlikely
that we'll leave a crash in beta that makes it into stable, certainly one
that'll be bad enough to hurt. The bigger reason though is that having special
code for stable channel makes me scared, regardless of how innocuous it looks.
What if somebody adds to it? Or cargo cults it? A problem that only shows up
once something is promoted to stable would be really bad.

+    console::Fatal(v8::Context::GetCalling(), with_extension_id);
+  else
+    console::Error(v8::Context::GetCalling(), with_extension_id);
+}
+
+void Warn(const std::string& message, const std::string& extension_id) {
+  console::Warn(v8::Context::GetCalling(),
+                PrependExtensionID(message, extension_id));
+}
+
 // Default exception handler which logs the exception.
 class DefaultExceptionHandler : public ModuleSystem::ExceptionHandler {
  public:
+  explicit DefaultExceptionHandler(const std::string& extension_id)
+      : extension_id_(extension_id) {}
+
   // Fatally dumps the debug info from |try_catch| to the console.
   // Make sure this is never used for exceptions that originate in external
   // code!
@@ -41,9 +74,12 @@ class DefaultExceptionHandler : public ModuleSystem::ExceptionHandler {
       else
         stack_trace = "<could not convert stack trace to string>";
     }
-    console::Fatal(v8::Context::GetCalling(),
-                   CreateExceptionString(try_catch) + "{" + stack_trace + "}");
+    Fatal(CreateExceptionString(try_catch) + "{" + stack_trace + "}",
+          extension_id_);
   }
+
+ private:
+  std::string extension_id_;
 };
 
 } // namespace
@@ -80,7 +116,8 @@ ModuleSystem::ModuleSystem(ChromeV8Context* context,
       context_(context),
       source_map_(source_map),
       natives_enabled_(0),
-      exception_handler_(new DefaultExceptionHandler()) {
+      exception_handler_(
+          new DefaultExceptionHandler(context->GetExtensionID())) {
   RouteFunction("require",
       base::Bind(&ModuleSystem::RequireForJs, base::Unretained(this)));
   RouteFunction("requireNative",
@@ -158,7 +195,7 @@ v8::Handle<v8::Value> ModuleSystem::RequireForJsInner(
   v8::Handle<v8::Value> modules_value =
       global->GetHiddenValue(v8::String::New(kModulesField));
   if (modules_value.IsEmpty() || modules_value->IsUndefined()) {
-    console::Warn(v8::Context::GetCalling(), "Extension view no longer exists");
+    Warn("Extension view no longer exists", context_->GetExtensionID());
     return v8::Undefined();
   }
 
@@ -170,8 +207,8 @@ v8::Handle<v8::Value> ModuleSystem::RequireForJsInner(
   std::string module_name_str = *v8::String::AsciiValue(module_name);
   v8::Handle<v8::Value> source(GetSource(module_name_str));
   if (source.IsEmpty() || source->IsUndefined()) {
-    console::Error(v8::Context::GetCalling(),
-                   "No source for require(" + module_name_str + ")");
+    Fatal("No source for require(" + module_name_str + ")",
+          context_->GetExtensionID());
     return v8::Undefined();
   }
   v8::Handle<v8::String> wrapped_source(WrapSource(
@@ -179,8 +216,8 @@ v8::Handle<v8::Value> ModuleSystem::RequireForJsInner(
   // Modules are wrapped in (function(){...}) so they always return functions.
   v8::Handle<v8::Value> func_as_value = RunString(wrapped_source, module_name);
   if (func_as_value.IsEmpty() || func_as_value->IsUndefined()) {
-    console::Error(v8::Context::GetCalling(),
-                   "Bad source for require(" + module_name_str + ")");
+    Fatal("Bad source for require(" + module_name_str + ")",
+          context_->GetExtensionID());
     return v8::Undefined();
   }
 
@@ -242,9 +279,8 @@ v8::Local<v8::Value> ModuleSystem::CallModuleMethod(
   }
 
   if (module.IsEmpty() || !module->IsObject()) {
-    console::Error(
-        v8::Context::GetCalling(),
-        "Failed to get module " + module_name + " to call " + method_name);
+    Fatal("Failed to get module " + module_name + " to call " + method_name,
+          context_->GetExtensionID());
     return handle_scope.Close(v8::Undefined());
   }
 
@@ -252,8 +288,8 @@ v8::Local<v8::Value> ModuleSystem::CallModuleMethod(
       v8::Handle<v8::Object>::Cast(module)->Get(
           v8::String::New(method_name.c_str()));
   if (value.IsEmpty() || !value->IsFunction()) {
-    console::Error(v8::Context::GetCalling(),
-                   module_name + "." + method_name + " is not a function");
+    Fatal(module_name + "." + method_name + " is not a function",
+          context_->GetExtensionID());
     return handle_scope.Close(v8::Undefined());
   }
 
@@ -317,8 +353,7 @@ void ModuleSystem::LazyFieldGetterInner(
   if (module_system_value.IsEmpty() || !module_system_value->IsExternal()) {
     // ModuleSystem has been deleted.
     // TODO(kalman): See comment in header file.
-    console::Warn(v8::Context::GetCalling(),
-                  "Module system has been deleted, does extension view exist?");
+    Warn("Module system has been deleted, does extension view exist?", "");
     return;
   }
 
@@ -350,9 +385,9 @@ void ModuleSystem::LazyFieldGetterInner(
 
   if (!module->Has(field)) {
     std::string field_str = *v8::String::AsciiValue(field);
-    console::Fatal(v8::Context::GetCalling(),
-                   "Lazy require of " + name + "." + field_str + " did not " +
-                   "set the " + field_str + " field");
+    Fatal("Lazy require of " + name + "." + field_str + " did not " +
+              "set the " + field_str + " field",
+          module_system->context_->GetExtensionID());
     return;
   }
 
@@ -452,8 +487,8 @@ v8::Handle<v8::Value> ModuleSystem::RequireNativeFromString(
     // we could crash.
     if (exception_handler_)
       return v8::ThrowException(v8::String::New("Natives disabled"));
-    console::Fatal(v8::Context::GetCalling(),
-                   "Natives disabled for requireNative(" + native_name + ")");
+    Fatal("Natives disabled for requireNative(" + native_name + ")",
+          context_->GetExtensionID());
     return v8::Undefined();
   }
 
@@ -462,9 +497,8 @@ v8::Handle<v8::Value> ModuleSystem::RequireNativeFromString(
 
   NativeHandlerMap::iterator i = native_handler_map_.find(native_name);
   if (i == native_handler_map_.end()) {
-    console::Fatal(
-        v8::Context::GetCalling(),
-        "Couldn't find native for requireNative(" + native_name + ")");
+    Fatal("Couldn't find native for requireNative(" + native_name + ")",
+          context_->GetExtensionID());
     return v8::Undefined();
   }
   return i->second->NewInstance();