ASSERTION FAILED: m_numBreakpoints >= numBreakpoints when deleting breakpoints.
<https://webkit.org/b/129393>
Reviewed by Geoffrey Garen.
The issue manifests because the debugger will iterate all CodeBlocks in
the heap when setting / clearing breakpoints, but it is possible for a
CodeBlock to have been instantiate but is not yet registered with the
debugger. This can happen because of the following:
1. DFG worklist compilation is still in progress, and the target
codeBlock is not ready for installation in its executable yet.
2. DFG compilation failed and we have a codeBlock that will never be
installed in its executable, and the codeBlock has not been cleaned
up by the GC yet.
The code for installing the codeBlock in its executable is the same code
that registers it with the debugger. Hence, these codeBlocks are not
registered with the debugger, and any pending breakpoints that would map
to that CodeBlock is as yet unset or will never be set. As such, an
attempt to remove a breakpoint in that CodeBlock will fail that assertion.
To fix this, we do the following:
1. We'll eagerly clean up any zombie CodeBlocks due to failed DFG / FTL
compilation. This is achieved by providing a
DeferredCompilationCallback::compilationDidComplete() that does this
clean up, and have all sub classes call it at the end of their
compilationDidComplete() methods.
2. Before the debugger or profiler iterates CodeBlocks in the heap, they
will wait for all compilations to complete before proceeding. This
ensures that:
1. any zombie CodeBlocks would have been cleaned up, and won't be
seen by the debugger or profiler.
2. all CodeBlocks that the debugger and profiler needs to operate on
will be "ready" for whatever needs to be done to them e.g.
jettison'ing of DFG codeBlocks.
* bytecode/DeferredCompilationCallback.cpp:
(JSC::DeferredCompilationCallback::compilationDidComplete):
* bytecode/DeferredCompilationCallback.h:
- Provide default implementation method to clean up zombie CodeBlocks.
* debugger/Debugger.cpp:
(JSC::Debugger::forEachCodeBlock):
- Utility function to iterate CodeBlocks. It ensures that all compilations
are complete before proceeding.
(JSC::Debugger::setSteppingMode):
(JSC::Debugger::toggleBreakpoint):
(JSC::Debugger::recompileAllJSFunctions):
(JSC::Debugger::clearBreakpoints):
(JSC::Debugger::clearDebuggerRequests):
- Use the utility iterator function.
* debugger/Debugger.h:
* dfg/DFGOperations.cpp:
- Added an assert to ensure that zombie CodeBlocks will be imminently cleaned up.
* dfg/DFGPlan.cpp:
(JSC::DFG::Plan::finalizeWithoutNotifyingCallback):
- Remove unneeded code (that was not the best solution anyway) for ensuring
that we don't generate new DFG codeBlocks after enabling the debugger or
profiler. Now that we wait for compilations to complete before proceeding
with debugger and profiler work, this scenario will never happen.
* dfg/DFGToFTLDeferredCompilationCallback.cpp:
(JSC::DFG::ToFTLDeferredCompilationCallback::compilationDidComplete):
- Call the super class method to clean up zombie codeBlocks.
* dfg/DFGToFTLForOSREntryDeferredCompilationCallback.cpp:
(JSC::DFG::ToFTLForOSREntryDeferredCompilationCallback::compilationDidComplete):
- Call the super class method to clean up zombie codeBlocks.
* heap/CodeBlockSet.cpp:
(JSC::CodeBlockSet::remove):
* heap/CodeBlockSet.h:
* heap/Heap.h:
(JSC::Heap::removeCodeBlock):
- New method to remove a codeBlock from the codeBlock set.
* jit/JITOperations.cpp:
- Added an assert to ensure that zombie CodeBlocks will be imminently cleaned up.
* jit/JITToDFGDeferredCompilationCallback.cpp:
(JSC::JITToDFGDeferredCompilationCallback::compilationDidComplete):
- Call the super class method to clean up zombie codeBlocks.
* runtime/VM.cpp:
(JSC::VM::waitForCompilationsToComplete):
- Renamed from prepareToDiscardCode() to be clearer about what it does.
(JSC::VM::discardAllCode):
(JSC::VM::releaseExecutableMemory):
(JSC::VM::setEnabledProfiler):
- Wait for compilation to complete before enabling the profiler.
* runtime/VM.h:
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@165005 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index cab4014..924e28a 100644
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,105 @@
+2014-03-03 Mark Lam <mark.lam@apple.com>
+
+ ASSERTION FAILED: m_numBreakpoints >= numBreakpoints when deleting breakpoints.
+ <https://webkit.org/b/129393>
+
+ Reviewed by Geoffrey Garen.
+
+ The issue manifests because the debugger will iterate all CodeBlocks in
+ the heap when setting / clearing breakpoints, but it is possible for a
+ CodeBlock to have been instantiate but is not yet registered with the
+ debugger. This can happen because of the following:
+
+ 1. DFG worklist compilation is still in progress, and the target
+ codeBlock is not ready for installation in its executable yet.
+
+ 2. DFG compilation failed and we have a codeBlock that will never be
+ installed in its executable, and the codeBlock has not been cleaned
+ up by the GC yet.
+
+ The code for installing the codeBlock in its executable is the same code
+ that registers it with the debugger. Hence, these codeBlocks are not
+ registered with the debugger, and any pending breakpoints that would map
+ to that CodeBlock is as yet unset or will never be set. As such, an
+ attempt to remove a breakpoint in that CodeBlock will fail that assertion.
+
+ To fix this, we do the following:
+
+ 1. We'll eagerly clean up any zombie CodeBlocks due to failed DFG / FTL
+ compilation. This is achieved by providing a
+ DeferredCompilationCallback::compilationDidComplete() that does this
+ clean up, and have all sub classes call it at the end of their
+ compilationDidComplete() methods.
+
+ 2. Before the debugger or profiler iterates CodeBlocks in the heap, they
+ will wait for all compilations to complete before proceeding. This
+ ensures that:
+ 1. any zombie CodeBlocks would have been cleaned up, and won't be
+ seen by the debugger or profiler.
+ 2. all CodeBlocks that the debugger and profiler needs to operate on
+ will be "ready" for whatever needs to be done to them e.g.
+ jettison'ing of DFG codeBlocks.
+
+ * bytecode/DeferredCompilationCallback.cpp:
+ (JSC::DeferredCompilationCallback::compilationDidComplete):
+ * bytecode/DeferredCompilationCallback.h:
+ - Provide default implementation method to clean up zombie CodeBlocks.
+
+ * debugger/Debugger.cpp:
+ (JSC::Debugger::forEachCodeBlock):
+ - Utility function to iterate CodeBlocks. It ensures that all compilations
+ are complete before proceeding.
+ (JSC::Debugger::setSteppingMode):
+ (JSC::Debugger::toggleBreakpoint):
+ (JSC::Debugger::recompileAllJSFunctions):
+ (JSC::Debugger::clearBreakpoints):
+ (JSC::Debugger::clearDebuggerRequests):
+ - Use the utility iterator function.
+
+ * debugger/Debugger.h:
+ * dfg/DFGOperations.cpp:
+ - Added an assert to ensure that zombie CodeBlocks will be imminently cleaned up.
+
+ * dfg/DFGPlan.cpp:
+ (JSC::DFG::Plan::finalizeWithoutNotifyingCallback):
+ - Remove unneeded code (that was not the best solution anyway) for ensuring
+ that we don't generate new DFG codeBlocks after enabling the debugger or
+ profiler. Now that we wait for compilations to complete before proceeding
+ with debugger and profiler work, this scenario will never happen.
+
+ * dfg/DFGToFTLDeferredCompilationCallback.cpp:
+ (JSC::DFG::ToFTLDeferredCompilationCallback::compilationDidComplete):
+ - Call the super class method to clean up zombie codeBlocks.
+
+ * dfg/DFGToFTLForOSREntryDeferredCompilationCallback.cpp:
+ (JSC::DFG::ToFTLForOSREntryDeferredCompilationCallback::compilationDidComplete):
+ - Call the super class method to clean up zombie codeBlocks.
+
+ * heap/CodeBlockSet.cpp:
+ (JSC::CodeBlockSet::remove):
+ * heap/CodeBlockSet.h:
+ * heap/Heap.h:
+ (JSC::Heap::removeCodeBlock):
+ - New method to remove a codeBlock from the codeBlock set.
+
+ * jit/JITOperations.cpp:
+ - Added an assert to ensure that zombie CodeBlocks will be imminently cleaned up.
+
+ * jit/JITToDFGDeferredCompilationCallback.cpp:
+ (JSC::JITToDFGDeferredCompilationCallback::compilationDidComplete):
+ - Call the super class method to clean up zombie codeBlocks.
+
+ * runtime/VM.cpp:
+ (JSC::VM::waitForCompilationsToComplete):
+ - Renamed from prepareToDiscardCode() to be clearer about what it does.
+
+ (JSC::VM::discardAllCode):
+ (JSC::VM::releaseExecutableMemory):
+ (JSC::VM::setEnabledProfiler):
+ - Wait for compilation to complete before enabling the profiler.
+
+ * runtime/VM.h:
+
2014-03-03 Brian Burg <bburg@apple.com>
Another unreviewed build fix attempt for Windows after r164986.
diff --git a/Source/JavaScriptCore/bytecode/DeferredCompilationCallback.cpp b/Source/JavaScriptCore/bytecode/DeferredCompilationCallback.cpp
index 35af7c7..9d5acb9 100644
--- a/Source/JavaScriptCore/bytecode/DeferredCompilationCallback.cpp
+++ b/Source/JavaScriptCore/bytecode/DeferredCompilationCallback.cpp
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2013, 2014 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -26,10 +26,26 @@
#include "config.h"
#include "DeferredCompilationCallback.h"
+#include "CodeBlock.h"
+
namespace JSC {
DeferredCompilationCallback::DeferredCompilationCallback() { }
DeferredCompilationCallback::~DeferredCompilationCallback() { }
+void DeferredCompilationCallback::compilationDidComplete(CodeBlock* codeBlock, CompilationResult result)
+{
+ switch (result) {
+ case CompilationFailed:
+ case CompilationInvalidated:
+ codeBlock->heap()->removeCodeBlock(codeBlock);
+ break;
+ case CompilationSuccessful:
+ break;
+ case CompilationDeferred:
+ RELEASE_ASSERT_NOT_REACHED();
+ }
+}
+
} // JSC
diff --git a/Source/JavaScriptCore/bytecode/DeferredCompilationCallback.h b/Source/JavaScriptCore/bytecode/DeferredCompilationCallback.h
index 6421e3e..5d252ca 100644
--- a/Source/JavaScriptCore/bytecode/DeferredCompilationCallback.h
+++ b/Source/JavaScriptCore/bytecode/DeferredCompilationCallback.h
@@ -41,7 +41,7 @@
virtual ~DeferredCompilationCallback();
virtual void compilationDidBecomeReadyAsynchronously(CodeBlock*) = 0;
- virtual void compilationDidComplete(CodeBlock*, CompilationResult) = 0;
+ virtual void compilationDidComplete(CodeBlock*, CompilationResult);
};
} // namespace JSC
diff --git a/Source/JavaScriptCore/debugger/Debugger.cpp b/Source/JavaScriptCore/debugger/Debugger.cpp
index dba1337..0b97c7a 100644
--- a/Source/JavaScriptCore/debugger/Debugger.cpp
+++ b/Source/JavaScriptCore/debugger/Debugger.cpp
@@ -138,6 +138,13 @@
Debugger& m_debugger;
};
+template<typename Functor>
+void Debugger::forEachCodeBlock(Functor& functor)
+{
+ m_vm->waitForCompilationsToComplete();
+ m_vm->heap.forEachCodeBlock(functor);
+}
+
Debugger::Debugger(bool isInWorkerThread)
: m_vm(nullptr)
, m_pauseOnExceptionsState(DontPauseOnExceptions)
@@ -232,7 +239,7 @@
if (!m_vm)
return;
SetSteppingModeFunctor functor(this, mode);
- m_vm->heap.forEachCodeBlock(functor);
+ forEachCodeBlock(functor);
}
void Debugger::registerCodeBlock(CodeBlock* codeBlock)
@@ -316,7 +323,7 @@
if (!m_vm)
return;
ToggleBreakpointFunctor functor(this, breakpoint, enabledOrNot);
- m_vm->heap.forEachCodeBlock(functor);
+ forEachCodeBlock(functor);
}
void Debugger::recompileAllJSFunctions(VM* vm)
@@ -328,7 +335,7 @@
return;
}
- vm->prepareToDiscardCode();
+ vm->waitForCompilationsToComplete();
Recompiler recompiler(this);
HeapIterationScope iterationScope(vm->heap);
@@ -496,7 +503,7 @@
if (!m_vm)
return;
ClearCodeBlockDebuggerRequestsFunctor functor(this);
- m_vm->heap.forEachCodeBlock(functor);
+ forEachCodeBlock(functor);
}
class Debugger::ClearDebuggerRequestsFunctor {
@@ -521,7 +528,7 @@
{
ASSERT(m_vm);
ClearDebuggerRequestsFunctor functor(globalObject);
- m_vm->heap.forEachCodeBlock(functor);
+ forEachCodeBlock(functor);
}
void Debugger::setBreakpointsActivated(bool activated)
diff --git a/Source/JavaScriptCore/debugger/Debugger.h b/Source/JavaScriptCore/debugger/Debugger.h
index f7b734f..09a7663 100644
--- a/Source/JavaScriptCore/debugger/Debugger.h
+++ b/Source/JavaScriptCore/debugger/Debugger.h
@@ -182,6 +182,8 @@
void clearDebuggerRequests(JSGlobalObject*);
+ template<typename Functor> inline void forEachCodeBlock(Functor&);
+
VM* m_vm;
HashSet<JSGlobalObject*> m_globalObjects;
diff --git a/Source/JavaScriptCore/dfg/DFGOperations.cpp b/Source/JavaScriptCore/dfg/DFGOperations.cpp
index 9720df4..c7fc2a6 100644
--- a/Source/JavaScriptCore/dfg/DFGOperations.cpp
+++ b/Source/JavaScriptCore/dfg/DFGOperations.cpp
@@ -1253,13 +1253,16 @@
Operands<JSValue> mustHandleValues;
jitCode->reconstruct(
exec, codeBlock, CodeOrigin(bytecodeIndex), streamIndex, mustHandleValues);
+ RefPtr<CodeBlock> replacementCodeBlock = codeBlock->newReplacement();
CompilationResult forEntryResult = compile(
- *vm, codeBlock->newReplacement().get(), codeBlock, FTLForOSREntryMode, bytecodeIndex,
+ *vm, replacementCodeBlock.get(), codeBlock, FTLForOSREntryMode, bytecodeIndex,
mustHandleValues, ToFTLForOSREntryDeferredCompilationCallback::create(codeBlock));
- if (forEntryResult != CompilationSuccessful)
+ if (forEntryResult != CompilationSuccessful) {
+ ASSERT(forEntryResult == CompilationDeferred || replacementCodeBlock->hasOneRef());
return 0;
-
+ }
+
// It's possible that the for-entry compile already succeeded. In that case OSR
// entry will succeed unless we ran out of stack. It's not clear what we should do.
// We signal to try again after a while if that happens.
diff --git a/Source/JavaScriptCore/dfg/DFGPlan.cpp b/Source/JavaScriptCore/dfg/DFGPlan.cpp
index d264623..13139c3 100644
--- a/Source/JavaScriptCore/dfg/DFGPlan.cpp
+++ b/Source/JavaScriptCore/dfg/DFGPlan.cpp
@@ -391,13 +391,6 @@
if (!isStillValid())
return CompilationInvalidated;
- if (vm.enabledProfiler())
- return CompilationInvalidated;
-
- Debugger* debugger = codeBlock->globalObject()->debugger();
- if (debugger && (debugger->isStepping() || codeBlock->baselineAlternative()->hasDebuggerRequests()))
- return CompilationInvalidated;
-
bool result;
if (codeBlock->codeType() == FunctionCode)
result = finalizer->finalizeFunction();
diff --git a/Source/JavaScriptCore/dfg/DFGToFTLDeferredCompilationCallback.cpp b/Source/JavaScriptCore/dfg/DFGToFTLDeferredCompilationCallback.cpp
index d2001e8..d0162b7 100644
--- a/Source/JavaScriptCore/dfg/DFGToFTLDeferredCompilationCallback.cpp
+++ b/Source/JavaScriptCore/dfg/DFGToFTLDeferredCompilationCallback.cpp
@@ -85,6 +85,8 @@
m_dfgCodeBlock->jitCode()->dfg()->setOptimizationThresholdBasedOnCompilationResult(
m_dfgCodeBlock.get(), result);
+
+ DeferredCompilationCallback::compilationDidComplete(codeBlock, result);
}
} } // JSC::DFG
diff --git a/Source/JavaScriptCore/dfg/DFGToFTLForOSREntryDeferredCompilationCallback.cpp b/Source/JavaScriptCore/dfg/DFGToFTLForOSREntryDeferredCompilationCallback.cpp
index 1f041d8..47c4fbb 100644
--- a/Source/JavaScriptCore/dfg/DFGToFTLForOSREntryDeferredCompilationCallback.cpp
+++ b/Source/JavaScriptCore/dfg/DFGToFTLForOSREntryDeferredCompilationCallback.cpp
@@ -79,19 +79,19 @@
switch (result) {
case CompilationSuccessful:
jitCode->osrEntryBlock = codeBlock;
- return;
+ break;
case CompilationFailed:
jitCode->osrEntryRetry = 0;
jitCode->abandonOSREntry = true;
- return;
+ break;
case CompilationDeferred:
- return;
+ RELEASE_ASSERT_NOT_REACHED();
case CompilationInvalidated:
jitCode->osrEntryRetry = 0;
- return;
+ break;
}
- RELEASE_ASSERT_NOT_REACHED();
+ DeferredCompilationCallback::compilationDidComplete(codeBlock, result);
}
} } // JSC::DFG
diff --git a/Source/JavaScriptCore/heap/CodeBlockSet.cpp b/Source/JavaScriptCore/heap/CodeBlockSet.cpp
index d2a9bd9..666d85e 100644
--- a/Source/JavaScriptCore/heap/CodeBlockSet.cpp
+++ b/Source/JavaScriptCore/heap/CodeBlockSet.cpp
@@ -96,6 +96,12 @@
}
}
+void CodeBlockSet::remove(CodeBlock* codeBlock)
+{
+ codeBlock->deref();
+ m_set.remove(codeBlock);
+}
+
void CodeBlockSet::traceMarked(SlotVisitor& visitor)
{
if (verbose)
diff --git a/Source/JavaScriptCore/heap/CodeBlockSet.h b/Source/JavaScriptCore/heap/CodeBlockSet.h
index b529d4b..104ffc4 100644
--- a/Source/JavaScriptCore/heap/CodeBlockSet.h
+++ b/Source/JavaScriptCore/heap/CodeBlockSet.h
@@ -65,6 +65,8 @@
// by this set), and that have not been marked.
void deleteUnmarkedAndUnreferenced();
+ void remove(CodeBlock*);
+
// Trace all marked code blocks. The CodeBlock is free to make use of
// mayBeExecuting.
void traceMarked(SlotVisitor&);
diff --git a/Source/JavaScriptCore/heap/Heap.h b/Source/JavaScriptCore/heap/Heap.h
index 042798e..9d06090 100644
--- a/Source/JavaScriptCore/heap/Heap.h
+++ b/Source/JavaScriptCore/heap/Heap.h
@@ -209,6 +209,8 @@
template<typename T> void releaseSoon(RetainPtr<T>&&);
#endif
+ void removeCodeBlock(CodeBlock* cb) { m_codeBlocks.remove(cb); }
+
private:
friend class CodeBlock;
friend class CopiedBlock;
diff --git a/Source/JavaScriptCore/jit/JITOperations.cpp b/Source/JavaScriptCore/jit/JITOperations.cpp
index 7241bdf..336a4ae 100644
--- a/Source/JavaScriptCore/jit/JITOperations.cpp
+++ b/Source/JavaScriptCore/jit/JITOperations.cpp
@@ -1199,12 +1199,15 @@
mustHandleValues[i] = exec->uncheckedR(operand).jsValue();
}
+ RefPtr<CodeBlock> replacementCodeBlock = codeBlock->newReplacement();
CompilationResult result = DFG::compile(
- vm, codeBlock->newReplacement().get(), 0, DFG::DFGMode, bytecodeIndex,
+ vm, replacementCodeBlock.get(), 0, DFG::DFGMode, bytecodeIndex,
mustHandleValues, JITToDFGDeferredCompilationCallback::create());
- if (result != CompilationSuccessful)
+ if (result != CompilationSuccessful) {
+ ASSERT(result == CompilationDeferred || replacementCodeBlock->hasOneRef());
return encodeResult(0, 0);
+ }
}
CodeBlock* optimizedCodeBlock = codeBlock->replacement();
diff --git a/Source/JavaScriptCore/jit/JITToDFGDeferredCompilationCallback.cpp b/Source/JavaScriptCore/jit/JITToDFGDeferredCompilationCallback.cpp
index 69d65a9..4786312 100644
--- a/Source/JavaScriptCore/jit/JITToDFGDeferredCompilationCallback.cpp
+++ b/Source/JavaScriptCore/jit/JITToDFGDeferredCompilationCallback.cpp
@@ -65,6 +65,8 @@
codeBlock->install();
codeBlock->alternative()->setOptimizationThresholdBasedOnCompilationResult(result);
+
+ DeferredCompilationCallback::compilationDidComplete(codeBlock, result);
}
} // JSC
diff --git a/Source/JavaScriptCore/runtime/VM.cpp b/Source/JavaScriptCore/runtime/VM.cpp
index 3ced71c..9e17062 100644
--- a/Source/JavaScriptCore/runtime/VM.cpp
+++ b/Source/JavaScriptCore/runtime/VM.cpp
@@ -512,7 +512,7 @@
interpreter->stopSampling();
}
-void VM::prepareToDiscardCode()
+void VM::waitForCompilationsToComplete()
{
#if ENABLE(DFG_JIT)
for (unsigned i = DFG::numberOfWorklists(); i--;) {
@@ -524,7 +524,7 @@
void VM::discardAllCode()
{
- prepareToDiscardCode();
+ waitForCompilationsToComplete();
m_codeCache->clear();
heap.deleteAllCompiledCode();
heap.reportAbandonedObjectGraph();
@@ -566,7 +566,7 @@
void VM::releaseExecutableMemory()
{
- prepareToDiscardCode();
+ waitForCompilationsToComplete();
if (entryScope) {
StackPreservingRecompiler recompiler;
@@ -872,6 +872,7 @@
{
m_enabledProfiler = profiler;
if (m_enabledProfiler) {
+ waitForCompilationsToComplete();
SetEnabledProfilerFunctor functor;
heap.forEachCodeBlock(functor);
}
diff --git a/Source/JavaScriptCore/runtime/VM.h b/Source/JavaScriptCore/runtime/VM.h
index 066160c..4c0e2e1 100644
--- a/Source/JavaScriptCore/runtime/VM.h
+++ b/Source/JavaScriptCore/runtime/VM.h
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2008, 2009, 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2008, 2009, 2013, 2014 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -498,7 +498,7 @@
JSLock& apiLock() { return *m_apiLock; }
CodeCache* codeCache() { return m_codeCache.get(); }
- void prepareToDiscardCode();
+ void waitForCompilationsToComplete();
JS_EXPORT_PRIVATE void discardAllCode();
