Add a ScopedPersistent class to chrome/renderer/extensions for automatically

New'ing and Dispose'ing v8::Persistent handles.

TBR=ben@chromium.org

Review URL: https://chromiumcodereview.appspot.com/12705003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@187125 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 109 insertions, 60 deletions

diff --git a/chrome/chrome_renderer.gypi b/chrome/chrome_renderer.gypi
index a9cf77a..dc3c274 100644
--- a/chrome/chrome_renderer.gypi
+++ b/chrome/chrome_renderer.gypi
@@ -112,6 +112,7 @@
         'renderer/extensions/resource_request_policy.h',
         'renderer/extensions/runtime_custom_bindings.cc',
         'renderer/extensions/runtime_custom_bindings.h',
+        'renderer/extensions/scoped_persistent.h',
         'renderer/extensions/send_request_natives.cc',
         'renderer/extensions/send_request_natives.h',
         'renderer/extensions/set_icon_natives.cc',
diff --git a/chrome/renderer/extensions/app_window_custom_bindings.cc b/chrome/renderer/extensions/app_window_custom_bindings.cc
index 29325d5..a66bb55 100644
--- a/chrome/renderer/extensions/app_window_custom_bindings.cc
+++ b/chrome/renderer/extensions/app_window_custom_bindings.cc
@@ -9,6 +9,7 @@
 #include "chrome/common/extensions/extension_messages.h"
 #include "chrome/renderer/extensions/chrome_v8_context.h"
 #include "chrome/renderer/extensions/dispatcher.h"
+#include "chrome/renderer/extensions/scoped_persistent.h"
 #include "content/public/renderer/render_thread.h"
 #include "content/public/renderer/render_view.h"
 #include "content/public/renderer/render_view_observer.h"
@@ -66,8 +67,7 @@ class LoadWatcher : public content::RenderViewObserver {
               content::RenderView* view,
               v8::Handle<v8::Function> cb)
       : content::RenderViewObserver(view),
-        isolate_(isolate),
-        callback_(v8::Persistent<v8::Function>::New(isolate, cb)) {
+        callback_(cb) {
   }
 
   virtual void DidCreateDocumentElement(WebKit::WebFrame* frame) OVERRIDE {
@@ -80,13 +80,8 @@ class LoadWatcher : public content::RenderViewObserver {
     CallbackAndDie(frame, false);
   }
 
-  virtual ~LoadWatcher() {
-    callback_.Dispose(isolate_);
-  }
-
  private:
-  v8::Isolate* isolate_;
-  v8::Persistent<v8::Function> callback_;
+  ScopedPersistent<v8::Function> callback_;
 
   void CallbackAndDie(WebKit::WebFrame* frame, bool succeeded) {
     v8::HandleScope handle_scope;
diff --git a/chrome/renderer/extensions/chrome_v8_context.cc b/chrome/renderer/extensions/chrome_v8_context.cc
index 2abc772..b683507 100644
--- a/chrome/renderer/extensions/chrome_v8_context.cc
+++ b/chrome/renderer/extensions/chrome_v8_context.cc
@@ -33,8 +33,7 @@ ChromeV8Context::ChromeV8Context(v8::Handle<v8::Context> v8_context,
                                  WebKit::WebFrame* web_frame,
                                  const Extension* extension,
                                  Feature::Context context_type)
-    : v8_context_(v8::Persistent<v8::Context>::New(v8_context->GetIsolate(),
-                                                   v8_context)),
+    : v8_context_(v8_context),
       web_frame_(web_frame),
       extension_(extension),
       context_type_(context_type) {
@@ -47,7 +46,6 @@ ChromeV8Context::ChromeV8Context(v8::Handle<v8::Context> v8_context,
 ChromeV8Context::~ChromeV8Context() {
   VLOG(1) << "Destroyed context for extension\n"
           << "  extension id: " << GetExtensionID();
-  v8_context_.Dispose(v8_context_->GetIsolate());
 }
 
 std::string ChromeV8Context::GetExtensionID() {
@@ -97,7 +95,7 @@ bool ChromeV8Context::CallChromeHiddenMethod(
     int argc,
     v8::Handle<v8::Value>* argv,
     v8::Handle<v8::Value>* result) const {
-  v8::Context::Scope context_scope(v8_context_);
+  v8::Context::Scope context_scope(v8_context_.get());
 
   // ChromeV8ContextSet calls clear_web_frame() and then schedules a task to
   // delete this object. This check prevents a race from attempting to execute
diff --git a/chrome/renderer/extensions/chrome_v8_context.h b/chrome/renderer/extensions/chrome_v8_context.h
index 8c71c04..d541e2c 100644
--- a/chrome/renderer/extensions/chrome_v8_context.h
+++ b/chrome/renderer/extensions/chrome_v8_context.h
@@ -10,6 +10,7 @@
 #include "base/basictypes.h"
 #include "chrome/common/extensions/features/feature.h"
 #include "chrome/renderer/extensions/module_system.h"
+#include "chrome/renderer/extensions/scoped_persistent.h"
 #include "v8/include/v8.h"
 
 namespace WebKit {
@@ -37,7 +38,7 @@ class ChromeV8Context {
   ~ChromeV8Context();
 
   v8::Handle<v8::Context> v8_context() const {
-    return v8_context_;
+    return v8_context_.get();
   }
 
   const Extension* extension() const {
@@ -104,14 +105,8 @@ class ChromeV8Context {
   std::string GetContextTypeDescription();
 
  private:
-  // The v8 context the bindings are accessible to. We keep a strong reference
-  // to it for simplicity. In the case of content scripts, this is necessary
-  // because we want all scripts from the same extension for the same frame to
-  // run in the same context, so we can't have the contexts being GC'd if
-  // nothing is happening. In the case of page contexts, this isn't necessary
-  // since the DOM keeps the context alive, but it makes things simpler to not
-  // distinguish the two cases.
-  v8::Persistent<v8::Context> v8_context_;
+  // The v8 context the bindings are accessible to.
+  ScopedPersistent<v8::Context> v8_context_;
 
   // The WebFrame associated with this context. This can be NULL because this
   // object can outlive is destroyed asynchronously.
diff --git a/chrome/renderer/extensions/miscellaneous_bindings.cc b/chrome/renderer/extensions/miscellaneous_bindings.cc
index 3830375..da4c58d 100644
--- a/chrome/renderer/extensions/miscellaneous_bindings.cc
+++ b/chrome/renderer/extensions/miscellaneous_bindings.cc
@@ -17,6 +17,7 @@
 #include "chrome/renderer/extensions/chrome_v8_extension.h"
 #include "chrome/renderer/extensions/dispatcher.h"
 #include "chrome/renderer/extensions/event_bindings.h"
+#include "chrome/renderer/extensions/scoped_persistent.h"
 #include "content/public/renderer/render_thread.h"
 #include "content/public/renderer/render_view.h"
 #include "grit/renderer_resources.h"
@@ -141,8 +142,15 @@ class ExtensionImpl : public extensions::ChromeV8Extension {
   }
 
   struct GCCallbackArgs {
-    v8::Persistent<v8::Object> object;
-    v8::Persistent<v8::Function> callback;
+    GCCallbackArgs(v8::Handle<v8::Object> object,
+                   v8::Handle<v8::Function> callback)
+        : object(object), callback(callback) {}
+
+    extensions::ScopedPersistent<v8::Object> object;
+    extensions::ScopedPersistent<v8::Function> callback;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(GCCallbackArgs);
   };
 
   static void GCCallback(v8::Isolate* isolate,
@@ -152,23 +160,16 @@ class ExtensionImpl : public extensions::ChromeV8Extension {
     GCCallbackArgs* args = reinterpret_cast<GCCallbackArgs*>(parameter);
     WebKit::WebScopedMicrotaskSuppression suppression;
     args->callback->Call(args->callback->CreationContext()->Global(), 0, NULL);
-    args->callback.Dispose(isolate);
-    args->object.Dispose(isolate);
     delete args;
   }
 
   // Binds a callback to be invoked when the given object is garbage collected.
   static v8::Handle<v8::Value> BindToGC(const v8::Arguments& args) {
     if (args.Length() == 2 && args[0]->IsObject() && args[1]->IsFunction()) {
-      v8::Isolate* isolate = args.GetIsolate();
-      GCCallbackArgs* context = new GCCallbackArgs;
-      context->callback = v8::Persistent<v8::Function>::New(
-          isolate,
+      GCCallbackArgs* context = new GCCallbackArgs(
+          v8::Handle<v8::Object>::Cast(args[0]),
           v8::Handle<v8::Function>::Cast(args[1]));
-      context->object = v8::Persistent<v8::Object>::New(
-          isolate,
-          v8::Handle<v8::Object>::Cast(args[0]));
-      context->object.MakeWeak(isolate, context, GCCallback);
+      context->object.MakeWeak(context, GCCallback);
     } else {
       NOTREACHED();
     }
diff --git a/chrome/renderer/extensions/module_system.cc b/chrome/renderer/extensions/module_system.cc
index 31146c8..24982cc 100644
--- a/chrome/renderer/extensions/module_system.cc
+++ b/chrome/renderer/extensions/module_system.cc
@@ -22,8 +22,7 @@ namespace extensions {
 ModuleSystem::ModuleSystem(v8::Handle<v8::Context> context,
                            SourceMap* source_map)
     : NativeHandler(context->GetIsolate()),
-      context_(v8::Persistent<v8::Context>::New(context->GetIsolate(),
-                                                context)),
+      context_(context),
       source_map_(source_map),
       natives_enabled_(0) {
   RouteFunction("require",
@@ -42,7 +41,6 @@ ModuleSystem::~ModuleSystem() {
   // Deleting this value here prevents future lazy field accesses from
   // referencing ModuleSystem after it has been freed.
   context_->Global()->DeleteHiddenValue(v8::String::New(kModuleSystem));
-  context_.Dispose(context_->GetIsolate());
 }
 
 ModuleSystem::NativesEnabledScope::NativesEnabledScope(
diff --git a/chrome/renderer/extensions/module_system.h b/chrome/renderer/extensions/module_system.h
index be03f2a..605a1c7 100644
--- a/chrome/renderer/extensions/module_system.h
+++ b/chrome/renderer/extensions/module_system.h
@@ -9,6 +9,7 @@
 #include "base/memory/linked_ptr.h"
 #include "base/memory/scoped_ptr.h"
 #include "chrome/renderer/extensions/native_handler.h"
+#include "chrome/renderer/extensions/scoped_persistent.h"
 #include "v8/include/v8.h"
 
 #include <map>
@@ -148,7 +149,7 @@ class ModuleSystem : public NativeHandler {
   v8::Handle<v8::Value> ThrowException(const std::string& message);
 
   // The context that this ModuleSystem is for.
-  v8::Persistent<v8::Context> context_;
+  ScopedPersistent<v8::Context> context_;
 
   // A map from module names to the JS source for that module. GetSource()
   // performs a lookup on this map.
diff --git a/chrome/renderer/extensions/native_handler.cc b/chrome/renderer/extensions/native_handler.cc
index 3c41ea5..ee03ad0 100644
--- a/chrome/renderer/extensions/native_handler.cc
+++ b/chrome/renderer/extensions/native_handler.cc
@@ -12,15 +12,9 @@
 namespace extensions {
 
 NativeHandler::NativeHandler(v8::Isolate* isolate)
-    : isolate_(isolate),
-      object_template_(
-          v8::Persistent<v8::ObjectTemplate>::New(isolate,
-                                                  v8::ObjectTemplate::New())) {
-}
+    : object_template_(v8::ObjectTemplate::New()) {}
 
-NativeHandler::~NativeHandler() {
-  object_template_.Dispose(isolate_);
-}
+NativeHandler::~NativeHandler() {}
 
 v8::Handle<v8::Object> NativeHandler::NewInstance() {
   return object_template_->NewInstance();
diff --git a/chrome/renderer/extensions/native_handler.h b/chrome/renderer/extensions/native_handler.h
index fd6bda6..dbe6a63 100644
--- a/chrome/renderer/extensions/native_handler.h
+++ b/chrome/renderer/extensions/native_handler.h
@@ -7,6 +7,7 @@
 
 #include "base/bind.h"
 #include "base/memory/linked_ptr.h"
+#include "chrome/renderer/extensions/scoped_persistent.h"
 #include "v8/include/v8.h"
 
 #include <string>
@@ -50,8 +51,7 @@ class NativeHandler {
   static v8::Handle<v8::Value> Router(const v8::Arguments& args);
 
   std::vector<linked_ptr<HandlerFunction> > handler_functions_;
-  v8::Isolate* isolate_;
-  v8::Persistent<v8::ObjectTemplate> object_template_;
+  ScopedPersistent<v8::ObjectTemplate> object_template_;
 
   DISALLOW_COPY_AND_ASSIGN(NativeHandler);
 };
diff --git a/chrome/renderer/extensions/request_sender.cc b/chrome/renderer/extensions/request_sender.cc
index 2120d0b7..0c54e17 100644
--- a/chrome/renderer/extensions/request_sender.cc
+++ b/chrome/renderer/extensions/request_sender.cc
@@ -9,6 +9,7 @@
 #include "chrome/renderer/extensions/chrome_v8_context.h"
 #include "chrome/renderer/extensions/chrome_v8_context_set.h"
 #include "chrome/renderer/extensions/dispatcher.h"
+#include "chrome/renderer/extensions/scoped_persistent.h"
 #include "content/public/renderer/render_view.h"
 #include "content/public/renderer/v8_value_converter.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebDocument.h"
@@ -23,16 +24,13 @@ namespace extensions {
 // Contains info relevant to a pending API request.
 struct PendingRequest {
  public :
-  PendingRequest(v8::Persistent<v8::Context> context, const std::string& name,
+  PendingRequest(v8::Handle<v8::Context> context,
+                 const std::string& name,
                  const std::string& extension_id)
       : context(context), name(name), extension_id(extension_id) {
   }
 
-  ~PendingRequest() {
-    context.Dispose(context->GetIsolate());
-  }
-
-  v8::Persistent<v8::Context> context;
+  ScopedPersistent<v8::Context> context;
   std::string name;
   std::string extension_id;
 };
@@ -94,14 +92,9 @@ void RequestSender::StartRequest(const std::string& name,
     source_origin = webframe->document().securityOrigin();
   }
 
-  v8::Local<v8::Context> ctx = v8::Context::GetCurrent();
-  v8::Persistent<v8::Context> v8_context =
-      v8::Persistent<v8::Context>::New(ctx->GetIsolate(), ctx);
-  DCHECK(!v8_context.IsEmpty());
-
   std::string extension_id = current_context->GetExtensionID();
   InsertRequest(request_id, new PendingRequest(
-      v8_context, name, extension_id));
+      v8::Context::GetCurrent(), name, extension_id));
 
   ExtensionHostMsg_Request_Params params;
   params.name = name;
@@ -135,7 +128,8 @@ void RequestSender::HandleResponse(int request_id,
     return;
   }
 
-  ChromeV8Context* v8_context = context_set_->GetByV8Context(request->context);
+  ChromeV8Context* v8_context =
+      context_set_->GetByV8Context(request->context.get());
   if (!v8_context)
     return;  // The frame went away.
 
diff --git a/chrome/renderer/extensions/scoped_persistent.h b/chrome/renderer/extensions/scoped_persistent.h
new file mode 100644
index 0000000..d1c7088
--- /dev/null
+++ b/chrome/renderer/extensions/scoped_persistent.h
@@ -0,0 +1,72 @@
+// Copyright 2013 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.
+
+#ifndef CHROME_RENDERER_EXTENSIONS_SCOPED_PERSISTENT_H_
+#define CHROME_RENDERER_EXTENSIONS_SCOPED_PERSISTENT_H_
+
+#include "base/logging.h"
+#include "v8/include/v8.h"
+
+namespace extensions {
+
+// A v8::Persistent handle to a V8 value which destroys and clears the
+// underlying handle on destruction.
+template <typename T>
+class ScopedPersistent {
+ public:
+  explicit ScopedPersistent(v8::Handle<T> handle) {
+    reset(handle);
+  }
+
+  ~ScopedPersistent() {
+    reset();
+  }
+
+  void reset(v8::Handle<T> handle) {
+    reset();
+    handle_ = v8::Persistent<T>::New(GetIsolate(handle), handle);
+  }
+
+  void reset() {
+    if (handle_.IsEmpty())
+      return;
+    handle_.Dispose(GetIsolate(handle_));
+    handle_.Clear();
+  }
+
+  v8::Persistent<T> operator->() const {
+    return handle_;
+  }
+
+  v8::Persistent<T> get() const {
+    return handle_;
+  }
+
+  void MakeWeak(void* parameters, v8::NearDeathCallback callback) {
+    handle_.MakeWeak(GetIsolate(handle_), parameters, callback);
+  }
+
+ private:
+  template <typename U>
+  static v8::Isolate* GetIsolate(v8::Handle<U> object_handle) {
+    // Only works for v8::Object and its subclasses. Add specialisations for
+    // anything else.
+    return GetIsolate(object_handle->CreationContext());
+  }
+  static v8::Isolate* GetIsolate(v8::Handle<v8::Context> context_handle) {
+    return context_handle->GetIsolate();
+  }
+  static v8::Isolate* GetIsolate(
+      v8::Handle<v8::ObjectTemplate> template_handle) {
+    return v8::Isolate::GetCurrent();
+  }
+
+  v8::Persistent<T> handle_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedPersistent);
+};
+
+}  // namespace extensions
+
+#endif  // CHROME_RENDERER_EXTENSIONS_SCOPED_PERSISTENT_H_