Revision: 21376
Author: [email protected]
Date: Tue May 20 08:24:51 2014 UTC
Log: 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
[email protected]
Review URL: https://codereview.chromium.org/291123002
http://code.google.com/p/v8/source/detail?r=21376
Modified:
/branches/bleeding_edge/src/execution.cc
/branches/bleeding_edge/src/isolate.cc
/branches/bleeding_edge/test/cctest/test-api.cc
=======================================
--- /branches/bleeding_edge/src/execution.cc Mon May 19 07:57:04 2014 UTC
+++ /branches/bleeding_edge/src/execution.cc Tue May 20 08:24:51 2014 UTC
@@ -713,43 +713,50 @@
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(FULL_DEOPT, access)) {
+ Deoptimizer::DeoptimizeAll(isolate_);
+ }
+
+ if (CheckAndClearInterrupt(DEOPT_MARKED_ALLOCATION_SITES, access)) {
+ isolate_->heap()->DeoptMarkedAllocationSites();
+ }
- if (CheckAndClearInterrupt(TERMINATE_EXECUTION, access)) {
- return isolate_->TerminateExecution();
- }
+ if (CheckAndClearInterrupt(INSTALL_CODE, access)) {
+ ASSERT(isolate_->concurrent_recompilation_enabled());
+ isolate_->optimizing_compiler_thread()->InstallOptimizedFunctions();
+ }
- if (CheckAndClearInterrupt(FULL_DEOPT, access)) {
- Deoptimizer::DeoptimizeAll(isolate_);
- }
+ has_api_interrupt = CheckAndClearInterrupt(API_INTERRUPT, access);
- if (CheckAndClearInterrupt(DEOPT_MARKED_ALLOCATION_SITES, access)) {
- isolate_->heap()->DeoptMarkedAllocationSites();
+ 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();
}
=======================================
--- /branches/bleeding_edge/src/isolate.cc Mon May 19 13:45:45 2014 UTC
+++ /branches/bleeding_edge/src/isolate.cc Tue May 20 08:24:51 2014 UTC
@@ -842,10 +842,16 @@
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<EXTERNAL> state(this);
=======================================
--- /branches/bleeding_edge/test/cctest/test-api.cc Mon May 19 07:57:04
2014 UTC
+++ /branches/bleeding_edge/test/cctest/test-api.cc Tue May 20 08:24:51
2014 UTC
@@ -21818,12 +21818,13 @@
}
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 @@
return should_continue_;
}
- protected:
static void ShouldContinueCallback(
const v8::FunctionCallbackInfo<Value>& info) {
RequestInterruptTestBase* test =
@@ -21861,6 +21861,24 @@
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 @@
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<Function> func = Function::New(
@@ -21900,7 +21915,8 @@
};
-class RequestInterruptTestWithMethodCall : public RequestInterruptTestBase
{
+class RequestInterruptTestWithMethodCall
+ : public RequestInterruptTestBaseWithSimpleInterrupt {
public:
virtual void TestBody() {
v8::Local<v8::FunctionTemplate> t =
v8::FunctionTemplate::New(isolate_);
@@ -21914,7 +21930,8 @@
};
-class RequestInterruptTestWithAccessor : public RequestInterruptTestBase {
+class RequestInterruptTestWithAccessor
+ : public RequestInterruptTestBaseWithSimpleInterrupt {
public:
virtual void TestBody() {
v8::Local<v8::FunctionTemplate> t =
v8::FunctionTemplate::New(isolate_);
@@ -21928,7 +21945,8 @@
};
-class RequestInterruptTestWithNativeAccessor : public
RequestInterruptTestBase {
+class RequestInterruptTestWithNativeAccessor
+ : public RequestInterruptTestBaseWithSimpleInterrupt {
public:
virtual void TestBody() {
v8::Local<v8::FunctionTemplate> t =
v8::FunctionTemplate::New(isolate_);
@@ -21955,7 +21973,7 @@
class RequestInterruptTestWithMethodCallAndInterceptor
- : public RequestInterruptTestBase {
+ : public RequestInterruptTestBaseWithSimpleInterrupt {
public:
virtual void TestBody() {
v8::Local<v8::FunctionTemplate> t =
v8::FunctionTemplate::New(isolate_);
@@ -21978,7 +21996,8 @@
};
-class RequestInterruptTestWithMathAbs : public RequestInterruptTestBase {
+class RequestInterruptTestWithMathAbs
+ : public RequestInterruptTestBaseWithSimpleInterrupt {
public:
virtual void TestBody() {
env_->Global()->Set(v8_str("WakeUpInterruptor"), Function::New(
@@ -22060,6 +22079,61 @@
TEST(RequestInterruptTestWithMathAbs) {
RequestInterruptTestWithMathAbs().RunTest();
}
+
+
+class ClearInterruptFromAnotherThread
+ : public RequestInterruptTestBase {
+ public:
+ ClearInterruptFromAnotherThread() : i_thread(this), sem2_(0) { }
+
+ virtual void StartInterruptThread() {
+ i_thread.Start();
+ }
+
+ virtual void TestBody() {
+ Local<Function> 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<ClearInterruptFromAnotherThread*>(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<Value> function_new_expected_env;
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
