From 38f2d25a84c044ba3565cc54a67a6c2b6256d2eb Mon Sep 17 00:00:00 2001 From: "vegorov@chromium.org" Date: Tue, 20 May 2014 08:24:51 +0000 Subject: [PATCH] Ensure that interruptor callback registered through API is called outside of ExecutionAccess lock. Such a coarse locking can cause a dead-lock when another thread is attempting to clear an interrupt while we are waiting in the interrupt callback. Add test that verifies this API invariant. BUG=chromium:374978 LOG=N R=yangguo@chromium.org Review URL: https://codereview.chromium.org/291123002 git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@21376 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/execution.cc | 61 ++++++++++++++++------------- src/isolate.cc | 14 +++++-- test/cctest/test-api.cc | 102 +++++++++++++++++++++++++++++++++++++++++------- 3 files changed, 132 insertions(+), 45 deletions(-) diff --git a/src/execution.cc b/src/execution.cc index b550415..62a4cbe 100644 --- a/src/execution.cc +++ b/src/execution.cc @@ -713,43 +713,50 @@ void Execution::ProcessDebugMessages(Isolate* isolate, Object* StackGuard::HandleInterrupts() { - ExecutionAccess access(isolate_); - if (should_postpone_interrupts(access)) { - return isolate_->heap()->undefined_value(); - } + bool has_api_interrupt = false; + { + ExecutionAccess access(isolate_); + if (should_postpone_interrupts(access)) { + return isolate_->heap()->undefined_value(); + } - if (CheckAndClearInterrupt(API_INTERRUPT, access)) { - isolate_->InvokeApiInterruptCallback(); - } + if (CheckAndClearInterrupt(GC_REQUEST, access)) { + isolate_->heap()->CollectAllGarbage(Heap::kNoGCFlags, "GC interrupt"); + } - if (CheckAndClearInterrupt(GC_REQUEST, access)) { - isolate_->heap()->CollectAllGarbage(Heap::kNoGCFlags, "GC interrupt"); - } + if (CheckDebugBreak() || CheckDebugCommand()) { + Execution::DebugBreakHelper(isolate_); + } - if (CheckDebugBreak() || CheckDebugCommand()) { - Execution::DebugBreakHelper(isolate_); - } + if (CheckAndClearInterrupt(TERMINATE_EXECUTION, access)) { + return isolate_->TerminateExecution(); + } - if (CheckAndClearInterrupt(TERMINATE_EXECUTION, access)) { - return isolate_->TerminateExecution(); - } + if (CheckAndClearInterrupt(FULL_DEOPT, access)) { + Deoptimizer::DeoptimizeAll(isolate_); + } - if (CheckAndClearInterrupt(FULL_DEOPT, access)) { - Deoptimizer::DeoptimizeAll(isolate_); - } + if (CheckAndClearInterrupt(DEOPT_MARKED_ALLOCATION_SITES, access)) { + isolate_->heap()->DeoptMarkedAllocationSites(); + } - if (CheckAndClearInterrupt(DEOPT_MARKED_ALLOCATION_SITES, access)) { - isolate_->heap()->DeoptMarkedAllocationSites(); + if (CheckAndClearInterrupt(INSTALL_CODE, access)) { + ASSERT(isolate_->concurrent_recompilation_enabled()); + isolate_->optimizing_compiler_thread()->InstallOptimizedFunctions(); + } + + has_api_interrupt = CheckAndClearInterrupt(API_INTERRUPT, access); + + isolate_->counters()->stack_interrupts()->Increment(); + isolate_->counters()->runtime_profiler_ticks()->Increment(); + isolate_->runtime_profiler()->OptimizeNow(); } - if (CheckAndClearInterrupt(INSTALL_CODE, access)) { - ASSERT(isolate_->concurrent_recompilation_enabled()); - isolate_->optimizing_compiler_thread()->InstallOptimizedFunctions(); + if (has_api_interrupt) { + // Callback must be invoked outside of ExecusionAccess lock. + isolate_->InvokeApiInterruptCallback(); } - isolate_->counters()->stack_interrupts()->Increment(); - isolate_->counters()->runtime_profiler_ticks()->Increment(); - isolate_->runtime_profiler()->OptimizeNow(); return isolate_->heap()->undefined_value(); } diff --git a/src/isolate.cc b/src/isolate.cc index 46f6d71..82ace29 100644 --- a/src/isolate.cc +++ b/src/isolate.cc @@ -842,10 +842,16 @@ void Isolate::CancelTerminateExecution() { void Isolate::InvokeApiInterruptCallback() { - InterruptCallback callback = api_interrupt_callback_; - void* data = api_interrupt_callback_data_; - api_interrupt_callback_ = NULL; - api_interrupt_callback_data_ = NULL; + // Note: callback below should be called outside of execution access lock. + InterruptCallback callback = NULL; + void* data = NULL; + { + ExecutionAccess access(this); + callback = api_interrupt_callback_; + data = api_interrupt_callback_data_; + api_interrupt_callback_ = NULL; + api_interrupt_callback_data_ = NULL; + } if (callback != NULL) { VMState state(this); diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index cbead0f..d7f6602 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -21819,11 +21819,12 @@ class RequestInterruptTestBase { virtual ~RequestInterruptTestBase() { } + virtual void StartInterruptThread() = 0; + virtual void TestBody() = 0; void RunTest() { - InterruptThread i_thread(this); - i_thread.Start(); + StartInterruptThread(); v8::HandleScope handle_scope(isolate_); @@ -21852,7 +21853,6 @@ class RequestInterruptTestBase { return should_continue_; } - protected: static void ShouldContinueCallback( const v8::FunctionCallbackInfo& info) { RequestInterruptTestBase* test = @@ -21861,6 +21861,24 @@ class RequestInterruptTestBase { info.GetReturnValue().Set(test->ShouldContinue()); } + LocalContext env_; + v8::Isolate* isolate_; + i::Semaphore sem_; + int warmup_; + bool should_continue_; +}; + + +class RequestInterruptTestBaseWithSimpleInterrupt + : public RequestInterruptTestBase { + public: + RequestInterruptTestBaseWithSimpleInterrupt() : i_thread(this) { } + + virtual void StartInterruptThread() { + i_thread.Start(); + } + + private: class InterruptThread : public i::Thread { public: explicit InterruptThread(RequestInterruptTestBase* test) @@ -21880,15 +21898,12 @@ class RequestInterruptTestBase { RequestInterruptTestBase* test_; }; - LocalContext env_; - v8::Isolate* isolate_; - i::Semaphore sem_; - int warmup_; - bool should_continue_; + InterruptThread i_thread; }; -class RequestInterruptTestWithFunctionCall : public RequestInterruptTestBase { +class RequestInterruptTestWithFunctionCall + : public RequestInterruptTestBaseWithSimpleInterrupt { public: virtual void TestBody() { Local func = Function::New( @@ -21900,7 +21915,8 @@ class RequestInterruptTestWithFunctionCall : public RequestInterruptTestBase { }; -class RequestInterruptTestWithMethodCall : public RequestInterruptTestBase { +class RequestInterruptTestWithMethodCall + : public RequestInterruptTestBaseWithSimpleInterrupt { public: virtual void TestBody() { v8::Local t = v8::FunctionTemplate::New(isolate_); @@ -21914,7 +21930,8 @@ class RequestInterruptTestWithMethodCall : public RequestInterruptTestBase { }; -class RequestInterruptTestWithAccessor : public RequestInterruptTestBase { +class RequestInterruptTestWithAccessor + : public RequestInterruptTestBaseWithSimpleInterrupt { public: virtual void TestBody() { v8::Local t = v8::FunctionTemplate::New(isolate_); @@ -21928,7 +21945,8 @@ class RequestInterruptTestWithAccessor : public RequestInterruptTestBase { }; -class RequestInterruptTestWithNativeAccessor : public RequestInterruptTestBase { +class RequestInterruptTestWithNativeAccessor + : public RequestInterruptTestBaseWithSimpleInterrupt { public: virtual void TestBody() { v8::Local t = v8::FunctionTemplate::New(isolate_); @@ -21955,7 +21973,7 @@ class RequestInterruptTestWithNativeAccessor : public RequestInterruptTestBase { class RequestInterruptTestWithMethodCallAndInterceptor - : public RequestInterruptTestBase { + : public RequestInterruptTestBaseWithSimpleInterrupt { public: virtual void TestBody() { v8::Local t = v8::FunctionTemplate::New(isolate_); @@ -21978,7 +21996,8 @@ class RequestInterruptTestWithMethodCallAndInterceptor }; -class RequestInterruptTestWithMathAbs : public RequestInterruptTestBase { +class RequestInterruptTestWithMathAbs + : public RequestInterruptTestBaseWithSimpleInterrupt { public: virtual void TestBody() { env_->Global()->Set(v8_str("WakeUpInterruptor"), Function::New( @@ -22062,6 +22081,61 @@ TEST(RequestInterruptTestWithMathAbs) { } +class ClearInterruptFromAnotherThread + : public RequestInterruptTestBase { + public: + ClearInterruptFromAnotherThread() : i_thread(this), sem2_(0) { } + + virtual void StartInterruptThread() { + i_thread.Start(); + } + + virtual void TestBody() { + Local func = Function::New( + isolate_, ShouldContinueCallback, v8::External::New(isolate_, this)); + env_->Global()->Set(v8_str("ShouldContinue"), func); + + CompileRun("while (ShouldContinue()) { }"); + } + + private: + class InterruptThread : public i::Thread { + public: + explicit InterruptThread(ClearInterruptFromAnotherThread* test) + : Thread("RequestInterruptTest"), test_(test) {} + + virtual void Run() { + test_->sem_.Wait(); + test_->isolate_->RequestInterrupt(&OnInterrupt, test_); + test_->sem_.Wait(); + test_->isolate_->ClearInterrupt(); + test_->sem2_.Signal(); + } + + static void OnInterrupt(v8::Isolate* isolate, void* data) { + ClearInterruptFromAnotherThread* test = + reinterpret_cast(data); + test->sem_.Signal(); + bool success = test->sem2_.WaitFor(i::TimeDelta::FromSeconds(2)); + // Crash instead of timeout to make this failure more prominent. + CHECK(success); + test->should_continue_ = false; + } + + private: + ClearInterruptFromAnotherThread* test_; + }; + + InterruptThread i_thread; + i::Semaphore sem2_; +}; + + +TEST(ClearInterruptFromAnotherThread) { + ClearInterruptFromAnotherThread().RunTest(); +} + + static Local function_new_expected_env; static void FunctionNewCallback(const v8::FunctionCallbackInfo& info) { CHECK_EQ(function_new_expected_env, info.Data()); -- 2.7.4