[Extensions] Don't require() a module for calling a method

extensions/renderer/module_system.cc

 // Copyright 2014 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.
 
 #include "extensions/renderer/module_system.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/macros.h"
@@ skipping 221 matching lines @@
 void ModuleSystem::HandleException(const v8::TryCatch& try_catch) {
   exception_handler_->HandleUncaughtException(try_catch);
 }
 
 v8::MaybeLocal<v8::Object> ModuleSystem::Require(
     const std::string& module_name) {
   v8::Local<v8::String> v8_module_name;
   if (!ToV8String(GetIsolate(), module_name, &v8_module_name))
     return v8::MaybeLocal<v8::Object>();
   v8::EscapableHandleScope handle_scope(GetIsolate());
-  v8::Local<v8::Value> value = RequireForJsInner(
-      v8_module_name);
+  v8::Local<v8::Value> value =
+      RequireForJsInner(v8_module_name, true /* create */);
   if (value.IsEmpty() || !value->IsObject())
     return v8::MaybeLocal<v8::Object>();
   return handle_scope.Escape(value.As<v8::Object>());
 }
 
 void ModuleSystem::RequireForJs(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   if (!args[0]->IsString()) {
     NOTREACHED() << "require() called with a non-string argument";
     return;
   }
   v8::Local<v8::String> module_name = args[0].As<v8::String>();
-  args.GetReturnValue().Set(RequireForJsInner(module_name));
+  args.GetReturnValue().Set(RequireForJsInner(module_name, true /* create */));
 }
 
 v8::Local<v8::Value> ModuleSystem::RequireForJsInner(
-    v8::Local<v8::String> module_name) {
+    v8::Local<v8::String> module_name,
+    bool create) {
   v8::EscapableHandleScope handle_scope(GetIsolate());
   v8::Local<v8::Context> v8_context = context()->v8_context();
   v8::Context::Scope context_scope(v8_context);
 
   v8::Local<v8::Object> global(context()->v8_context()->Global());
 
   // The module system might have been deleted. This can happen if a different
   // context keeps a reference to us, but our frame is destroyed (e.g.
   // background page keeps reference to chrome object in a closed popup).
   v8::Local<v8::Value> modules_value;
   if (!GetPrivate(global, kModulesField, &modules_value) ||
       modules_value->IsUndefined()) {
     Warn(GetIsolate(), "Extension view no longer exists");
     return v8::Undefined(GetIsolate());
   }
 
   v8::Local<v8::Object> modules(v8::Local<v8::Object>::Cast(modules_value));
   v8::Local<v8::Value> exports;
   if (!GetPrivateProperty(v8_context, modules, module_name, &exports) ||
       !exports->IsUndefined())
     return handle_scope.Escape(exports);
 
+  if (!create)
+    return v8::Undefined(GetIsolate());
+
   exports = LoadModule(*v8::String::Utf8Value(module_name));
   SetPrivateProperty(v8_context, modules, module_name, exports);
   return handle_scope.Escape(exports);
 }
 
 void ModuleSystem::CallModuleMethodSafe(const std::string& module_name,
                                         const std::string& method_name) {
   v8::HandleScope handle_scope(GetIsolate());
   v8::Local<v8::Value> no_args;
   CallModuleMethodSafe(module_name, method_name, 0, &no_args,
@@ skipping 24 matching lines @@
     const ScriptInjectionCallback::CompleteCallback& callback) {
   TRACE_EVENT2("v8", "v8.callModuleMethodSafe", "module_name", module_name,
                "method_name", method_name);
 
   v8::HandleScope handle_scope(GetIsolate());
   v8::Local<v8::Context> v8_context = context()->v8_context();
   v8::Context::Scope context_scope(v8_context);
 
   v8::Local<v8::Function> function =
       GetModuleFunction(module_name, method_name);
-  if (function.IsEmpty()) {
-    NOTREACHED() << "GetModuleFunction() returns empty function handle";
+  if (function.IsEmpty())
     return;

[Devlin, 2017/06/16 14:53:16]

Note: I'll add a comment explaining when this can legitimately happen.

[lazyboy, 2017/06/23 21:28:13]

OK, I'd be interested to know.

[Devlin, 2017/06/24 02:04:21]

Done.

-  }
 
   {
     v8::TryCatch try_catch(GetIsolate());
     try_catch.SetCaptureMessage(true);
     context_->SafeCallFunction(function, argc, argv, callback);
     if (try_catch.HasCaught())
       HandleException(try_catch);
   }
 }
 
@@ skipping 253 matching lines @@
       GetIsolate()->ThrowException(
           ToV8StringUnsafe(GetIsolate(), "Natives disabled"));
       return v8::MaybeLocal<v8::Object>();
     }
     Fatal(context_, "Natives disabled for requireNative(" + native_name + ")");
     return v8::MaybeLocal<v8::Object>();
   }
 
   if (overridden_native_handlers_.count(native_name) > 0u) {
     v8::Local<v8::Value> value = RequireForJsInner(
-        ToV8StringUnsafe(GetIsolate(), native_name.c_str()));
+        ToV8StringUnsafe(GetIsolate(), native_name.c_str()), true /* create */);
     if (value.IsEmpty() || !value->IsObject())
       return v8::MaybeLocal<v8::Object>();
     return value.As<v8::Object>();
   }
 
   NativeHandlerMap::iterator i = native_handler_map_.find(native_name);
   if (i == native_handler_map_.end()) {
     Fatal(context_,
           "Couldn't find native for requireNative(" + native_name + ")");
     return v8::MaybeLocal<v8::Object>();
@@ skipping 243 matching lines @@
     const std::string& method_name) {
   v8::Local<v8::String> v8_module_name;
   v8::Local<v8::String> v8_method_name;
   v8::Local<v8::Function> function;
   if (!ToV8String(GetIsolate(), module_name.c_str(), &v8_module_name) ||
       !ToV8String(GetIsolate(), method_name.c_str(), &v8_method_name)) {
     return function;
   }
 
   v8::Local<v8::Value> module;
-  {
-    NativesEnabledScope natives_enabled(this);
-    module = RequireForJsInner(v8_module_name);
-  }
+  // Important: don't create the module if it doesn't exist. Doing so would
+  // force a call into JS, which is something we want to avoid in case it has
+  // been suspended. Additionally, we should only be calling module methods for
+  // modules that have been instantiated.
+  bool create = false;
+  module = RequireForJsInner(v8_module_name, create);
+
+  if (!module.IsEmpty() && module->IsUndefined())

[lazyboy, 2017/06/23 21:28:13]

Add a note what this (!IsEmpty but IsUndefined = true) signifies.

[Devlin, 2017/06/24 02:04:21]

Done.

+    return function;

[lazyboy, 2017/06/23 21:28:13]

nit: do you think it makes it more readable to "return
v8::Local<v8::Function>()" for empty cases here and in lines 866, 883 etc.
Otherwise, reader has to scan lines above to check what |function| contains...
|function| is only set in line 894 and its declaration can be moved there.

[Devlin, 2017/06/24 02:04:21]

Sure, that's reasonable.  It means we lose RVO, but that shouldn't matter with
v8::Locals.

 
   if (module.IsEmpty() || !module->IsObject()) {
     Fatal(context_,
           "Failed to get module " + module_name + " to call " + method_name);
     return function;
   }
 
   v8::Local<v8::Object> object = v8::Local<v8::Object>::Cast(module);
   v8::Local<v8::Value> value;
   if (!GetProperty(context()->v8_context(), object, v8_method_name, &value) ||
       !value->IsFunction()) {
     Fatal(context_, module_name + "." + method_name + " is not a function");
     return function;
   }
 
   function = v8::Local<v8::Function>::Cast(value);
   return function;
 }
 
 }  // namespace extensions