Fix memory leak in V8ContextSet unit test.

BUG=99301

Review URL: http://codereview.chromium.org/8175012

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

8 files changed, 31 insertions, 63 deletions

diff --git a/chrome/renderer/extensions/chrome_v8_context.cc b/chrome/renderer/extensions/chrome_v8_context.cc
index c4c4e5a..d3cc655 100644
--- a/chrome/renderer/extensions/chrome_v8_context.cc
+++ b/chrome/renderer/extensions/chrome_v8_context.cc
@@ -14,16 +14,6 @@
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebView.h"
 #include "v8/include/v8.h"
 
-void ChromeV8Context::FireOnLoadEvent(bool is_extension_process,
-                                      bool is_incognito_process) const {
-  v8::HandleScope handle_scope;
-  v8::Handle<v8::Value> argv[3];
-  argv[0] = v8::String::New(extension_id_.c_str());
-  argv[1] = v8::Boolean::New(is_extension_process);
-  argv[2] = v8::Boolean::New(is_incognito_process);
-  CallChromeHiddenMethod("dispatchOnLoad", arraysize(argv), argv);
-}
-
 ChromeV8Context::ChromeV8Context(v8::Handle<v8::Context> v8_context,
                                  WebKit::WebFrame* web_frame,
                                  const std::string& extension_id)
@@ -38,8 +28,6 @@ ChromeV8Context::ChromeV8Context(v8::Handle<v8::Context> v8_context,
 ChromeV8Context::~ChromeV8Context() {
   VLOG(1) << "Destroyed context for extension\n"
           << "  id:    " << extension_id_;
-  v8::HandleScope handle_scope;
-  CallChromeHiddenMethod("dispatchOnUnload", 0, NULL);
   v8_context_.Dispose();
 }
 
@@ -72,3 +60,18 @@ v8::Handle<v8::Value> ChromeV8Context::CallChromeHiddenMethod(
   return v8::Local<v8::Function>::Cast(value)->Call(
       v8::Object::New(), argc, argv);
 }
+
+void ChromeV8Context::DispatchOnLoadEvent(bool is_extension_process,
+                                          bool is_incognito_process) const {
+  v8::HandleScope handle_scope;
+  v8::Handle<v8::Value> argv[3];
+  argv[0] = v8::String::New(extension_id_.c_str());
+  argv[1] = v8::Boolean::New(is_extension_process);
+  argv[2] = v8::Boolean::New(is_incognito_process);
+  CallChromeHiddenMethod("dispatchOnLoad", arraysize(argv), argv);
+}
+
+void ChromeV8Context::DispatchOnUnloadEvent() const {
+  v8::HandleScope handle_scope;
+  CallChromeHiddenMethod("dispatchOnUnload", 0, NULL);
+}
diff --git a/chrome/renderer/extensions/chrome_v8_context.h b/chrome/renderer/extensions/chrome_v8_context.h
index b75c74e..f55fc54 100644
--- a/chrome/renderer/extensions/chrome_v8_context.h
+++ b/chrome/renderer/extensions/chrome_v8_context.h
@@ -48,9 +48,11 @@ class ChromeV8Context {
   // context is in the process of being destroyed.
   RenderView* GetRenderView() const;
 
-  // Fires the onload event on the chromeHidden object.
-  void FireOnLoadEvent(bool is_extension_process,
-                       bool is_incognito_process) const;
+  // Fires the onload and onunload events on the chromeHidden object.
+  // TODO(aa): Does these make more sense with EventBindings?
+  void DispatchOnLoadEvent(bool is_extension_process,
+                           bool is_incognito_process) const;
+  void DispatchOnUnloadEvent() const;
 
   // Call the named method of the chromeHidden object in this context.
   // The function can be a sub-property like "Port.dispatchOnMessage". Returns
diff --git a/chrome/renderer/extensions/chrome_v8_context_set.cc b/chrome/renderer/extensions/chrome_v8_context_set.cc
index a4f4bd6..a670704 100644
--- a/chrome/renderer/extensions/chrome_v8_context_set.cc
+++ b/chrome/renderer/extensions/chrome_v8_context_set.cc
@@ -75,13 +75,6 @@ void ChromeV8ContextSet::Remove(ChromeV8Context* context) {
   }
 }
 
-void ChromeV8ContextSet::RemoveByV8Context(
-    v8::Handle<v8::Context> v8_context) {
-  ChromeV8Context* context = GetByV8Context(v8_context);
-  if (context)
-    Remove(context);
-}
-
 ChromeV8ContextSet::ContextSet ChromeV8ContextSet::GetAll()
     const {
   return contexts_;
diff --git a/chrome/renderer/extensions/chrome_v8_context_set.h b/chrome/renderer/extensions/chrome_v8_context_set.h
index 4d27a3d..37293c6 100644
--- a/chrome/renderer/extensions/chrome_v8_context_set.h
+++ b/chrome/renderer/extensions/chrome_v8_context_set.h
@@ -45,7 +45,6 @@ class ChromeV8ContextSet {
   // it asynchronously. After this call returns the context object will still
   // be valid, but its frame() pointer will be cleared.
   void Remove(ChromeV8Context* context);
-  void RemoveByV8Context(v8::Handle<v8::Context> context);
 
   // Returns a copy to protect against changes.
   typedef std::set<ChromeV8Context*> ContextSet;
diff --git a/chrome/renderer/extensions/chrome_v8_context_set_unittest.cc b/chrome/renderer/extensions/chrome_v8_context_set_unittest.cc
index b7305df..54a2400 100644
--- a/chrome/renderer/extensions/chrome_v8_context_set_unittest.cc
+++ b/chrome/renderer/extensions/chrome_v8_context_set_unittest.cc
@@ -43,24 +43,7 @@ TEST(ChromeV8ContextSet, Lifecycle) {
 
   // After removal, the context should be marked for destruction.
   EXPECT_FALSE(context->web_frame());
-}
-
-TEST(ChromeV8ContextSet, RemoveByV8Context) {
-  MessageLoop loop;
-
-  ChromeV8ContextSet context_set;
-
-  v8::HandleScope handle_scope;
-  v8::Handle<v8::Context> v8_context(v8::Context::New());
 
-  WebKit::WebFrame* frame = reinterpret_cast<WebKit::WebFrame*>(1);
-  std::string extension_id = "00000000000000000000000000000000";
-  ChromeV8Context* context =
-      new ChromeV8Context(v8_context, frame, extension_id);
-
-  context_set.Add(context);
-  EXPECT_EQ(1, context_set.size());
-  context_set.RemoveByV8Context(context->v8_context());
-  EXPECT_EQ(0, context_set.size());
-  EXPECT_FALSE(context->web_frame());
+  // Run loop to do the actual deletion.
+  loop.RunAllPending();
 }
diff --git a/chrome/renderer/extensions/extension_dispatcher.cc b/chrome/renderer/extensions/extension_dispatcher.cc
index 792496c..f35c4de 100644
--- a/chrome/renderer/extensions/extension_dispatcher.cc
+++ b/chrome/renderer/extensions/extension_dispatcher.cc
@@ -246,14 +246,20 @@ void ExtensionDispatcher::DidCreateScriptContext(
   ChromeV8Context* context =
       new ChromeV8Context(v8_context, frame, extension_id);
   v8_context_set_.Add(context);
-  context->FireOnLoadEvent(is_extension_process_,
-                           ChromeRenderProcessObserver::is_incognito_process());
+  context->DispatchOnLoadEvent(
+      is_extension_process_,
+      ChromeRenderProcessObserver::is_incognito_process());
   VLOG(1) << "Num tracked contexts: " << v8_context_set_.size();
 }
 
 void ExtensionDispatcher::WillReleaseScriptContext(
     WebFrame* frame, v8::Handle<v8::Context> v8_context, int world_id) {
-  v8_context_set_.RemoveByV8Context(v8_context);
+  ChromeV8Context* context = v8_context_set_.GetByV8Context(v8_context);
+  if (!context)
+    return;
+
+  context->DispatchOnUnloadEvent();
+  v8_context_set_.Remove(context);
   VLOG(1) << "Num tracked contexts: " << v8_context_set_.size();
 }
 
diff --git a/tools/heapcheck/suppressions.txt b/tools/heapcheck/suppressions.txt
index 76d4d1e..40d2e44 100644
--- a/tools/heapcheck/suppressions.txt
+++ b/tools/heapcheck/suppressions.txt
@@ -1661,12 +1661,6 @@
    fun:AppNotificationManager::Init
 }
 {
-   bug_99301
-   Heapcheck:Leak
-   ...
-   fun:ChromeV8ContextSet_*_Test::TestBody
-}
-{
    bug_99304
    Heapcheck:Leak
    fun:v8::internal::SkipList::Update
diff --git a/tools/valgrind/memcheck/suppressions.txt b/tools/valgrind/memcheck/suppressions.txt
index 0cbcdeb..13c1784 100644
--- a/tools/valgrind/memcheck/suppressions.txt
+++ b/tools/valgrind/memcheck/suppressions.txt
@@ -4654,18 +4654,6 @@
    fun:_ZN22AppNotificationStorage6CreateERK8FilePath
 }
 {
-   bug_99301_a
-   Memcheck:Leak
-   fun:_Znw*
-   fun:_ZN*ChromeV8ContextSet_*_Test8TestBodyEv
-}
-{
-   bug_99301_b
-   Memcheck:Leak
-   fun:_Znw*
-   fun:_ZN*ExtensionBindingsContextSet_*_Test8TestBodyEv
-}
-{
    bug_99307
    Memcheck:Value4
    fun:modp_b64_encode