Fix issue 142:

- Removed the potential for a NULL pointer access in
  ContextSwitcher::PreemptionReceived.
- Removed a leak of the semaphore in the ContexSwitcher thread, by removing
  the need for this semaphore entirely.
- Added a regression test case which will catch accesses to the ContextSwitcher
  singleton after it has been stopped.

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

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@994 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/v8threads.cc b/src/v8threads.cc
index 2b4a02715e65eb87ac479bd18af6fbf3f216cf56..6b7533b6be691204c1014bfe31094bb71b8416e2 100644 (file)
--- a/src/v8threads.cc
+++ b/src/v8threads.cc
@@ -269,62 +269,64 @@ void ThreadManager::MarkCompactEpilogue() {
 }
 
 
+// This is the ContextSwitcher singleton. There is at most a single thread
+// running which delivers preemption events to V8 threads.
+ContextSwitcher* ContextSwitcher::singleton_ = NULL;
+
+
 ContextSwitcher::ContextSwitcher(int every_n_ms)
-  : preemption_semaphore_(OS::CreateSemaphore(0)),
-    keep_going_(true),
+  : keep_going_(true),
     sleep_ms_(every_n_ms) {
 }
 
 
-static v8::internal::ContextSwitcher* switcher;
-
-
+// Set the scheduling interval of V8 threads. This function starts the
+// ContextSwitcher thread if needed.
 void ContextSwitcher::StartPreemption(int every_n_ms) {
   ASSERT(Locker::IsLocked());
-  if (switcher == NULL) {
-    switcher = new ContextSwitcher(every_n_ms);
-    switcher->Start();
+  if (singleton_ == NULL) {
+    // If the ContextSwitcher thread is not running at the moment start it now.
+    singleton_ = new ContextSwitcher(every_n_ms);
+    singleton_->Start();
   } else {
-    switcher->sleep_ms_ = every_n_ms;
+    // ContextSwitcher thread is already running, so we just change the
+    // scheduling interval.
+    singleton_->sleep_ms_ = every_n_ms;
   }
 }
 
 
+// Disable preemption of V8 threads. If multiple threads want to use V8 they
+// must cooperatively schedule amongst them from this point on.
 void ContextSwitcher::StopPreemption() {
   ASSERT(Locker::IsLocked());
-  if (switcher != NULL) {
-    switcher->Stop();
-    delete(switcher);
-    switcher = NULL;
+  if (singleton_ != NULL) {
+    // The ContextSwitcher thread is running. We need to stop it and release
+    // its resources.
+    singleton_->keep_going_ = false;
+    singleton_->Join();  // Wait for the ContextSwitcher thread to exit.
+    // Thread has exited, now we can delete it.
+    delete(singleton_);
+    singleton_ = NULL;
   }
 }
 
 
+// Main loop of the ContextSwitcher thread: Preempt the currently running V8
+// thread at regular intervals.
 void ContextSwitcher::Run() {
   while (keep_going_) {
     OS::Sleep(sleep_ms_);
     StackGuard::Preempt();
-    WaitForPreemption();
   }
 }
 
 
-void ContextSwitcher::Stop() {
-  ASSERT(Locker::IsLocked());
-  keep_going_ = false;
-  preemption_semaphore_->Signal();
-  Join();
-}
-
-
-void ContextSwitcher::WaitForPreemption() {
-  preemption_semaphore_->Wait();
-}
-
-
+// Acknowledge the preemption by the receiving thread.
 void ContextSwitcher::PreemptionReceived() {
   ASSERT(Locker::IsLocked());
-  switcher->preemption_semaphore_->Signal();
+  // There is currently no accounting being done for this. But could be in the
+  // future, which is why we leave this in.
 }
 
 

diff --git a/src/v8threads.h b/src/v8threads.h
index f5c844d8ae871661802b57bb83e65d9cb391cb6d..557a8f19c94cfe296d8522f917df882cd15b3bc3 100644 (file)
--- a/src/v8threads.h
+++ b/src/v8threads.h
@@ -87,19 +87,31 @@ class ThreadManager : public AllStatic {
 };
 
 
+// The ContextSwitcher thread is used to schedule regular preemptions to
+// multiple running V8 threads. Generally it is necessary to call
+// StartPreemption if there is more than one thread running. If not, a single
+// JavaScript can take full control of V8 and not allow other threads to run.
 class ContextSwitcher: public Thread {
  public:
-  void Run();
+  // Set the preemption interval for the ContextSwitcher thread.
   static void StartPreemption(int every_n_ms);
+
+  // Stop sending preemption requests to threads.
   static void StopPreemption();
+
+  // Preempted thread needs to call back to the ContextSwitcher to acknowlege
+  // the handling of a preemption request.
   static void PreemptionReceived();
+
  private:
   explicit ContextSwitcher(int every_n_ms);
-  void WaitForPreemption();
-  void Stop();
-  Semaphore* preemption_semaphore_;
+
+  void Run();
+
   bool keep_going_;
   int sleep_ms_;
+
+  static ContextSwitcher* singleton_;
 };
 
 } }  // namespace v8::internal

diff --git a/test/cctest/SConscript b/test/cctest/SConscript
index c2a96f2729921d4564baa3bbb2dcdeb022cfdd46..c70b07d0fa0b499ed6a4cd300de5a9f06c09a18e 100644 (file)
--- a/test/cctest/SConscript
+++ b/test/cctest/SConscript
@@ -38,7 +38,7 @@ SOURCES = {
     'test-ast.cc', 'test-heap.cc', 'test-utils.cc', 'test-compiler.cc',
     'test-spaces.cc', 'test-mark-compact.cc', 'test-lock.cc',
     'test-conversions.cc', 'test-strings.cc', 'test-serialize.cc',
-    'test-decls.cc', 'test-alloc.cc', 'test-regexp.cc'
+    'test-decls.cc', 'test-alloc.cc', 'test-regexp.cc', 'test-threads.cc'
   ],
   'arch:arm':  ['test-assembler-arm.cc', 'test-disasm-arm.cc'],
   'arch:ia32': ['test-assembler-ia32.cc', 'test-disasm-ia32.cc'],

diff --git a/test/cctest/test-threads.cc b/test/cctest/test-threads.cc
new file mode 100644 (file)
index 0000000..c0d55f2
--- /dev/null
+++ b/test/cctest/test-threads.cc
@@ -0,0 +1,54 @@
+// Copyright 2008 the V8 project authors. All rights reserved.
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+//       notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+//       copyright notice, this list of conditions and the following
+//       disclaimer in the documentation and/or other materials provided
+//       with the distribution.
+//     * Neither the name of Google Inc. nor the names of its
+//       contributors may be used to endorse or promote products derived
+//       from this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+#include "v8.h"
+
+#include "platform.h"
+
+#include "cctest.h"
+
+
+TEST(Preemption) {
+  v8::Locker locker;
+  v8::V8::Initialize();
+  v8::HandleScope scope;
+  v8::Context::Scope context_scope(v8::Context::New());
+
+  v8::Locker::StartPreemption(100);
+
+  v8::Handle<v8::Script> script = v8::Script::Compile(
+      v8::String::New("var count = 0; var obj = new Object(); count++;\n"));
+
+  script->Run();
+
+  v8::Locker::StopPreemption();
+  v8::internal::OS::Sleep(500);  // Make sure the timer fires.
+
+  script->Run();
+}
+
+