Revert "Convert extensions::SafeBuiltins from a v8::Extension to a NativeHandler."

This reverts commit 68c06a78f74212907faafd985b0bdee67a6d4120.

It caused an iframe performance regression. I plan on re-landing this
after doing some profiling to figure out what exactly the problem is.

TBR=rdevlin.cronin@chromium.org, jochen@chromium.org
BUG=517509, 383974

Review URL: https://codereview.chromium.org/1281793003

Cr-Commit-Position: refs/heads/master@{#342858}

11 files changed, 144 insertions, 230 deletions

diff --git a/extensions/renderer/dispatcher.cc b/extensions/renderer/dispatcher.cc
index 94bfb78..e870f86 100644
--- a/extensions/renderer/dispatcher.cc
+++ b/extensions/renderer/dispatcher.cc
@@ -72,6 +72,7 @@
 #include "extensions/renderer/render_frame_observer_natives.h"
 #include "extensions/renderer/request_sender.h"
 #include "extensions/renderer/runtime_custom_bindings.h"
+#include "extensions/renderer/safe_builtins.h"
 #include "extensions/renderer/script_context.h"
 #include "extensions/renderer/script_context_set.h"
 #include "extensions/renderer/script_injection.h"
@@ -738,6 +739,8 @@ bool Dispatcher::OnControlMessageReceived(const IPC::Message& message) {
 }
 
 void Dispatcher::WebKitInitialized() {
+  RenderThread::Get()->RegisterExtension(SafeBuiltins::CreateV8Extension());
+
   // For extensions, we want to ensure we call the IdleHandler every so often,
   // even if the extension keeps up activity.
   if (set_idle_notifications_) {
diff --git a/extensions/renderer/messaging_bindings.cc b/extensions/renderer/messaging_bindings.cc
index adb6a98..1e0f6e2 100644
--- a/extensions/renderer/messaging_bindings.cc
+++ b/extensions/renderer/messaging_bindings.cc
@@ -55,7 +55,7 @@ namespace extensions {
 
 using v8_helpers::ToV8String;
 using v8_helpers::ToV8StringUnsafe;
-using v8_helpers::IsEmptyOrUndefined;
+using v8_helpers::IsEmptyOrUndefied;
 
 namespace {
 
@@ -365,7 +365,7 @@ void DispatchOnConnectToScriptContext(
       script_context->module_system()->CallModuleMethod(
           "messaging", "dispatchOnConnect", arraysize(arguments), arguments);
 
-  if (!IsEmptyOrUndefined(retval)) {
+  if (!IsEmptyOrUndefied(retval)) {
     CHECK(retval->IsBoolean());
     *port_created |= retval.As<v8::Boolean>()->Value();
   } else {
@@ -388,7 +388,7 @@ void DeliverMessageToScriptContext(const Message& message,
                                                         1, &port_id_handle);
   // Could be empty/undefined if an exception was thrown.
   // TODO(kalman): Should this be built into CallModuleMethod?
-  if (IsEmptyOrUndefined(has_port))
+  if (IsEmptyOrUndefied(has_port))
     return;
   CHECK(has_port->IsBoolean());
   if (!has_port.As<v8::Boolean>()->Value())
diff --git a/extensions/renderer/module_system_test.cc b/extensions/renderer/module_system_test.cc
index ebdba3b..50b6416 100644
--- a/extensions/renderer/module_system_test.cc
+++ b/extensions/renderer/module_system_test.cc
@@ -18,6 +18,7 @@
 #include "extensions/common/extension_paths.h"
 #include "extensions/renderer/logging_native_handler.h"
 #include "extensions/renderer/object_backed_native_handler.h"
+#include "extensions/renderer/safe_builtins.h"
 #include "extensions/renderer/utils_native_handler.h"
 #include "ui/base/resource/resource_bundle.h"
 
@@ -32,6 +33,30 @@ class FailsOnException : public ModuleSystem::ExceptionHandler {
   }
 };
 
+class V8ExtensionConfigurator {
+ public:
+  V8ExtensionConfigurator()
+      : safe_builtins_(SafeBuiltins::CreateV8Extension()),
+        names_(1, safe_builtins_->name()),
+        configuration_(
+            new v8::ExtensionConfiguration(static_cast<int>(names_.size()),
+                                           vector_as_array(&names_))) {
+    v8::RegisterExtension(safe_builtins_.get());
+  }
+
+  v8::ExtensionConfiguration* GetConfiguration() {
+    return configuration_.get();
+  }
+
+ private:
+  scoped_ptr<v8::Extension> safe_builtins_;
+  std::vector<const char*> names_;
+  scoped_ptr<v8::ExtensionConfiguration> configuration_;
+};
+
+base::LazyInstance<V8ExtensionConfigurator>::Leaky g_v8_extension_configurator =
+    LAZY_INSTANCE_INITIALIZER;
+
 }  // namespace
 
 // Native JS functions for doing asserts.
@@ -102,7 +127,8 @@ ModuleSystemTestEnvironment::ModuleSystemTestEnvironment(v8::Isolate* isolate)
       context_holder_(new gin::ContextHolder(isolate_)),
       handle_scope_(isolate_),
       source_map_(new StringSourceMap()) {
-  context_holder_->SetContext(v8::Context::New(isolate));
+  context_holder_->SetContext(v8::Context::New(
+      isolate, g_v8_extension_configurator.Get().GetConfiguration()));
   context_.reset(new ScriptContext(context_holder_->context(),
                                    nullptr,  // WebFrame
                                    nullptr,  // Extension
diff --git a/extensions/renderer/object_backed_native_handler.cc b/extensions/renderer/object_backed_native_handler.cc
index f93610d..060606b 100644
--- a/extensions/renderer/object_backed_native_handler.cc
+++ b/extensions/renderer/object_backed_native_handler.cc
@@ -10,13 +10,10 @@
 #include "extensions/renderer/module_system.h"
 #include "extensions/renderer/script_context.h"
 #include "extensions/renderer/script_context_set.h"
-#include "extensions/renderer/v8_helpers.h"
 #include "v8/include/v8.h"
 
 namespace extensions {
 
-using namespace v8_helpers;
-
 namespace {
 // Key for the base::Bound routed function.
 const char* kHandlerFunction = "handler_function";
@@ -46,7 +43,8 @@ void ObjectBackedNativeHandler::Router(
   v8::Local<v8::Value> handler_function_value =
       data->Get(v8::String::NewFromUtf8(args.GetIsolate(), kHandlerFunction));
   // See comment in header file for why we do this.
-  if (IsEmptyOrUndefined(handler_function_value)) {
+  if (handler_function_value.IsEmpty() ||
+      handler_function_value->IsUndefined()) {
     ScriptContext* script_context = ScriptContextSet::GetContextByV8Context(
         args.GetIsolate()->GetCallingContext());
     console::Error(script_context ? script_context->GetRenderFrame() : nullptr,
@@ -63,15 +61,12 @@ void ObjectBackedNativeHandler::RouteFunction(
     const HandlerFunction& handler_function) {
   v8::Isolate* isolate = v8::Isolate::GetCurrent();
   v8::HandleScope handle_scope(isolate);
-  v8::Local<v8::Context> v8_context = context_->v8_context();
-  v8::Context::Scope context_scope(v8_context);
+  v8::Context::Scope context_scope(context_->v8_context());
 
   v8::Local<v8::Object> data = v8::Object::New(isolate);
-  SetProperty(
-      v8_context, data, kHandlerFunction,
+  data->Set(
+      v8::String::NewFromUtf8(isolate, kHandlerFunction),
       v8::External::New(isolate, new HandlerFunction(handler_function)));
-
-  // TODO(kalman): Cache these. See https://crbug.com/478744.
   v8::Local<v8::FunctionTemplate> function_template =
       v8::FunctionTemplate::New(isolate, Router, data);
   v8::Local<v8::ObjectTemplate>::New(isolate, object_template_)
@@ -86,17 +81,16 @@ v8::Isolate* ObjectBackedNativeHandler::GetIsolate() const {
 void ObjectBackedNativeHandler::Invalidate() {
   v8::Isolate* isolate = GetIsolate();
   v8::HandleScope handle_scope(isolate);
-  v8::Local<v8::Context> v8_context = context_->v8_context();
-  v8::Context::Scope context_scope(v8_context);
+  v8::Context::Scope context_scope(context_->v8_context());
 
   for (size_t i = 0; i < router_data_.Size(); i++) {
     v8::Local<v8::Object> data = router_data_.Get(i);
     v8::Local<v8::Value> handler_function_value =
-        GetPropertyUnsafe(v8_context, data, kHandlerFunction);
+        data->Get(v8::String::NewFromUtf8(isolate, kHandlerFunction));
     CHECK(!handler_function_value.IsEmpty());
     delete static_cast<HandlerFunction*>(
         handler_function_value.As<v8::External>()->Value());
-    DeletePropertyUnsafe(v8_context, data, kHandlerFunction);
+    data->Delete(v8::String::NewFromUtf8(isolate, kHandlerFunction));
   }
 
   router_data_.Clear();
diff --git a/extensions/renderer/resources/binding.js b/extensions/renderer/resources/binding.js
index 253717c..cf671cb 100644
--- a/extensions/renderer/resources/binding.js
+++ b/extensions/renderer/resources/binding.js
@@ -353,7 +353,9 @@ Binding.prototype = {
           return;
         }
 
-        // TODO(kalman): Unit test this with a ModuleSystemTest.
+        // TODO(aa): It would be best to run this in a unit test, but in order
+        // to do that we would need to better factor this code so that it
+        // doesn't depend on so much v8::Extension machinery.
         if (logging.DCHECK_IS_ON() &&
             schemaUtils.isFunctionSignatureAmbiguous(apiFunction.definition)) {
           throw new Error(
diff --git a/extensions/renderer/safe_builtins.cc b/extensions/renderer/safe_builtins.cc
index 2103ff2..96fb155 100644
--- a/extensions/renderer/safe_builtins.cc
+++ b/extensions/renderer/safe_builtins.cc
@@ -4,17 +4,11 @@
 
 #include "extensions/renderer/safe_builtins.h"
 
-#include "base/bind.h"
-#include "base/bind_helpers.h"
-#include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
-#include "base/synchronization/lock.h"
-#include "extensions/renderer/object_backed_native_handler.h"
 #include "extensions/renderer/script_context.h"
 #include "extensions/renderer/v8_helpers.h"
-#include "third_party/WebKit/public/web/WebScopedMicrotaskSuppression.h"
 
 namespace extensions {
 
@@ -22,29 +16,24 @@ using namespace v8_helpers;
 
 namespace {
 
-// Preface:
-//
-// This is the documentation for makeCallback() function in the JavaScript, out
-// here to reduce the amount of effort that the v8 parser needs to do:
-//
-// Returns a new object with every function on |obj| configured to call()
-// itself with the given arguments.
-//
-// E.g. given
-//    var result = makeCallable(Function.prototype)
-//
-// |result| will be an object including 'bind' such that
-//    result.bind(foo, 1, 2, 3);
-//
-// is equivalent to Function.prototype.bind.call(foo, 1, 2, 3), and so on.
-// This is a convenient way to save functions that user scripts may clobber.
+const char kClassName[] = "extensions::SafeBuiltins";
 
-// This is the source of a script which evaluates to a function, which should
-// then be executed to save the bindings. This function is passed references
-// to ScriptRunner::Apply and ScriptRunner::Save via the |natives| argument.
+// Documentation for makeCallback in the JavaScript, out here to reduce the
+// (very small) amount of effort that the v8 parser needs to do:
+//
+// Returns a new object with every function on |obj| configured to call()\n"
+// itself with the given arguments.\n"
+// E.g. given\n"
+//    var result = makeCallable(Function.prototype)\n"
+// |result| will be a object including 'bind' such that\n"
+//    result.bind(foo, 1, 2, 3);\n"
+// is equivalent to Function.prototype.bind.call(foo, 1, 2, 3), and so on.\n"
+// This is a convenient way to save functions that user scripts may clobber.\n"
 const char kScript[] =
-    "(function(natives) {\n"
+    "(function() {\n"
     "'use strict';\n"
+    "native function Apply();\n"
+    "native function Save();\n"
     "\n"
     "// Used in the callback implementation, could potentially be clobbered.\n"
     "function makeCallable(obj, target, isStatic, propertyNames) {\n"
@@ -59,7 +48,7 @@ const char kScript[] =
     "        recv = arguments[0];\n"
     "        firstArgIndex = 1;\n"
     "      }\n"
-    "      return natives.Apply(\n"
+    "      return Apply(\n"
     "          property, recv, arguments, firstArgIndex, arguments.length);\n"
     "    };\n"
     "  });\n"
@@ -74,7 +63,7 @@ const char kScript[] =
     "  makeCallable(builtin.prototype, safe, false, protoPropertyNames);\n"
     "  if (staticPropertyNames)\n"
     "    makeCallable(builtin, safe, true, staticPropertyNames);\n"
-    "  natives.Save(builtin.name, safe);\n"
+    "  Save(builtin.name, safe);\n"
     "}\n"
     "\n"
     "// Save only what is needed by the extension modules.\n"
@@ -106,7 +95,7 @@ const char kScript[] =
     "});\n"
     "var builtinArray = Array;\n"
     "var builtinJSONStringify = JSON.stringify;\n"
-    "natives.Save('JSON', {\n"
+    "Save('JSON', {\n"
     "  parse: JSON.parse,\n"
     "  stringify: function(obj) {\n"
     "    var savedToJSONs = new builtinArray(builtinTypes.length);\n"
@@ -134,201 +123,127 @@ const char kScript[] =
     "  }\n"
     "});\n"
     "\n"
-    "});\n";
+    "}());\n";
 
-// Holds a compiled instance of |kScript| so that every instance of
-// SafeBuiltins doesn't need to recompile it. Thread-safe.
-// TODO(kalman): It would benefit to cache ModuleSystem's native handlers in
-// this way as well.
-class CompiledScript {
- public:
-  CompiledScript() {}
-
-  // Returns a handle to the instance of the compiled script, bound to the
-  // current context (assumed to be |context|).
-  v8::Local<v8::Script> GetForCurrentContext(v8::Local<v8::Context> context) {
-    v8::Isolate* isolate = context->GetIsolate();
-    DCHECK(v8::Isolate::GetCurrent() == isolate &&
-           isolate->GetCurrentContext() == context);
-    v8::EscapableHandleScope handle_scope(isolate);
-
-    v8::Local<v8::Script> compiled_script;
-    base::AutoLock lock_scope(lock_);
-    if (unbound_compiled_script_.IsEmpty()) {
-      compiled_script =
-          v8::Script::Compile(context, ToV8StringUnsafe(isolate, kScript))
-              .ToLocalChecked();
-      unbound_compiled_script_.Reset(isolate,
-                                     compiled_script->GetUnboundScript());
-    } else {
-      compiled_script =
-          v8::Local<v8::UnboundScript>::New(isolate, unbound_compiled_script_)
-              ->BindToCurrentContext();
-    }
-    return handle_scope.Escape(compiled_script);
-  }
-
- private:
-  // CompiledScript needs to be accessed on multiple threads - the main
-  // RenderThread, plus worker threads. Singletons are thread-safe, but
-  // access to |unbound_compiled_script_| must be locked.
-  base::Lock lock_;
-
-  // Use a v8::Persistent not a v8::Global because Globals attempt to reset the
-  // handle on destruction, and by the time CompiledScript is destroyed the
-  // renderer will be shutting down, and accessing into v8 will crash.
-  v8::Persistent<v8::UnboundScript> unbound_compiled_script_;
-
-  DISALLOW_COPY_AND_ASSIGN(CompiledScript);
-};
-
-base::LazyInstance<CompiledScript> g_compiled_script =
-    LAZY_INSTANCE_INITIALIZER;
-
-// Returns a unique key to use as a hidden value in an object without a
-// namespace collision.
 v8::Local<v8::String> MakeKey(const char* name, v8::Isolate* isolate) {
   return ToV8StringUnsafe(isolate,
-                          base::StringPrintf("safe_builtins::%s", name));
+                          base::StringPrintf("%s::%s", kClassName, name));
 }
 
-class ScriptNativeHandler : public ObjectBackedNativeHandler {
- public:
-  explicit ScriptNativeHandler(ScriptContext* context)
-      : ObjectBackedNativeHandler(context) {
-    RouteFunction("Apply", base::Bind(&ScriptNativeHandler::Apply,
-                                      base::Unretained(this)));
-    RouteFunction(
-        "Save", base::Bind(&ScriptNativeHandler::Save, base::Unretained(this)));
-  }
+void SaveImpl(const char* name,
+              v8::Local<v8::Value> value,
+              v8::Local<v8::Context> context) {
+  CHECK(!value.IsEmpty() && value->IsObject()) << name;
+  context->Global()->SetHiddenValue(MakeKey(name, context->GetIsolate()),
+                                    value);
+}
+
+v8::Local<v8::Object> Load(const char* name, v8::Local<v8::Context> context) {
+  v8::Local<v8::Value> value =
+      context->Global()->GetHiddenValue(MakeKey(name, context->GetIsolate()));
+  CHECK(!value.IsEmpty() && value->IsObject()) << name;
+  return v8::Local<v8::Object>::Cast(value);
+}
 
-  ~ScriptNativeHandler() override { Invalidate(); }
+class ExtensionImpl : public v8::Extension {
+ public:
+  ExtensionImpl() : v8::Extension(kClassName, kScript) {}
 
  private:
-  // Takes 5 arguments:
-  //  |function| The function that the arguments are being applied to.
-  //  |recv| The receiver of the function call (i.e. the "this" value).
-  //  |args| The arguments to the function call. This is actually an Arguments
-  //    object but that isn't exposed in a convenient way in the v8 API, so we
-  //    just use an Object and pass in |args_length| explicitly.
-  //  |first_arg_index| The index of the first argument within |args|.
-  //    This is 1 for prototype methods where the first argument to the
-  //    function is the receiver. It's 0 for static methods which don't have a
-  //    receiver.
-  //  |args_length| The length of the argument list. This is needed because
-  //    |args| is an Object which doesn't have a reliable concept of a length.
-  void Apply(const v8::FunctionCallbackInfo<v8::Value>& info) {
-    CHECK(info.Length() == 5 && info[0]->IsFunction() && info[2]->IsObject() &&
-          info[3]->IsInt32() && info[4]->IsInt32());
+  v8::Local<v8::FunctionTemplate> GetNativeFunctionTemplate(
+      v8::Isolate* isolate,
+      v8::Local<v8::String> name) override {
+    v8::Local<v8::Context> context = isolate->GetCurrentContext();
+    if (IsTrue(name->Equals(context, ToV8StringUnsafe(isolate, "Apply"))))
+      return v8::FunctionTemplate::New(isolate, Apply);
+    if (IsTrue(name->Equals(context, ToV8StringUnsafe(isolate, "Save"))))
+      return v8::FunctionTemplate::New(isolate, Save);
+    NOTREACHED() << *v8::String::Utf8Value(name);
+    return v8::Local<v8::FunctionTemplate>();
+  }
+
+  static void Apply(const v8::FunctionCallbackInfo<v8::Value>& info) {
+    CHECK(info.Length() == 5 && info[0]->IsFunction() &&  // function
+          // info[1] could be an object or a string
+          info[2]->IsObject() &&  // args
+          info[3]->IsInt32() &&   // first_arg_index
+          info[4]->IsInt32());    // args_length
     v8::Local<v8::Function> function = info[0].As<v8::Function>();
-    v8::Local<v8::Value> recv = info[1];
-    v8::Local<v8::Object> args = info[2].As<v8::Object>();
+    v8::Local<v8::Object> recv;
+    if (info[1]->IsObject()) {
+      recv = v8::Local<v8::Object>::Cast(info[1]);
+    } else if (info[1]->IsString()) {
+      recv = v8::StringObject::New(v8::Local<v8::String>::Cast(info[1]))
+                 .As<v8::Object>();
+    } else {
+      info.GetIsolate()->ThrowException(
+          v8::Exception::TypeError(ToV8StringUnsafe(
+              info.GetIsolate(),
+              "The first argument is the receiver and must be an object")));
+      return;
+    }
+    v8::Local<v8::Object> args = v8::Local<v8::Object>::Cast(info[2]);
     int first_arg_index = info[3].As<v8::Int32>()->Value();
     int args_length = info[4].As<v8::Int32>()->Value();
 
-    v8::Local<v8::Context> v8_context = context()->v8_context();
-
+    v8::Local<v8::Context> context = info.GetIsolate()->GetCurrentContext();
     int argc = args_length - first_arg_index;
     scoped_ptr<v8::Local<v8::Value> []> argv(new v8::Local<v8::Value>[argc]);
     for (int i = 0; i < argc; ++i) {
-      CHECK(IsTrue(args->Has(v8_context, i + first_arg_index)));
+      CHECK(IsTrue(args->Has(context, i + first_arg_index)));
       // Getting a property value could throw an exception.
-      if (!GetProperty(v8_context, args, i + first_arg_index, &argv[i]))
+      if (!GetProperty(context, args, i + first_arg_index, &argv[i]))
         return;
     }
 
     v8::Local<v8::Value> return_value;
-    if (function->Call(v8_context, recv, argc, argv.get())
-            .ToLocal(&return_value))
+    if (function->Call(context, recv, argc, argv.get()).ToLocal(&return_value))
       info.GetReturnValue().Set(return_value);
   }
 
-  void Save(const v8::FunctionCallbackInfo<v8::Value>& info) {
+  static void Save(const v8::FunctionCallbackInfo<v8::Value>& info) {
     CHECK(info.Length() == 2 && info[0]->IsString() && info[1]->IsObject());
-    v8::Local<v8::Object> object = info[1].As<v8::Object>();
-    context()->v8_context()->Global()->SetHiddenValue(
-        MakeKey(*v8::String::Utf8Value(info[0]), GetIsolate()), object);
+    SaveImpl(*v8::String::Utf8Value(info[0]),
+             info[1],
+             info.GetIsolate()->GetCallingContext());
   }
-
-  DISALLOW_COPY_AND_ASSIGN(ScriptNativeHandler);
 };
 
-void DeleteScriptHandler(scoped_ptr<ScriptNativeHandler> script_handler) {
-  // |script_handler| is a scoped_ptr so will delete itself.
-}
-
 }  // namespace
 
-SafeBuiltins::~SafeBuiltins() {}
-
 // static
-scoped_ptr<SafeBuiltins> SafeBuiltins::Install(ScriptContext* context) {
-  v8::Isolate* isolate = context->isolate();
-  v8::HandleScope handle_scope(isolate);
-  v8::Handle<v8::Context> v8_context = context->v8_context();
-  v8::Context::Scope context_scope(v8_context);
-  blink::WebScopedMicrotaskSuppression microtask_suppression;
-
-  // Run the script to return a new function bound to this context.
-  v8::Local<v8::Script> script =
-      g_compiled_script.Get().GetForCurrentContext(v8_context);
-  v8::Local<v8::Value> return_value = script->Run();
-  CHECK(return_value->IsFunction());
-  v8::Local<v8::Function> script_function = return_value.As<v8::Function>();
-
-  // Call the script function to save builtins.
-  scoped_ptr<ScriptNativeHandler> script_handler(
-      new ScriptNativeHandler(context));
-  v8::Local<v8::Value> args[] = {script_handler->NewInstance()};
-  CHECK(!script_function->Call(v8_context, v8_context->Global(),
-                               arraysize(args), args)
-             .IsEmpty());
+v8::Extension* SafeBuiltins::CreateV8Extension() { return new ExtensionImpl(); }
 
-  // Bind the lifetime of |script_handler| to |context|.
-  context->AddInvalidationObserver(
-      base::Bind(&DeleteScriptHandler, base::Passed(&script_handler)));
+SafeBuiltins::SafeBuiltins(ScriptContext* context) : context_(context) {}
 
-  // The SafeBuiltins instance itself is just a thin wrapper around accessing
-  // the hidden properties that were just installed on |context|.
-  return make_scoped_ptr(new SafeBuiltins(context));
-}
+SafeBuiltins::~SafeBuiltins() {}
 
 v8::Local<v8::Object> SafeBuiltins::GetArray() const {
-  return Load("Array");
+  return Load("Array", context_->v8_context());
 }
 
 v8::Local<v8::Object> SafeBuiltins::GetFunction() const {
-  return Load("Function");
+  return Load("Function", context_->v8_context());
 }
 
 v8::Local<v8::Object> SafeBuiltins::GetJSON() const {
-  return Load("JSON");
+  return Load("JSON", context_->v8_context());
 }
 
 v8::Local<v8::Object> SafeBuiltins::GetObjekt() const {
-  return Load("Object");
+  return Load("Object", context_->v8_context());
 }
 
 v8::Local<v8::Object> SafeBuiltins::GetRegExp() const {
-  return Load("RegExp");
+  return Load("RegExp", context_->v8_context());
 }
 
 v8::Local<v8::Object> SafeBuiltins::GetString() const {
-  return Load("String");
+  return Load("String", context_->v8_context());
 }
 
 v8::Local<v8::Object> SafeBuiltins::GetError() const {
-  return Load("Error");
-}
-
-SafeBuiltins::SafeBuiltins(ScriptContext* context) : context_(context) {}
-
-v8::Local<v8::Object> SafeBuiltins::Load(const char* name) const {
-  v8::Local<v8::Value> value = context_->v8_context()->Global()->GetHiddenValue(
-      MakeKey(name, context_->isolate()));
-  CHECK(!IsEmptyOrUndefined(value));
-  CHECK(value->IsObject()) << name;
-  return v8::Local<v8::Object>::Cast(value);
+  return Load("Error", context_->v8_context());
 }
 
 }  //  namespace extensions
diff --git a/extensions/renderer/safe_builtins.h b/extensions/renderer/safe_builtins.h
index 0da56bb..83aaca9 100644
--- a/extensions/renderer/safe_builtins.h
+++ b/extensions/renderer/safe_builtins.h
@@ -5,21 +5,21 @@
 #ifndef EXTENSIONS_RENDERER_SAFE_BUILTINS_H_
 #define EXTENSIONS_RENDERER_SAFE_BUILTINS_H_
 
-#include "base/macros.h"
-#include "base/memory/scoped_ptr.h"
 #include "v8/include/v8.h"
 
 namespace extensions {
 class ScriptContext;
 
-// Saves a subset of the JavaScript builtin types, so that they can be used
-// later without extensions tampering with them.
+// A collection of safe builtin objects, in that they won't be tained by
+// extensions overriding methods on them.
 class SafeBuiltins {
  public:
-  ~SafeBuiltins();
+  // Creates the v8::Extension which manages SafeBuiltins instances.
+  static v8::Extension* CreateV8Extension();
 
-  // Creates and immediately installs a SafeBuiltins instance in |context|.
-  static scoped_ptr<SafeBuiltins> Install(ScriptContext* context);
+  explicit SafeBuiltins(ScriptContext* context);
+
+  virtual ~SafeBuiltins();
 
   // Each method returns an object with methods taken from their respective
   // builtin object's prototype, adapted to automatically call() themselves.
@@ -39,13 +39,7 @@ class SafeBuiltins {
   v8::Local<v8::Object> GetError() const;
 
  private:
-  explicit SafeBuiltins(ScriptContext* context);
-
-  v8::Local<v8::Object> Load(const char* name) const;
-
   ScriptContext* context_;
-
-  DISALLOW_COPY_AND_ASSIGN(SafeBuiltins);
 };
 
 }  //  namespace extensions
diff --git a/extensions/renderer/script_context.cc b/extensions/renderer/script_context.cc
index f747b25..9321468 100644
--- a/extensions/renderer/script_context.cc
+++ b/extensions/renderer/script_context.cc
@@ -21,7 +21,6 @@
 #include "extensions/common/features/base_feature_provider.h"
 #include "extensions/common/manifest_handlers/sandboxed_page_info.h"
 #include "extensions/common/permissions/permissions_data.h"
-#include "extensions/renderer/safe_builtins.h"
 #include "gin/per_context_data.h"
 #include "third_party/WebKit/public/web/WebDataSource.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
@@ -101,6 +100,7 @@ ScriptContext::ScriptContext(const v8::Local<v8::Context>& v8_context,
       context_type_(context_type),
       effective_extension_(effective_extension),
       effective_context_type_(effective_context_type),
+      safe_builtins_(this),
       isolate_(v8_context->GetIsolate()),
       url_(web_frame_ ? GetDataSourceURLForFrame(web_frame_) : GURL()),
       runner_(new Runner(this)) {
@@ -108,7 +108,6 @@ ScriptContext::ScriptContext(const v8::Local<v8::Context>& v8_context,
   gin::PerContextData* gin_data = gin::PerContextData::From(v8_context);
   CHECK(gin_data);  // may fail if the v8::Context hasn't been registered yet
   gin_data->set_runner(runner_.get());
-  safe_builtins_ = SafeBuiltins::Install(this);
 }
 
 ScriptContext::~ScriptContext() {
diff --git a/extensions/renderer/script_context.h b/extensions/renderer/script_context.h
index 4d7b496..4a9531a 100644
--- a/extensions/renderer/script_context.h
+++ b/extensions/renderer/script_context.h
@@ -15,6 +15,7 @@
 #include "extensions/common/permissions/api_permission_set.h"
 #include "extensions/renderer/module_system.h"
 #include "extensions/renderer/request_sender.h"
+#include "extensions/renderer/safe_builtins.h"
 #include "gin/runner.h"
 #include "url/gurl.h"
 #include "v8/include/v8.h"
@@ -31,7 +32,6 @@ class RenderFrame;
 namespace extensions {
 class Extension;
 class ExtensionSet;
-class SafeBuiltins;
 
 // Extensions wrapper for a v8 context.
 class ScriptContext : public RequestSender::Source {
@@ -87,9 +87,9 @@ class ScriptContext : public RequestSender::Source {
 
   ModuleSystem* module_system() { return module_system_.get(); }
 
-  SafeBuiltins* safe_builtins() { return safe_builtins_.get(); }
+  SafeBuiltins* safe_builtins() { return &safe_builtins_; }
 
-  const SafeBuiltins* safe_builtins() const { return safe_builtins_.get(); }
+  const SafeBuiltins* safe_builtins() const { return &safe_builtins_; }
 
   // Returns the ID of the extension associated with this context, or empty
   // string if there is no such extension.
@@ -213,7 +213,7 @@ class ScriptContext : public RequestSender::Source {
   scoped_ptr<ModuleSystem> module_system_;
 
   // Contains safe copies of builtin objects like Function.prototype.
-  scoped_ptr<SafeBuiltins> safe_builtins_;
+  SafeBuiltins safe_builtins_;
 
   // The set of capabilities granted to this context by extensions.
   APIPermissionSet content_capabilities_;
diff --git a/extensions/renderer/utils_native_handler.cc b/extensions/renderer/utils_native_handler.cc
index a9ab97f..49ff32a 100644
--- a/extensions/renderer/utils_native_handler.cc
+++ b/extensions/renderer/utils_native_handler.cc
@@ -5,7 +5,6 @@
 #include "extensions/renderer/utils_native_handler.h"
 
 #include "base/strings/stringprintf.h"
-#include "extensions/renderer/safe_builtins.h"
 #include "extensions/renderer/script_context.h"
 #include "third_party/WebKit/public/web/WebScopedMicrotaskSuppression.h"
 #include "third_party/WebKit/public/web/WebSerializedScriptValue.h"
diff --git a/extensions/renderer/v8_helpers.h b/extensions/renderer/v8_helpers.h
index e9ac61c..2bfeee8 100644
--- a/extensions/renderer/v8_helpers.h
+++ b/extensions/renderer/v8_helpers.h
@@ -52,7 +52,7 @@ inline bool IsTrue(v8::Maybe<bool> maybe) {
 }
 
 // Returns true if |value| is empty or undefined.
-inline bool IsEmptyOrUndefined(v8::Local<v8::Value> value) {
+inline bool IsEmptyOrUndefied(v8::Local<v8::Value> value) {
   return value.IsEmpty() || value->IsUndefined();
 }
 
@@ -119,24 +119,6 @@ inline v8::Local<v8::Value> GetPropertyUnsafe(
       .ToLocalChecked();
 }
 
-// DeletePropertyUnsafe() family wraps v8::Object::Delete(). They crash when an
-// exception is thrown.
-inline bool DeletePropertyUnsafe(v8::Local<v8::Context> context,
-                                 v8::Local<v8::Object> object,
-                                 v8::Local<v8::Value> key) {
-  return object->Delete(context, key).FromJust();
-}
-
-inline bool DeletePropertyUnsafe(
-    v8::Local<v8::Context> context,
-    v8::Local<v8::Object> object,
-    const char* key,
-    v8::NewStringType string_type = v8::NewStringType::kNormal) {
-  return object->Delete(context, ToV8StringUnsafe(context->GetIsolate(), key,
-                                                  string_type))
-      .FromJust();
-}
-
 // Wraps v8::Function::Call(). Returns true on success.
 inline bool CallFunction(v8::Local<v8::Context> context,
                          v8::Local<v8::Function> function,