From 358591f9ea7a2953ddc41016668fcac876ee200a Mon Sep 17 00:00:00 2001 From: "iposva@chromium.org" Date: Wed, 17 Dec 2008 17:40:02 +0000 Subject: [PATCH] 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 --- src/v8threads.cc | 58 +++++++++++++++++++------------------ src/v8threads.h | 20 ++++++++++--- test/cctest/SConscript | 2 +- test/cctest/test-threads.cc | 54 ++++++++++++++++++++++++++++++++++ 4 files changed, 101 insertions(+), 33 deletions(-) create mode 100644 test/cctest/test-threads.cc diff --git a/src/v8threads.cc b/src/v8threads.cc index 2b4a02715..6b7533b6b 100644 --- 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 f5c844d8a..557a8f19c 100644 --- 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 c2a96f272..c70b07d0f 100644 --- 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 index 000000000..c0d55f250 --- /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 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(); +} + + -- 2.34.1