Added a test for destroying an isolate while a timer is outstanding.

Cleaned up a few of the existing tests.

BUG=
R=abarth@chromium.org

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

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

2 files changed, 86 insertions, 47 deletions

diff --git a/gin/modules/timer.cc b/gin/modules/timer.cc
index ebe75bb..6baf2d2 100644
--- a/gin/modules/timer.cc
+++ b/gin/modules/timer.cc
@@ -18,7 +18,7 @@ v8::Handle<v8::String> GetHiddenPropertyName(v8::Isolate* isolate) {
 
 }  // namespace
 
-// Timer
+// Timer =======================================================================
 
 gin::WrapperInfo Timer::kWrapperInfo = { gin::kEmbedderNativeGin };
 
@@ -30,6 +30,8 @@ Handle<Timer> Timer::Create(TimerType type, v8::Isolate* isolate, int delay_ms,
 }
 
 ObjectTemplateBuilder Timer::GetObjectTemplateBuilder(v8::Isolate* isolate) {
+  // We use Unretained() here because we directly own timer_, so we know it will
+  // be alive when these methods are called.
   return Wrappable<Timer>::GetObjectTemplateBuilder(isolate)
       .SetMethod("cancel",
                  base::Bind(&base::Timer::Stop, base::Unretained(&timer_)))
@@ -53,8 +55,13 @@ Timer::~Timer() {
 }
 
 void Timer::OnTimerFired() {
-  if (!runner_)
+  // This can happen in spite of the weak callback because it is possible for
+  // a gin::Handle<> to keep this object alive past when the isolate it is part
+  // of is destroyed.
+  if (!runner_.get()) {
     return;
+  }
+
   Runner::Scope scope(runner_.get());
   v8::Handle<v8::Function> function = v8::Handle<v8::Function>::Cast(
       GetWrapper(runner_->isolate())->GetHiddenValue(
@@ -63,7 +70,7 @@ void Timer::OnTimerFired() {
 }
 
 
-// TimerModule
+// TimerModule =================================================================
 
 WrapperInfo TimerModule::kWrapperInfo = { kEmbedderNativeGin };
 
diff --git a/gin/modules/timer_unittest.cc b/gin/modules/timer_unittest.cc
index 96972d5..aaffbde 100644
--- a/gin/modules/timer_unittest.cc
+++ b/gin/modules/timer_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "gin/modules/timer.h"
 
+#include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "gin/handle.h"
 #include "gin/object_template_builder.h"
@@ -28,8 +29,8 @@ class Result : public Wrappable<Result> {
   int count() const { return count_; }
   void set_count(int count) { count_ = count; }
 
-  void quit() {
-    base::MessageLoop::current()->Quit();
+  void Quit() {
+    base::MessageLoop::current()->QuitNow();
   }
 
  private:
@@ -43,7 +44,7 @@ class Result : public Wrappable<Result> {
       v8::Isolate* isolate) OVERRIDE {
     return Wrappable<Result>::GetObjectTemplateBuilder(isolate)
         .SetProperty("count", &Result::count, &Result::set_count)
-        .SetMethod("quit", &Result::quit);
+        .SetMethod("quit", &Result::Quit);
   }
 
   int count_;
@@ -51,74 +52,105 @@ class Result : public Wrappable<Result> {
 
 WrapperInfo Result::kWrapperInfo = { gin::kEmbedderNativeGin };
 
-}  // namespace
-
-typedef V8Test TimerUnittest;
+struct TestHelper {
+  TestHelper(v8::Isolate* isolate)
+      : runner(new Runner(&delegate, isolate)),
+        scope(runner.get()),
+        timer_module(TimerModule::Create(isolate)),
+        result(Result::Create(isolate)),
+        loop(base::MessageLoop::TYPE_DEFAULT) {
+    EXPECT_FALSE(runner->global().IsEmpty());
+    runner->global()->Set(StringToV8(isolate, "timer"),
+                          timer_module->GetWrapper(isolate));
+    runner->global()->Set(StringToV8(isolate, "result"),
+                          result->GetWrapper(isolate));
+  }
 
-TEST_F(TimerUnittest, OneShot) {
-  v8::Isolate* isolate = instance_->isolate();
+  void QuitSoon() {
+    loop.PostDelayedTask(FROM_HERE, base::MessageLoop::QuitWhenIdleClosure(),
+                         base::TimeDelta::FromMilliseconds(0));
+  }
 
   RunnerDelegate delegate;
-  Runner runner(&delegate, isolate);
-  Runner::Scope scope(&runner);
+  scoped_ptr<Runner> runner;
+  Runner::Scope scope;
+  Handle<TimerModule> timer_module;
+  Handle<Result> result;
+  base::MessageLoop loop;
+};
 
-  Handle<TimerModule> timer_module = TimerModule::Create(isolate);
-  Handle<Result> result = Result::Create(isolate);
+}  // namespace
 
-  EXPECT_FALSE(runner.global().IsEmpty());
-  runner.global()->Set(StringToV8(isolate, "timer"),
-                       timer_module->GetWrapper(isolate));
-  runner.global()->Set(StringToV8(isolate, "result"),
-                       result->GetWrapper(isolate));
+typedef V8Test TimerUnittest;
 
+TEST_F(TimerUnittest, OneShot) {
+  TestHelper helper(instance_->isolate());
   std::string source =
-     "timer.createOneShot(100, function() {"
+     "timer.createOneShot(0, function() {"
      "  result.count++;"
-     "  result.quit();"
      "});";
 
-  base::MessageLoop loop(base::MessageLoop::TYPE_DEFAULT);
-  runner.Run(source, "script");
-  EXPECT_EQ(0, result->count());
+  helper.runner->Run(source, "script");
+  EXPECT_EQ(0, helper.result->count());
 
-  loop.Run();
-  loop.RunUntilIdle();
-
-  EXPECT_EQ(1, result->count());
+  helper.QuitSoon();
+  helper.loop.Run();
+  EXPECT_EQ(1, helper.result->count());
 }
 
-TEST_F(TimerUnittest, Repeating) {
-  v8::Isolate* isolate = instance_->isolate();
+TEST_F(TimerUnittest, OneShotCancel) {
+  TestHelper helper(instance_->isolate());
+  std::string source =
+     "var t = timer.createOneShot(0, function() {"
+     "  result.count++;"
+     "});"
+     "t.cancel()";
 
-  RunnerDelegate delegate;
-  Runner runner(&delegate, isolate);
-  Runner::Scope scope(&runner);
+  helper.runner->Run(source, "script");
+  EXPECT_EQ(0, helper.result->count());
 
-  Handle<TimerModule> timer_module = TimerModule::Create(isolate);
-  Handle<Result> result = Result::Create(isolate);
+  helper.QuitSoon();
+  helper.loop.Run();
+  EXPECT_EQ(0, helper.result->count());
+}
 
-  EXPECT_FALSE(runner.global().IsEmpty());
-  runner.global()->Set(StringToV8(isolate, "timer"),
-                       timer_module->GetWrapper(isolate));
-  runner.global()->Set(StringToV8(isolate, "result"),
-                       result->GetWrapper(isolate));
+TEST_F(TimerUnittest, Repeating) {
+  TestHelper helper(instance_->isolate());
 
   // TODO(aa): Cannot do: if (++result.count == 3) because of v8 bug. Create
   // test case and report.
   std::string source =
-     "timer.createRepeating(10, function() {"
+     "timer.createRepeating(0, function() {"
      "  result.count++;"
      "  if (result.count == 3) {"
      "    result.quit();"
      "  }"
      "});";
 
-  base::MessageLoop loop(base::MessageLoop::TYPE_DEFAULT);
-  runner.Run(source, "script");
-  EXPECT_EQ(0, result->count());
+  helper.runner->Run(source, "script");
+  EXPECT_EQ(0, helper.result->count());
+
+  helper.loop.Run();
+  EXPECT_EQ(3, helper.result->count());
+}
+
+TEST_F(TimerUnittest, TimerCallbackToDestroyedRunner) {
+  TestHelper helper(instance_->isolate());
+  std::string source =
+     "timer.createOneShot(0, function() {"
+     "  result.count++;"
+     "});";
+
+  helper.runner->Run(source, "script");
+  EXPECT_EQ(0, helper.result->count());
+
+  // Destroy runner, which should destroy the timer object we created.
+  helper.QuitSoon();
+  helper.runner.reset(NULL);
+  helper.loop.Run();
 
-  loop.Run();
-  EXPECT_EQ(3, result->count());
+  // Timer should not have run because it was deleted.
+  EXPECT_EQ(0, helper.result->count());
 }
 
 }  // namespace gin