- Revision
- 170677
- Author
- mark....@apple.com
- Date
- 2014-07-01 16:40:32 -0700 (Tue, 01 Jul 2014)
Log Message
Debugger's breakpoint list should not be a Vector.
<https://webkit.org/b/134514>
Reviewed by Geoffrey Garen.
The debugger currently stores breakpoint data as entries in a Vector (see
BreakpointsInLine). It also keeps a fast map look up of breakpoint IDs to
the breakpoint data (see m_breakpointIDToBreakpoint). Because a Vector can
compact or reallocate its backing store, this can causes all sorts of havoc.
The m_breakpointIDToBreakpoint map assumes that the breakpoint data doesn't
move in memory.
The fix is to replace the BreakpointsInLine Vector with a BreakpointsList
doubly linked list.
* debugger/Breakpoint.h:
(JSC::Breakpoint::Breakpoint):
(JSC::BreakpointsList::~BreakpointsList):
* debugger/Debugger.cpp:
(JSC::Debugger::setBreakpoint):
(JSC::Debugger::removeBreakpoint):
(JSC::Debugger::hasBreakpoint):
* debugger/Debugger.h:
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (170676 => 170677)
--- trunk/Source/_javascript_Core/ChangeLog 2014-07-01 23:40:06 UTC (rev 170676)
+++ trunk/Source/_javascript_Core/ChangeLog 2014-07-01 23:40:32 UTC (rev 170677)
@@ -1,3 +1,29 @@
+2014-07-01 Mark Lam <mark....@apple.com>
+
+ Debugger's breakpoint list should not be a Vector.
+ <https://webkit.org/b/134514>
+
+ Reviewed by Geoffrey Garen.
+
+ The debugger currently stores breakpoint data as entries in a Vector (see
+ BreakpointsInLine). It also keeps a fast map look up of breakpoint IDs to
+ the breakpoint data (see m_breakpointIDToBreakpoint). Because a Vector can
+ compact or reallocate its backing store, this can causes all sorts of havoc.
+ The m_breakpointIDToBreakpoint map assumes that the breakpoint data doesn't
+ move in memory.
+
+ The fix is to replace the BreakpointsInLine Vector with a BreakpointsList
+ doubly linked list.
+
+ * debugger/Breakpoint.h:
+ (JSC::Breakpoint::Breakpoint):
+ (JSC::BreakpointsList::~BreakpointsList):
+ * debugger/Debugger.cpp:
+ (JSC::Debugger::setBreakpoint):
+ (JSC::Debugger::removeBreakpoint):
+ (JSC::Debugger::hasBreakpoint):
+ * debugger/Debugger.h:
+
2014-06-30 Michael Saboff <msab...@apple.com>
Add option to run-jsc-stress-testes to filter out tests that use large heaps
Modified: trunk/Source/_javascript_Core/debugger/Breakpoint.h (170676 => 170677)
--- trunk/Source/_javascript_Core/debugger/Breakpoint.h 2014-07-01 23:40:06 UTC (rev 170676)
+++ trunk/Source/_javascript_Core/debugger/Breakpoint.h 2014-07-01 23:40:32 UTC (rev 170677)
@@ -27,11 +27,13 @@
#define Breakpoint_h
#include "DebuggerPrimitives.h"
+#include <wtf/DoublyLinkedList.h>
+#include <wtf/RefCounted.h>
#include <wtf/text/WTFString.h>
namespace JSC {
-struct Breakpoint {
+struct Breakpoint : public DoublyLinkedListNode<Breakpoint> {
Breakpoint()
: id(noBreakpointID)
, sourceID(noSourceID)
@@ -51,6 +53,16 @@
{
}
+ Breakpoint(const Breakpoint& other)
+ : id(other.id)
+ , sourceID(other.sourceID)
+ , line(other.line)
+ , column(other.column)
+ , condition(other.condition)
+ , autoContinue(other.autoContinue)
+ {
+ }
+
BreakpointID id;
SourceID sourceID;
unsigned line;
@@ -59,8 +71,26 @@
bool autoContinue;
static const unsigned unspecifiedColumn = UINT_MAX;
+
+private:
+ Breakpoint* m_prev;
+ Breakpoint* m_next;
+
+ friend class WTF::DoublyLinkedListNode<Breakpoint>;
};
+class BreakpointsList : public DoublyLinkedList<Breakpoint>,
+ public RefCounted<BreakpointsList> {
+public:
+ ~BreakpointsList()
+ {
+ Breakpoint* breakpoint;
+ while ((breakpoint = removeHead()))
+ delete breakpoint;
+ ASSERT(isEmpty());
+ }
+};
+
} // namespace JSC
#endif // Breakpoint_h
Modified: trunk/Source/_javascript_Core/debugger/Debugger.cpp (170676 => 170677)
--- trunk/Source/_javascript_Core/debugger/Debugger.cpp 2014-07-01 23:40:06 UTC (rev 170676)
+++ trunk/Source/_javascript_Core/debugger/Debugger.cpp 2014-07-01 23:40:32 UTC (rev 170677)
@@ -357,18 +357,18 @@
it = m_sourceIDToBreakpoints.set(sourceID, LineToBreakpointsMap()).iterator;
LineToBreakpointsMap::iterator breaksIt = it->value.find(line);
if (breaksIt == it->value.end())
- breaksIt = it->value.set(line, BreakpointsInLine()).iterator;
+ breaksIt = it->value.set(line, adoptRef(new BreakpointsList)).iterator;
- BreakpointsInLine& breakpoints = breaksIt->value;
- unsigned breakpointsCount = breakpoints.size();
- for (unsigned i = 0; i < breakpointsCount; i++)
- if (breakpoints[i].column == column) {
+ BreakpointsList& breakpoints = *breaksIt->value;
+ for (Breakpoint* current = breakpoints.head(); current; current = current->next()) {
+ if (current->column == column) {
// The breakpoint already exists. We're not allowed to create a new
// breakpoint at this location. Rather than returning the breakpointID
// of the pre-existing breakpoint, we need to return noBreakpointID
// to indicate that we're not creating a new one.
return noBreakpointID;
}
+ }
BreakpointID id = ++m_topBreakpointID;
RELEASE_ASSERT(id != noBreakpointID);
@@ -377,8 +377,9 @@
actualLine = line;
actualColumn = column;
- breakpoints.append(breakpoint);
- m_breakpointIDToBreakpoint.set(id, &breakpoints.last());
+ Breakpoint* newBreakpoint = new Breakpoint(breakpoint);
+ breakpoints.append(newBreakpoint);
+ m_breakpointIDToBreakpoint.set(id, newBreakpoint);
toggleBreakpoint(breakpoint, BreakpointEnabled);
@@ -391,31 +392,35 @@
BreakpointIDToBreakpointMap::iterator idIt = m_breakpointIDToBreakpoint.find(id);
ASSERT(idIt != m_breakpointIDToBreakpoint.end());
- Breakpoint& breakpoint = *idIt->value;
+ Breakpoint* breakpoint = idIt->value;
- SourceID sourceID = breakpoint.sourceID;
+ SourceID sourceID = breakpoint->sourceID;
ASSERT(sourceID);
SourceIDToBreakpointsMap::iterator it = m_sourceIDToBreakpoints.find(sourceID);
ASSERT(it != m_sourceIDToBreakpoints.end());
- LineToBreakpointsMap::iterator breaksIt = it->value.find(breakpoint.line);
+ LineToBreakpointsMap::iterator breaksIt = it->value.find(breakpoint->line);
ASSERT(breaksIt != it->value.end());
- toggleBreakpoint(breakpoint, BreakpointDisabled);
+ toggleBreakpoint(*breakpoint, BreakpointDisabled);
- BreakpointsInLine& breakpoints = breaksIt->value;
- unsigned breakpointsCount = breakpoints.size();
- for (unsigned i = 0; i < breakpointsCount; i++) {
- if (breakpoints[i].id == breakpoint.id) {
- breakpoints.remove(i);
- m_breakpointIDToBreakpoint.remove(idIt);
+ BreakpointsList& breakpoints = *breaksIt->value;
+#if !ASSERT_DISABLED
+ bool found = false;
+ for (Breakpoint* current = breakpoints.head(); current && !found; current = current->next()) {
+ if (current->id == breakpoint->id)
+ found = true;
+ }
+ ASSERT(found);
+#endif
- if (breakpoints.isEmpty()) {
- it->value.remove(breaksIt);
- if (it->value.isEmpty())
- m_sourceIDToBreakpoints.remove(it);
- }
- break;
- }
+ m_breakpointIDToBreakpoint.remove(idIt);
+ breakpoints.remove(breakpoint);
+ delete breakpoint;
+
+ if (breakpoints.isEmpty()) {
+ it->value.remove(breaksIt);
+ if (it->value.isEmpty())
+ m_sourceIDToBreakpoints.remove(it);
}
}
@@ -436,12 +441,11 @@
return false;
bool hit = false;
- const BreakpointsInLine& breakpoints = breaksIt->value;
- unsigned breakpointsCount = breakpoints.size();
- unsigned i;
- for (i = 0; i < breakpointsCount; i++) {
- unsigned breakLine = breakpoints[i].line;
- unsigned breakColumn = breakpoints[i].column;
+ const BreakpointsList& breakpoints = *breaksIt->value;
+ Breakpoint* breakpoint;
+ for (breakpoint = breakpoints.head(); breakpoint; breakpoint = breakpoint->next()) {
+ unsigned breakLine = breakpoint->line;
+ unsigned breakColumn = breakpoint->column;
// Since frontend truncates the indent, the first statement in a line must match the breakpoint (line,0).
ASSERT(this == m_currentCallFrame->codeBlock()->globalObject()->debugger());
if ((line != m_lastExecutedLine && line == breakLine && !breakColumn)
@@ -454,9 +458,9 @@
return false;
if (hitBreakpoint)
- *hitBreakpoint = breakpoints[i];
+ *hitBreakpoint = *breakpoint;
- if (breakpoints[i].condition.isEmpty())
+ if (breakpoint->condition.isEmpty())
return true;
// We cannot stop in the debugger while executing condition code,
@@ -465,7 +469,7 @@
JSValue exception;
DebuggerCallFrame* debuggerCallFrame = currentDebuggerCallFrame();
- JSValue result = debuggerCallFrame->evaluate(breakpoints[i].condition, exception);
+ JSValue result = debuggerCallFrame->evaluate(breakpoint->condition, exception);
// We can lose the debugger while executing _javascript_.
if (!m_currentCallFrame)
Modified: trunk/Source/_javascript_Core/debugger/Debugger.h (170676 => 170677)
--- trunk/Source/_javascript_Core/debugger/Debugger.h 2014-07-01 23:40:06 UTC (rev 170676)
+++ trunk/Source/_javascript_Core/debugger/Debugger.h 2014-07-01 23:40:32 UTC (rev 170677)
@@ -29,7 +29,6 @@
#include <wtf/HashMap.h>
#include <wtf/HashSet.h>
#include <wtf/RefPtr.h>
-#include <wtf/Vector.h>
#include <wtf/text/TextPosition.h>
namespace JSC {
@@ -128,8 +127,7 @@
private:
typedef HashMap<BreakpointID, Breakpoint*> BreakpointIDToBreakpointMap;
- typedef Vector<Breakpoint> BreakpointsInLine;
- typedef HashMap<unsigned, BreakpointsInLine, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>> LineToBreakpointsMap;
+ typedef HashMap<unsigned, RefPtr<BreakpointsList>, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>> LineToBreakpointsMap;
typedef HashMap<SourceID, LineToBreakpointsMap, WTF::IntHash<SourceID>, WTF::UnsignedWithZeroKeyHashTraits<SourceID>> SourceIDToBreakpointsMap;
class ClearCodeBlockDebuggerRequestsFunctor;
