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

ID if there is one, (2) only crash extension processes. The latter is a shame -
it'll hide JS problems in the wild - but I'm not comfortable the way it is.

R=jyasskin@chromium.org
TBR=jochen@chromium.org
NOTRY=true

Review URL: https://chromiumcodereview.appspot.com/17104003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@208058 0039d316-1c4b-4281-b951-d872f2087c98

chrome/renderer/extensions/chrome_v8_context_set.cc

@@ -60,10 +60,13 @@ ChromeV8ContextSet::ContextSet ChromeV8ContextSet::GetAll() const {
 }
 
 ChromeV8Context* ChromeV8ContextSet::GetCurrent() const {
-  if (!v8::Context::InContext())
-    return NULL;
-  else
-    return GetByV8Context(v8::Context::GetCurrent());
+  return v8::Context::InContext() ?
+      GetByV8Context(v8::Context::GetCurrent()) : NULL;
+}
+
+ChromeV8Context* ChromeV8ContextSet::GetCalling() const {
+  v8::Local<v8::Context> calling = v8::Context::GetCalling();
+  return calling.IsEmpty() ? NULL : GetByV8Context(calling);
 }
 
 ChromeV8Context* ChromeV8ContextSet::GetByV8Context(

chrome/renderer/extensions/chrome_v8_context_set.h

@@ -51,10 +51,14 @@ class ChromeV8ContextSet {
   typedef std::set<ChromeV8Context*> ContextSet;
   ContextSet GetAll() const;
 
-  // Gets the ChromeV8Context corresponding to the v8::Context that is
-  // on the top of the stack, or NULL if no such context exists.
+  // Gets the ChromeV8Context corresponding to v8::Context::GetCurrent(), or
+  // NULL if no such context exists.
   ChromeV8Context* GetCurrent() const;
 
+  // Gets the ChromeV8Context corresponding to v8::Context::GetCalling(), or
+  // NULL if no such context exists.
+  ChromeV8Context* GetCalling() const;
+
   // Gets the ChromeV8Context corresponding to the specified
   // v8::Context or NULL if no such context exists.
   ChromeV8Context* GetByV8Context(v8::Handle<v8::Context> context) const;

chrome/renderer/extensions/dispatcher.cc

@@ -304,8 +304,12 @@ class ProcessInfoNativeHandler : public ChromeV8Extension {
     RouteFunction("IsSendRequestDisabled",
         base::Bind(&ProcessInfoNativeHandler::IsSendRequestDisabled,
                    base::Unretained(this)));
+    RouteFunction("HasSwitch",
+        base::Bind(&ProcessInfoNativeHandler::HasSwitch,
+                   base::Unretained(this)));
   }
 
+ private:
   void GetExtensionId(const v8::FunctionCallbackInfo<v8::Value>& args) {
     args.GetReturnValue().Set(v8::String::New(extension_id_.c_str()));
   }
@@ -330,7 +334,13 @@ class ProcessInfoNativeHandler : public ChromeV8Extension {
     }
   }
 
- private:
+  void HasSwitch(const v8::FunctionCallbackInfo<v8::Value>& args) {
+    CHECK(args.Length() == 1 && args[0]->IsString());
+    bool has_switch = CommandLine::ForCurrentProcess()->HasSwitch(
+        *v8::String::AsciiValue(args[0]));
+    args.GetReturnValue().Set(v8::Boolean::New(has_switch));
+  }
+
   std::string extension_id_;
   std::string context_type_;
   bool is_incognito_context_;
@@ -971,10 +981,6 @@ void Dispatcher::PopulateSourceMap() {
                              IDR_WEB_VIEW_EXPERIMENTAL_JS);
   source_map_.RegisterSource("denyWebView", IDR_WEB_VIEW_DENY_JS);
   source_map_.RegisterSource("adView", IDR_AD_VIEW_JS);
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableAdviewSrcAttribute)) {
-    source_map_.RegisterSource("adViewCustom", IDR_AD_VIEW_CUSTOM_JS);
-  }
   source_map_.RegisterSource("denyAdView", IDR_AD_VIEW_DENY_JS);
   source_map_.RegisterSource("platformApp", IDR_PLATFORM_APP_JS);
   source_map_.RegisterSource("injectAppTitlebar", IDR_INJECT_APP_TITLEBAR_JS);
@@ -1114,10 +1120,6 @@ void Dispatcher::DidCreateScriptContext(
       is_within_platform_app) {
     if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableAdview)) {
       if (extension->HasAPIPermission(APIPermission::kAdView)) {
-        if (CommandLine::ForCurrentProcess()->HasSwitch(
-                switches::kEnableAdviewSrcAttribute)) {
-          module_system->Require("adViewCustom");
-        }
         module_system->Require("adView");
       } else {
         module_system->Require("denyAdView");

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"
@@ -26,9 +28,50 @@ 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& extension_id,
+                               const std::string& message) {
+  std::string with_extension_id;
+  if (!extension_id.empty()) {
+    with_extension_id += "(";
+    with_extension_id += extension_id;
+    with_extension_id += ") ";
+  }
+  with_extension_id += message;
+  return with_extension_id;
+}
+
+void Fatal(const std::string& extension_id, const std::string& message) {
+  // Only crash web pages in dev channel.
+  // Always crash extension processes, or when in single process mode (since
+  // typically it's used to debug renderer crashes).
+  bool is_fatal = false;
+  const CommandLine* command_line = CommandLine::ForCurrentProcess();
+  if (command_line->HasSwitch(switches::kExtensionProcess) ||
+      command_line->HasSwitch(switches::kSingleProcess)) {
+    is_fatal = true;
+  } else {
+    // <= dev means dev, canary, and trunk.
+    is_fatal = Feature::GetCurrentChannel() <= chrome::VersionInfo::CHANNEL_DEV;
+  }
+  std::string with_extension_id = PrependExtensionID(extension_id, message);
+  if (is_fatal)
+    console::Fatal(v8::Context::GetCalling(), with_extension_id);
+  else
+    console::Error(v8::Context::GetCalling(), with_extension_id);
+}
+
+void Warn(const std::string& extension_id, const std::string& message) {
+  console::Warn(v8::Context::GetCalling(),
+                PrependExtensionID(extension_id, message));
+}
+
 // 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!
@@ -42,9 +85,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(extension_id_,
+          CreateExceptionString(try_catch) + "{" + stack_trace + "}");
   }
+
+ private:
+  std::string extension_id_;
 };
 
 } // namespace
@@ -81,7 +127,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",
@@ -159,7 +206,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(context_->GetExtensionID(), "Extension view no longer exists");
     return v8::Undefined();
   }
 
@@ -171,8 +218,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(context_->GetExtensionID(),
+          "No source for require(" + module_name_str + ")");
     return v8::Undefined();
   }
   v8::Handle<v8::String> wrapped_source(WrapSource(
@@ -180,8 +227,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(context_->GetExtensionID(),
+          "Bad source for require(" + module_name_str + ")");
     return v8::Undefined();
   }
 
@@ -254,9 +301,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(context_->GetExtensionID(),
+          "Failed to get module " + module_name + " to call " + method_name);
     return handle_scope.Close(v8::Undefined());
   }
 
@@ -264,8 +310,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(context_->GetExtensionID(),
+          module_name + "." + method_name + " is not a function");
     return handle_scope.Close(v8::Undefined());
   }
 
@@ -329,8 +375,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;
   }
 
@@ -362,9 +407,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(module_system->context_->GetExtensionID(),
+          "Lazy require of " + name + "." + field_str + " did not " +
+              "set the " + field_str + " field");
     return;
   }
 
@@ -464,8 +509,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(context_->GetExtensionID(),
+          "Natives disabled for requireNative(" + native_name + ")");
     return v8::Undefined();
   }
 
@@ -474,9 +519,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(context_->GetExtensionID(),
+          "Couldn't find native for requireNative(" + native_name + ")");
     return v8::Undefined();
   }
   return i->second->NewInstance();

chrome/renderer/resources/extensions/ad_view.js

@@ -10,17 +10,16 @@
 // TODO(rpaquay): This file is currently very similar to "web_view.js". Do we
 //                want to refactor to extract common pieces?
 
-var adViewCustom = require('adViewCustom');
+var process = requireNative('process');
 var watchForTag = require('tagWatcher').watchForTag;
 
 /**
- * Define "allowCustomAdNetworks" function such that it returns "true" if the
- * "adViewCustom" module was injected. This is so that the
+ * Define "allowCustomAdNetworks" function such that the
  * "kEnableAdviewSrcAttribute" flag is respected.
  */
-var allowCustomAdNetworks = (function(allow){
-  return function() { return Boolean(allow); }
-})(adViewCustom ? adViewCustom.enabled : false);
+function allowCustomAdNetworks() {
+  return process.HasSwitch('enable-adview-src-attribute');
+}
 
 /**
  * List of attribute names to "blindly" sync between <adview> tag and internal

chrome/renderer/resources/extensions/ad_view_custom.js

-// Copyright (c) 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.
-
-exports.enabled = true;

chrome/renderer/resources/renderer_resources.grd

@@ -47,7 +47,6 @@ without changes to the corresponding grd file. fb9 -->
         <!-- Custom bindings for extension APIs. -->
         <include name="IDR_AD_VIEW_DENY_JS" file="extensions\ad_view_deny.js" type="BINDATA" />
         <include name="IDR_AD_VIEW_JS" file="extensions\ad_view.js" type="BINDATA" />
-        <include name="IDR_AD_VIEW_CUSTOM_JS" file="extensions\ad_view_custom.js" type="BINDATA" />
         <include name="IDR_APP_CUSTOM_BINDINGS_JS" file="extensions\app_custom_bindings.js" type="BINDATA" />
         <include name="IDR_APP_RUNTIME_CUSTOM_BINDINGS_JS" file="extensions\app_runtime_custom_bindings.js" type="BINDATA" />
         <include name="IDR_APP_WINDOW_CUSTOM_BINDINGS_JS" file="extensions\app_window_custom_bindings.js" type="BINDATA" />