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

extensions/renderer/module_system.cc

Index: extensions/renderer/module_system.cc
diff --git a/extensions/renderer/module_system.cc b/extensions/renderer/module_system.cc
index 2349218a544884d9bd8b341915c78bcb5a4b8c03..bf927c1d0f75ba6560871198bc3a9379f7e5f736 100644
--- a/extensions/renderer/module_system.cc
+++ b/extensions/renderer/module_system.cc
@@ -239,8 +239,8 @@ v8::MaybeLocal<v8::Object> ModuleSystem::Require(
   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>());
@@ -253,11 +253,12 @@ void ModuleSystem::RequireForJs(
     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);
@@ -280,6 +281,9 @@ v8::Local<v8::Value> ModuleSystem::RequireForJsInner(
       !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);
@@ -324,10 +328,8 @@ void ModuleSystem::CallModuleMethodSafe(
 
   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());
@@ -601,7 +603,7 @@ v8::MaybeLocal<v8::Object> ModuleSystem::RequireNativeFromString(
 
   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>();
@@ -865,10 +867,15 @@ v8::Local<v8::Function> ModuleSystem::GetModuleFunction(
   }
 
   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_,