extensions: Use V8 Maybe APIs in ModuleSystem

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 05b51b2534dac7c628be080ca03ed8d3bfec6f00..73e49292c81db48fd79ce5f2370f92a0a24fa8ea 100644
--- a/extensions/renderer/module_system.cc
+++ b/extensions/renderer/module_system.cc
@@ -18,6 +18,7 @@
 #include "extensions/renderer/console.h"
 #include "extensions/renderer/safe_builtins.h"
 #include "extensions/renderer/script_context.h"
+#include "extensions/renderer/v8_maybe_helpers.h"
 #include "gin/modules/module_registry.h"
 #include "third_party/WebKit/public/web/WebFrame.h"
 #include "third_party/WebKit/public/web/WebScopedMicrotaskSuppression.h"
@@ -69,7 +70,7 @@ void Warn(v8::Isolate* isolate, const std::string& message) {
 class DefaultExceptionHandler : public ModuleSystem::ExceptionHandler {
  public:
   explicit DefaultExceptionHandler(ScriptContext* context)
-      : context_(context) {}
+      : ModuleSystem::ExceptionHandler(context) {}
 
   // Fatally dumps the debug info from |try_catch| to the console.
   // Make sure this is never used for exceptions that originate in external
@@ -77,14 +78,16 @@ class DefaultExceptionHandler : public ModuleSystem::ExceptionHandler {
   void HandleUncaughtException(const v8::TryCatch& try_catch) override {
     v8::HandleScope handle_scope(context_->isolate());
     std::string stack_trace = "<stack trace unavailable>";
-    if (!try_catch.StackTrace().IsEmpty()) {
-      v8::String::Utf8Value stack_value(try_catch.StackTrace());
+    v8::Local<v8::Value> v8_stack_trace;
+    if (try_catch.StackTrace(context_->v8_context()).ToLocal(&v8_stack_trace)) {
+      v8::String::Utf8Value stack_value(v8_stack_trace);
       if (*stack_value)
         stack_trace.assign(*stack_value, stack_value.length());
       else
         stack_trace = "<could not convert stack trace to string>";
     }
-    Fatal(context_, CreateExceptionString(try_catch) + "{" + stack_trace + "}");
+    Fatal(context_,
+          CreateExceptionString(try_catch) + "{" + stack_trace + "}");
   }
 
  private:
@@ -113,9 +116,11 @@ std::string ModuleSystem::ExceptionHandler::CreateExceptionString(
     error_message.assign(*error_message_v8, error_message_v8.length());
   }
 
+  auto maybe = message->GetLineNumber(context_->v8_context());
+  int line_number = maybe.IsJust() ? maybe.FromJust() : 0;
   return base::StringPrintf("%s:%d: %s",
                             resource_name.c_str(),
-                            message->GetLineNumber(),
+                            line_number,
                             error_message.c_str());
 }
 
@@ -140,9 +145,9 @@ ModuleSystem::ModuleSystem(ScriptContext* context, SourceMap* source_map)
 
   v8::Local<v8::Object> global(context->v8_context()->Global());
   v8::Isolate* isolate = context->isolate();
-  global->SetHiddenValue(v8::String::NewFromUtf8(isolate, kModulesField),
+  global->SetHiddenValue(ToV8String(isolate, kModulesField),
                          v8::Object::New(isolate));
-  global->SetHiddenValue(v8::String::NewFromUtf8(isolate, kModuleSystem),
+  global->SetHiddenValue(ToV8String(isolate, kModuleSystem),
                          v8::External::New(isolate, this));
 
   gin::ModuleRegistry::From(context->v8_context())->AddObserver(this);
@@ -161,10 +166,8 @@ void ModuleSystem::Invalidate() {
   {
     v8::HandleScope scope(GetIsolate());
     v8::Local<v8::Object> global = context()->v8_context()->Global();
-    global->DeleteHiddenValue(
-        v8::String::NewFromUtf8(GetIsolate(), kModulesField));
-    global->DeleteHiddenValue(
-        v8::String::NewFromUtf8(GetIsolate(), kModuleSystem));
+    global->DeleteHiddenValue(ToV8String(GetIsolate(), kModulesField));
+    global->DeleteHiddenValue(ToV8String(GetIsolate(), kModuleSystem));
   }
 
   // Invalidate all active and clobbered NativeHandlers we own.
@@ -192,21 +195,26 @@ void ModuleSystem::HandleException(const v8::TryCatch& try_catch) {
 }
 
 v8::Local<v8::Value> ModuleSystem::Require(const std::string& module_name) {
+  if (module_name.size() >= v8::String::kMaxLength)
+    return v8::Undefined(GetIsolate());
   v8::EscapableHandleScope handle_scope(GetIsolate());
   return handle_scope.Escape(RequireForJsInner(
-      v8::String::NewFromUtf8(GetIsolate(), module_name.c_str())));
+      ToV8String(GetIsolate(), module_name.c_str())));
 }
 
 void ModuleSystem::RequireForJs(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
-  v8::Local<v8::String> module_name = args[0]->ToString(args.GetIsolate());
+  if (!args[0]->IsString())
+    return;

[not at google - send to devlin, 2015/06/12 16:51:36]

If this isn't a string it means that JS called require(something) where
something isn't a string. That's almost certainly a Chrome programmer error, so
I'd rather that not be a silent failure. It would be nice to throw a JS
exception then (or just LOG(FATAL); they amount to the the same thing) but there
exists a bug where a third party did manage to call require(), so perhaps that
isn't wise.

Well, the worst they can do is crash themselves, I suppose. Something to think
about.

How about:

if (!args[0]->IsString()) {
  NOTREACHED() << "require() called with a non-string argument";
  return;
}

to at least catch Chrome programmer error to some degree.

[bashi, 2015/06/16 03:12:46]

Done.

+  v8::Local<v8::String> module_name = args[0].As<v8::String>();
   args.GetReturnValue().Set(RequireForJsInner(module_name));
 }
 
 v8::Local<v8::Value> ModuleSystem::RequireForJsInner(
     v8::Local<v8::String> module_name) {
   v8::EscapableHandleScope handle_scope(GetIsolate());
-  v8::Context::Scope context_scope(context()->v8_context());
+  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());
 
@@ -214,19 +222,20 @@ v8::Local<v8::Value> ModuleSystem::RequireForJsInner(
   // 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 = global->GetHiddenValue(
-      v8::String::NewFromUtf8(GetIsolate(), kModulesField));
+      ToV8String(GetIsolate(), kModulesField));
   if (modules_value.IsEmpty() || 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(modules->Get(module_name));
-  if (!exports->IsUndefined())
+  v8::Local<v8::Value> exports;
+  if (!modules->Get(v8_context, module_name).ToLocal(&exports) ||
+      !exports->IsUndefined())
     return handle_scope.Escape(exports);
 
   exports = LoadModule(*v8::String::Utf8Value(module_name));
-  modules->Set(module_name, exports);
+  SetProperty(v8_context, modules, module_name, exports);
   return handle_scope.Escape(exports);
 }
 
@@ -260,13 +269,18 @@ v8::Local<v8::Value> ModuleSystem::CallModuleMethod(
                method_name);
 
   v8::EscapableHandleScope handle_scope(GetIsolate());
-  v8::Context::Scope context_scope(context()->v8_context());
+  v8::Local<v8::Context> v8_context = context()->v8_context();
+  v8::Context::Scope context_scope(v8_context);
+
+  if (module_name.size() >= v8::String::kMaxLength ||

[not at google - send to devlin, 2015/06/12 16:51:36]

It would be nice to build a version of ToV8String that takes these checks into
account, otherwise it'll probably regress. Possibly another version of
ToV8String that takes a pointer to a handle and returns a bool?

v8::Local<v8::Context> v8_module_name, v8_method_name;
if (!ToV8String(GetIsolate(), module_name, &v8_module_name) ||
    !ToV8String(GetIsolate(), method_name, &v8_method_name)) {
  return ...;
}

Overloading ToV8String in that way is probably a bad idea, so I'd either get rid
of the handle-returning version, or have a name like ToV8StringUnsafe maybe?

[bashi, 2015/06/16 03:12:46]

Added.
Renamed ToV8String() to ToV8StringUnsafe().

+      method_name.size() >= v8::String::kMaxLength)
+    return handle_scope.Escape(
+        v8::Local<v8::Primitive>(v8::Undefined(GetIsolate())));

[not at google - send to devlin, 2015/06/12 16:51:36]

Both the condition and body of this function are multiple lines, I think the
body should be wrapped in {}.

[bashi, 2015/06/16 03:12:47]

Done.

 
   v8::Local<v8::Value> module;
   {
     NativesEnabledScope natives_enabled(this);
-    module = RequireForJsInner(
-        v8::String::NewFromUtf8(GetIsolate(), module_name.c_str()));
+    module = RequireForJsInner(ToV8String(GetIsolate(), module_name.c_str()));
   }
 
   if (module.IsEmpty() || !module->IsObject()) {
@@ -276,9 +290,12 @@ v8::Local<v8::Value> ModuleSystem::CallModuleMethod(
         v8::Local<v8::Primitive>(v8::Undefined(GetIsolate())));
   }
 
-  v8::Local<v8::Value> value = v8::Local<v8::Object>::Cast(module)->Get(
-      v8::String::NewFromUtf8(GetIsolate(), method_name.c_str()));
-  if (value.IsEmpty() || !value->IsFunction()) {
+  v8::Local<v8::Object> object = v8::Local<v8::Object>::Cast(module);
+  v8::Local<v8::String> v8_method_name =
+      ToV8String(GetIsolate(), method_name.c_str());
+  v8::Local<v8::Value> value;
+  if (!object->Get(v8_context, v8_method_name).ToLocal(&value) ||
+      !value->IsFunction()) {
     Fatal(context_, module_name + "." + method_name + " is not a function");
     return handle_scope.Escape(
         v8::Local<v8::Primitive>(v8::Undefined(GetIsolate())));
@@ -287,11 +304,13 @@ v8::Local<v8::Value> ModuleSystem::CallModuleMethod(
   v8::Local<v8::Function> func = v8::Local<v8::Function>::Cast(value);
   v8::Local<v8::Value> result;
   {
-    v8::TryCatch try_catch;
+    v8::TryCatch try_catch(GetIsolate());
     try_catch.SetCaptureMessage(true);
     result = context_->CallFunction(func, argc, argv);
-    if (try_catch.HasCaught())
+    if (try_catch.HasCaught()) {
       HandleException(try_catch);
+      result = v8::Local<v8::Primitive>(v8::Undefined(GetIsolate()));

[not at google - send to devlin, 2015/06/12 16:51:36]

Arghhhh so much verbosity. Why do methods like v8::Undefined and v8::Object::Get
not automatically return a v8::Local rather than a v8::Handle?

[bashi, 2015/06/16 03:12:47]

Not sure, but maybe a historical reason?

+    }
   }
   return handle_scope.Escape(result);
 }
@@ -311,22 +330,28 @@ void ModuleSystem::OverrideNativeHandlerForTest(const std::string& name) {
 
 void ModuleSystem::RunString(const std::string& code, const std::string& name) {
   v8::HandleScope handle_scope(GetIsolate());
-  RunString(v8::String::NewFromUtf8(GetIsolate(), code.c_str()),
-            v8::String::NewFromUtf8(GetIsolate(), name.c_str()));
+  if (code.size() >= v8::String::kMaxLength ||
+      name.size() >= v8::String::kMaxLength) {
+    Warn(GetIsolate(), "Too long code or name.");
+    return;
+  }
+  RunString(ToV8String(GetIsolate(), code.c_str()),
+            ToV8String(GetIsolate(), name.c_str()));
 }
 
 // static
 void ModuleSystem::NativeLazyFieldGetter(
-    v8::Local<v8::String> property,
+    v8::Local<v8::Name> property,
     const v8::PropertyCallbackInfo<v8::Value>& info) {
-  LazyFieldGetterInner(property, info, &ModuleSystem::RequireNativeFromString);
+  LazyFieldGetterInner(property.As<v8::String>(), info,
+                       &ModuleSystem::RequireNativeFromString);
 }
 
 // static
 void ModuleSystem::LazyFieldGetter(
-    v8::Local<v8::String> property,
+    v8::Local<v8::Name> property,
     const v8::PropertyCallbackInfo<v8::Value>& info) {
-  LazyFieldGetterInner(property, info, &ModuleSystem::Require);
+  LazyFieldGetterInner(property.As<v8::String>(), info, &ModuleSystem::Require);
 }
 
 // static
@@ -342,7 +367,7 @@ void ModuleSystem::LazyFieldGetterInner(
   v8::Local<v8::Context> context = parameters->CreationContext();
   v8::Local<v8::Object> global(context->Global());
   v8::Local<v8::Value> module_system_value = global->GetHiddenValue(
-      v8::String::NewFromUtf8(info.GetIsolate(), kModuleSystem));
+      ToV8String(info.GetIsolate(), kModuleSystem));
   if (module_system_value.IsEmpty() || !module_system_value->IsExternal()) {
     // ModuleSystem has been deleted.
     // TODO(kalman): See comment in header file.
@@ -354,15 +379,21 @@ void ModuleSystem::LazyFieldGetterInner(
   ModuleSystem* module_system = static_cast<ModuleSystem*>(
       v8::Local<v8::External>::Cast(module_system_value)->Value());
 
-  std::string name = *v8::String::Utf8Value(parameters->Get(
-      v8::String::NewFromUtf8(info.GetIsolate(), kModuleName)));
+  v8::Local<v8::String> v8_module_name =
+      ToV8String(info.GetIsolate(), kModuleName);
+  v8::Local<v8::Value> v8_name;

[not at google - send to devlin, 2015/06/12 16:51:36]

This should be the one called v8_module_name (with name --> module_name), versus
what's currently called |v8_module_name| instead called |v8_k_module_name| or
just inlined. It should always be safe to cast to a v8 value, it would be crazy
not to be.

[bashi, 2015/06/16 03:12:47]

Done.

+  if (!parameters->Get(context, v8_module_name).ToLocal(&v8_name)) {
+    Warn(info.GetIsolate(), "Cannot find module.");
+    return;
+  }
+  std::string name = *v8::String::Utf8Value(v8_name);
 
   // Switch to our v8 context because we need functions created while running
   // the require()d module to belong to our context, not the current one.
   v8::Context::Scope context_scope(context);
   NativesEnabledScope natives_enabled_scope(module_system);
 
-  v8::TryCatch try_catch;
+  v8::TryCatch try_catch(info.GetIsolate());
   v8::Local<v8::Value> module_value = (module_system->*require_function)(name);
   if (try_catch.HasCaught()) {
     module_system->HandleException(try_catch);
@@ -374,11 +405,19 @@ void ModuleSystem::LazyFieldGetterInner(
   }
 
   v8::Local<v8::Object> module = v8::Local<v8::Object>::Cast(module_value);
-  v8::Local<v8::String> field =
-      parameters->Get(v8::String::NewFromUtf8(info.GetIsolate(), kModuleField))
-          ->ToString(info.GetIsolate());
+  v8::Local<v8::Value> field_value;
+  if (!parameters->Get(context, ToV8String(info.GetIsolate(), kModuleField))
+           .ToLocal(&field_value)) {
+    module_system->HandleException(try_catch);
+    return;
+  }
+  v8::Local<v8::String> field;
+  if (!field_value->ToString(context).ToLocal(&field)) {
+    module_system->HandleException(try_catch);
+    return;
+  }
 
-  if (!module->Has(field)) {
+  if (!CheckV8Call(module->Has(context, field))) {
     std::string field_str = *v8::String::Utf8Value(field);
     Fatal(module_system->context_,
           "Lazy require of " + name + "." + field_str + " did not set the " +
@@ -386,8 +425,8 @@ void ModuleSystem::LazyFieldGetterInner(
     return;
   }
 
-  v8::Local<v8::Value> new_field = module->Get(field);
-  if (try_catch.HasCaught()) {
+  v8::Local<v8::Value> new_field;
+  if (!module->Get(context, field).ToLocal(&new_field)) {
     module_system->HandleException(try_catch);
     return;
   }
@@ -401,8 +440,8 @@ void ModuleSystem::LazyFieldGetterInner(
   v8::Local<v8::Value> val = info.This();
   if (val->IsObject()) {
     v8::Local<v8::Object> object = v8::Local<v8::Object>::Cast(val);
-    object->Delete(property);
-    object->Set(property, new_field);
+    object->Delete(context, property);
+    SetProperty(context, object, property, new_field);
   } else {
     NOTREACHED();
   }
@@ -421,17 +460,21 @@ void ModuleSystem::SetLazyField(v8::Local<v8::Object> object,
                                 const std::string& field,
                                 const std::string& module_name,
                                 const std::string& module_field,
-                                v8::AccessorGetterCallback getter) {
+                                v8::AccessorNameGetterCallback getter) {
+  CHECK(field.size() < v8::String::kMaxLength);
+  CHECK(module_name.size() < v8::String::kMaxLength);
+  CHECK(module_field.size() < v8::String::kMaxLength);
   v8::HandleScope handle_scope(GetIsolate());
   v8::Local<v8::Object> parameters = v8::Object::New(GetIsolate());
-  parameters->Set(v8::String::NewFromUtf8(GetIsolate(), kModuleName),
-                  v8::String::NewFromUtf8(GetIsolate(), module_name.c_str()));
-  parameters->Set(v8::String::NewFromUtf8(GetIsolate(), kModuleField),
-                  v8::String::NewFromUtf8(GetIsolate(), module_field.c_str()));
-  object->SetAccessor(v8::String::NewFromUtf8(GetIsolate(), field.c_str()),
-                      getter,
-                      NULL,
-                      parameters);
+  v8::Local<v8::Context> context = context_->v8_context();
+  SetProperty(context, parameters, ToV8String(GetIsolate(), kModuleName),
+              ToV8String(GetIsolate(), module_name.c_str()));
+  SetProperty(context, parameters, ToV8String(GetIsolate(), kModuleField),
+              ToV8String(GetIsolate(), module_field.c_str()));
+  auto maybe =
+      object->SetAccessor(context, ToV8String(GetIsolate(), field.c_str()),
+                          getter, NULL, parameters);
+  DCHECK(maybe.IsJust() && maybe.FromJust());
 }
 
 void ModuleSystem::SetNativeLazyField(v8::Local<v8::Object> object,
@@ -448,27 +491,29 @@ void ModuleSystem::SetNativeLazyField(v8::Local<v8::Object> object,
 v8::Local<v8::Value> ModuleSystem::RunString(v8::Local<v8::String> code,
                                              v8::Local<v8::String> name) {
   v8::EscapableHandleScope handle_scope(GetIsolate());
-  v8::Context::Scope context_scope(context()->v8_context());
+  v8::Local<v8::Context> v8_context = context()->v8_context();
+  v8::Context::Scope context_scope(v8_context);
 
   // Prepend extensions:: to |name| so that internal code can be differentiated
   // from external code in stack traces. This has no effect on behaviour.
   std::string internal_name =
       base::StringPrintf("extensions::%s", *v8::String::Utf8Value(name));
 
+  if (internal_name.size() >= v8::String::kMaxLength)

[not at google - send to devlin, 2015/06/12 16:51:36]

Generally I think many of these failures should have a NOTREACHED or LOG(FATAL)
in their body, so as to not hide mysterious failures.

[bashi, 2015/06/16 03:12:47]

Done.

+    return v8::Undefined(GetIsolate());
+
   blink::WebScopedMicrotaskSuppression suppression;
-  v8::TryCatch try_catch;
+  v8::TryCatch try_catch(GetIsolate());
   try_catch.SetCaptureMessage(true);
-  v8::Local<v8::Script> script(v8::Script::Compile(
-      code, v8::String::NewFromUtf8(GetIsolate(), internal_name.c_str(),
-                                    v8::String::kNormalString,
-                                    internal_name.size())));
-  if (try_catch.HasCaught()) {
+  v8::ScriptOrigin origin(ToV8String(GetIsolate(), internal_name.c_str()));
+  v8::Local<v8::Script> script;
+  if (!v8::Script::Compile(v8_context, code, &origin).ToLocal(&script)) {
     HandleException(try_catch);
     return v8::Undefined(GetIsolate());
   }
 
-  v8::Local<v8::Value> result = script->Run();
-  if (try_catch.HasCaught()) {
+  v8::Local<v8::Value> result;
+  if (!script->Run(v8_context).ToLocal(&result)) {
     HandleException(try_catch);
     return v8::Undefined(GetIsolate());
   }
@@ -499,7 +544,7 @@ v8::Local<v8::Value> ModuleSystem::RequireNativeFromString(
     // we could crash.
     if (exception_handler_) {
       return GetIsolate()->ThrowException(
-          v8::String::NewFromUtf8(GetIsolate(), "Natives disabled"));
+          ToV8String(GetIsolate(), "Natives disabled"));
     }
     Fatal(context_, "Natives disabled for requireNative(" + native_name + ")");
     return v8::Undefined(GetIsolate());
@@ -507,7 +552,7 @@ v8::Local<v8::Value> ModuleSystem::RequireNativeFromString(
 
   if (overridden_native_handlers_.count(native_name) > 0u) {
     return RequireForJsInner(
-        v8::String::NewFromUtf8(GetIsolate(), native_name.c_str()));
+        ToV8String(GetIsolate(), native_name.c_str()));
   }
 
   NativeHandlerMap::iterator i = native_handler_map_.find(native_name);
@@ -523,16 +568,17 @@ void ModuleSystem::RequireAsync(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   CHECK_EQ(1, args.Length());
   std::string module_name = *v8::String::Utf8Value(args[0]);
+  v8::Local<v8::Context> v8_context = context_->v8_context();
   v8::Local<v8::Promise::Resolver> resolver(
-      v8::Promise::Resolver::New(GetIsolate()));
+      v8::Promise::Resolver::New(v8_context).ToLocalChecked());
   args.GetReturnValue().Set(resolver->GetPromise());
   scoped_ptr<v8::Global<v8::Promise::Resolver>> global_resolver(
       new v8::Global<v8::Promise::Resolver>(GetIsolate(), resolver));
   gin::ModuleRegistry* module_registry =
-      gin::ModuleRegistry::From(context_->v8_context());
+      gin::ModuleRegistry::From(v8_context);
   if (!module_registry) {
     Warn(GetIsolate(), "Extension view no longer exists");
-    resolver->Reject(v8::Exception::Error(v8::String::NewFromUtf8(
+    resolver->Reject(v8_context, v8::Exception::Error(ToV8String(
         GetIsolate(), "Extension view no longer exists")));
     return;
   }
@@ -547,13 +593,13 @@ void ModuleSystem::RequireAsync(
 v8::Local<v8::String> ModuleSystem::WrapSource(v8::Local<v8::String> source) {
   v8::EscapableHandleScope handle_scope(GetIsolate());
   // Keep in order with the arguments in RequireForJsInner.
-  v8::Local<v8::String> left = v8::String::NewFromUtf8(
+  v8::Local<v8::String> left = ToV8String(
       GetIsolate(),
       "(function(define, require, requireNative, requireAsync, exports, "
       "console, privates,"
       "$Array, $Function, $JSON, $Object, $RegExp, $String, $Error) {"
       "'use strict';");
-  v8::Local<v8::String> right = v8::String::NewFromUtf8(GetIsolate(), "\n})");
+  v8::Local<v8::String> right = ToV8String(GetIsolate(), "\n})");
   return handle_scope.Escape(v8::Local<v8::String>(
       v8::String::Concat(left, v8::String::Concat(source, right))));
 }
@@ -562,7 +608,7 @@ void ModuleSystem::Private(const v8::FunctionCallbackInfo<v8::Value>& args) {
   CHECK_EQ(1, args.Length());
   if (!args[0]->IsObject() || args[0]->IsNull()) {
     GetIsolate()->ThrowException(
-        v8::Exception::TypeError(v8::String::NewFromUtf8(GetIsolate(),
+        v8::Exception::TypeError(ToV8String(GetIsolate(),
             args[0]->IsUndefined()
                 ? "Method called without a valid receiver (this). "
                   "Did you forget to call .bind()?"
@@ -570,14 +616,13 @@ void ModuleSystem::Private(const v8::FunctionCallbackInfo<v8::Value>& args) {
     return;
   }
   v8::Local<v8::Object> obj = args[0].As<v8::Object>();
-  v8::Local<v8::String> privates_key =
-      v8::String::NewFromUtf8(GetIsolate(), "privates");
+  v8::Local<v8::String> privates_key = ToV8String(GetIsolate(), "privates");
   v8::Local<v8::Value> privates = obj->GetHiddenValue(privates_key);
   if (privates.IsEmpty()) {
     privates = v8::Object::New(args.GetIsolate());
     if (privates.IsEmpty()) {
       GetIsolate()->ThrowException(
-          v8::String::NewFromUtf8(GetIsolate(), "Failed to create privates"));
+          ToV8String(GetIsolate(), "Failed to create privates"));
       return;
     }
     obj->SetHiddenValue(privates_key, privates);
@@ -587,7 +632,8 @@ void ModuleSystem::Private(const v8::FunctionCallbackInfo<v8::Value>& args) {
 
 v8::Local<v8::Value> ModuleSystem::LoadModule(const std::string& module_name) {
   v8::EscapableHandleScope handle_scope(GetIsolate());
-  v8::Context::Scope context_scope(context()->v8_context());
+  v8::Local<v8::Context> v8_context = context()->v8_context();
+  v8::Context::Scope context_scope(v8_context);
 
   v8::Local<v8::Value> source(GetSource(module_name));
   if (source.IsEmpty() || source->IsUndefined()) {
@@ -596,10 +642,14 @@ v8::Local<v8::Value> ModuleSystem::LoadModule(const std::string& module_name) {
   }
   v8::Local<v8::String> wrapped_source(
       WrapSource(v8::Local<v8::String>::Cast(source)));
+  if (module_name.size() >= v8::String::kMaxLength) {
+    Fatal(context_, "Module name is too long");
+    return v8::Undefined(GetIsolate());
+  }
   // Modules are wrapped in (function(){...}) so they always return functions.
   v8::Local<v8::Value> func_as_value =
       RunString(wrapped_source,
-                v8::String::NewFromUtf8(GetIsolate(), module_name.c_str()));
+                ToV8String(GetIsolate(), module_name.c_str()));
   if (func_as_value.IsEmpty() || func_as_value->IsUndefined()) {
     Fatal(context_, "Bad source for require(" + module_name + ")");
     return v8::Undefined(GetIsolate());
@@ -612,24 +662,28 @@ v8::Local<v8::Value> ModuleSystem::LoadModule(const std::string& module_name) {
 
   v8::Local<v8::Value> exports = v8::Object::New(GetIsolate());
   v8::Local<v8::Object> natives(NewInstance());
-  CHECK(!natives.IsEmpty());  // this can happen if v8 has issues
+  CHECK(!natives.IsEmpty());  // this can fail if v8 has issues
 
   // These must match the argument order in WrapSource.
   v8::Local<v8::Value> args[] = {
       // AMD.
-      define_object->Get(v8::String::NewFromUtf8(GetIsolate(), "define")),
+      UnsafeGet(v8_context, define_object, ToV8String(GetIsolate(), "define")),
       // CommonJS.
-      natives->Get(v8::String::NewFromUtf8(GetIsolate(), "require",
-                                           v8::String::kInternalizedString)),
-      natives->Get(v8::String::NewFromUtf8(GetIsolate(), "requireNative",
-                                           v8::String::kInternalizedString)),
-      natives->Get(v8::String::NewFromUtf8(GetIsolate(), "requireAsync",
-                                           v8::String::kInternalizedString)),
+      UnsafeGet(v8_context, natives,
+                ToV8String(GetIsolate(), "require",
+                           v8::NewStringType::kInternalized)),
+      UnsafeGet(v8_context, natives,
+                ToV8String(GetIsolate(), "requireNative",
+                           v8::NewStringType::kInternalized)),
+      UnsafeGet(v8_context, natives,
+                ToV8String(GetIsolate(), "requireAsync",
+                           v8::NewStringType::kInternalized)),
       exports,
       // Libraries that we magically expose to every module.
       console::AsV8Object(GetIsolate()),
-      natives->Get(v8::String::NewFromUtf8(GetIsolate(), "privates",
-                                           v8::String::kInternalizedString)),
+      UnsafeGet(v8_context, natives,
+                ToV8String(GetIsolate(), "privates",
+                           v8::NewStringType::kInternalized)),
       // Each safe builtin. Keep in order with the arguments in WrapSource.
       context_->safe_builtins()->GetArray(),
       context_->safe_builtins()->GetFunction(),
@@ -640,7 +694,7 @@ v8::Local<v8::Value> ModuleSystem::LoadModule(const std::string& module_name) {
       context_->safe_builtins()->GetError(),
   };
   {
-    v8::TryCatch try_catch;
+    v8::TryCatch try_catch(GetIsolate());
     try_catch.SetCaptureMessage(true);
     context_->CallFunction(func, arraysize(args), args);
     if (try_catch.HasCaught()) {
@@ -680,7 +734,7 @@ void ModuleSystem::OnModuleLoaded(
   v8::HandleScope handle_scope(GetIsolate());
   v8::Local<v8::Promise::Resolver> resolver_local(
       v8::Local<v8::Promise::Resolver>::New(GetIsolate(), *resolver));
-  resolver_local->Resolve(value);
+  resolver_local->Resolve(GetIsolate()->GetCurrentContext(), value);

[not at google - send to devlin, 2015/06/12 16:51:36]

Can this be context() rather than GetCurrentContext()?

[bashi, 2015/06/16 03:12:47]

Done.

 }
 
 void ModuleSystem::ClobberExistingNativeHandler(const std::string& name) {