Attempting to fix problems in 11571014.

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 74e7d3d8d998fc99b1fc719e6dd32f4aff94342d..75eb2b97be5d687cc8782c602f49e2f40d10f1f6 100644
--- a/chrome/renderer/extensions/module_system.cc
+++ b/chrome/renderer/extensions/module_system.cc
@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "base/stl_util.h"
+#include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "chrome/common/extensions/extension_messages.h"
 #include "chrome/renderer/extensions/chrome_v8_context.h"
@@ -27,8 +28,7 @@ ModuleSystem::ModuleSystem(v8::Handle<v8::Context> context,
                            SourceMap* source_map)
     : ObjectBackedNativeHandler(context),
       source_map_(source_map),
-      natives_enabled_(0),
-      is_valid_(true) {
+      natives_enabled_(0) {
   RouteFunction("require",
       base::Bind(&ModuleSystem::RequireForJs, base::Unretained(this)));
   RouteFunction("requireNative",
@@ -44,16 +44,14 @@ ModuleSystem::~ModuleSystem() {
   Invalidate();
 }
 
-bool ModuleSystem::Invalidate() {
-  if (!ObjectBackedNativeHandler::Invalidate())
-    return false;
-
-  v8::HandleScope handle_scope;
-  // Deleting this value here prevents future lazy field accesses from
-  // referencing ModuleSystem after it has been freed.
-  v8_context()->Global()->DeleteHiddenValue(v8::String::New(kModuleSystem));
-
-  return true;
+void ModuleSystem::Invalidate() {
+  if (!is_valid())
+    return;
+  for (NativeHandlerMap::iterator it = native_handler_map_.begin();
+       it != native_handler_map_.end(); ++it) {
+    it->second->Invalidate();
+  }
+  ObjectBackedNativeHandler::Invalidate();
 }
 
 ModuleSystem::NativesEnabledScope::NativesEnabledScope(
@@ -67,17 +65,6 @@ ModuleSystem::NativesEnabledScope::~NativesEnabledScope() {
   CHECK_GE(module_system_->natives_enabled_, 0);
 }
 
-// static
-bool ModuleSystem::IsPresentInCurrentContext() {
-  // XXX(kalman): Not sure if this is safe. Investigate.
-  v8::Handle<v8::Object> global(v8::Context::GetCurrent()->Global());
-  if (global.IsEmpty())
-    return false;
-  v8::Handle<v8::Value> module_system =
-      global->GetHiddenValue(v8::String::New(kModuleSystem));
-  return !module_system.IsEmpty() && !module_system->IsUndefined();
-}
-
 void ModuleSystem::HandleException(const v8::TryCatch& try_catch) {
   DumpException(try_catch);
   if (exception_handler_.get())
@@ -137,7 +124,6 @@ v8::Handle<v8::Value> ModuleSystem::RequireForJs(const v8::Arguments& args) {
 
 v8::Handle<v8::Value> ModuleSystem::RequireForJsInner(
     v8::Handle<v8::String> module_name) {
-  CHECK(is_valid_);
   v8::HandleScope handle_scope;
   v8::Handle<v8::Object> global(v8_context()->Global());
   v8::Handle<v8::Object> modules(v8::Handle<v8::Object>::Cast(
@@ -253,7 +239,7 @@ v8::Handle<v8::Value> ModuleSystem::LazyFieldGetter(
 v8::Handle<v8::Value> ModuleSystem::LazyFieldGetterInner(
     v8::Local<v8::String> property,
     const v8::AccessorInfo& info,
-    GetModuleFunc get_module) {
+    RequireFn require_fn) {
   CHECK(!info.Data().IsEmpty());
   CHECK(info.Data()->IsObject());
   v8::HandleScope handle_scope;
@@ -271,20 +257,10 @@ v8::Handle<v8::Value> ModuleSystem::LazyFieldGetterInner(
   std::string name = *v8::String::AsciiValue(
       parameters->Get(v8::String::New(kModuleName))->ToString());
 
-  // HACK(kalman): Switch to the context of the owner module system while
-  // lazily requiring modules.
-  //
-  // It seems to be a common incorrect assumption throughout code that the
-  // current context is the owner context. This makes that assumption true for
-  // at least the period where the JavaScript is first evaluated, which is when
-  // things are most likely to go wrong.
-  v8::Context::Scope context_scope(parameters->CreationContext());
-
   NativesEnabledScope natives_enabled_scope(module_system);
-
   v8::TryCatch try_catch;
   v8::Handle<v8::Object> module = v8::Handle<v8::Object>::Cast(
-      (module_system->*get_module)(name));
+      (module_system->*require_fn)(name));
   if (try_catch.HasCaught()) {
     module_system->HandleException(try_catch);
     return handle_scope.Close(v8::Handle<v8::Value>());
@@ -296,10 +272,18 @@ v8::Handle<v8::Value> ModuleSystem::LazyFieldGetterInner(
   v8::Handle<v8::String> field =
       parameters->Get(v8::String::New(kModuleField))->ToString();
 
+  // http://crbug.com/179741.
+  std::string field_name = *v8::String::AsciiValue(field);
+  char stack_debug[64];

[Matt Perry, 2013/03/05 21:51:38]

you might need to use base::Alias or whatever to make this available in crash
dumps.

[not at google - send to devlin, 2013/03/05 22:05:38]

Done.

+  base::snprintf(stack_debug, arraysize(stack_debug),
+                 "%s.%s", name.c_str(), field_name.c_str());
+
   v8::Local<v8::Value> new_field = module->Get(field);
   v8::Handle<v8::Object> object = info.This();
   // Delete the getter and set this field to |new_field| so the same object is
   // returned every time a certain API is accessed.
+  // CHECK is for http://crbug.com/179741.
+  CHECK(!new_field.IsEmpty()) << "Empty require " << name << "." << field_name;
   if (!new_field->IsUndefined()) {
     object->Delete(property);
     object->Set(property, new_field);