Changeset 159110 in webkit for trunk/Source/JavaScriptCore/debugger/Debugger.cpp

Timestamp:

Nov 12, 2013, 7:41:55 AM (12 years ago)

Author:

Alexandru Chiculita

Message:

Web Inspector: Crash when closing the Inspector while debugging an exception inside a breakpoint condition.
​https://bugs.webkit.org/show_bug.cgi?id=124078

Reviewed by Joseph Pecoraro.

Source/JavaScriptCore:

The crash would happen because the Debugger is not designed to support nested
breaks. For example, when the debugger handles a breakpoint and the Inspector
executes a console command that would hit the breakpoint again, the Debugger
will just ignore the breakpoint.

There were no checks for conditions and actions. Because of that conditions and actions
could trigger exceptions and breakpoints. This patch disables that functionality as it
cannot be supported without a bigger rewrite of the code.

• debugger/Debugger.cpp:

(JSC::TemporaryPausedState::TemporaryPausedState):
(JSC::TemporaryPausedState::~TemporaryPausedState):
(JSC::Debugger::hasBreakpoint):
(JSC::Debugger::pauseIfNeeded):

• debugger/Debugger.h:

LayoutTests:

Checking that the debugger will not crash nor stall when exceptions are throw while the debugger
is already paused. The cases when that can happen include breakpoint conditions, actions, eval
or runtime object inspection.

The current behavior was to ignore the exceptions or breakpoints while executing "console commands"
when the debugger was already paused. I'm extending this mechanism to breakpoint conditions and
actions as the Debugger is not designed to support nested "debugger breaks".

• http/tests/inspector-protocol/resources/protocol-test.js:

(closeTest): Avoid having internals.closeDummyInspectorFrontend and testRunner.notifyDone
in the same function. The debugger will not have a chance to exit the temporary EventLoop
before loading the next test.

• inspector-protocol/debugger/breakpoint-action-detach-expected.txt: Added.
• inspector-protocol/debugger/breakpoint-action-detach.html: Added.
• inspector-protocol/debugger/breakpoint-action-with-exception-expected.txt: Added.
• inspector-protocol/debugger/breakpoint-action-with-exception.html: Added.
• inspector-protocol/debugger/breakpoint-condition-detach-expected.txt: Added.
• inspector-protocol/debugger/breakpoint-condition-detach.html: Added.
• inspector-protocol/debugger/breakpoint-condition-with-exception-expected.txt: Added.
• inspector-protocol/debugger/breakpoint-condition-with-exception.html: Added.
• inspector-protocol/debugger/breakpoint-eval-with-exception-expected.txt: Added.
• inspector-protocol/debugger/breakpoint-eval-with-exception.html: Added.
• inspector-protocol/debugger/breakpoint-inside-conditons-and-actions-expected.txt: Added.
• inspector-protocol/debugger/breakpoint-inside-conditons-and-actions.html: Added.

File:

• trunk/Source/JavaScriptCore/debugger/Debugger.cpp (modified) (6 diffs)

trunk/Source/JavaScriptCore/debugger/Debugger.cpp

@@ -116 +116 @@
 };
 
+// This is very similar to TemporaryChange<bool>, but that cannot be used
+// as the m_isPaused field uses only one bit.
+class TemporaryPausedState {
+public:
+    TemporaryPausedState(Debugger& debugger)
+        : m_debugger(debugger)
+    {
+        ASSERT(!m_debugger.m_isPaused);
+        m_debugger.m_isPaused = true;
+    }
+
+    ~TemporaryPausedState()
+    {
+        m_debugger.m_isPaused = false;
+    }
+
+private:
+    Debugger& m_debugger;
+};
 
 Debugger::Debugger(bool isInWorkerThread)
@@ -265 +284 @@
 }
 
-bool Debugger::hasBreakpoint(SourceID sourceID, const TextPosition& position, Breakpoint *hitBreakpoint) const
+bool Debugger::hasBreakpoint(SourceID sourceID, const TextPosition& position, Breakpoint *hitBreakpoint)
 {
     if (!m_breakpointsActivated)
@@ -304 +323 @@
         return true;
 
+    // We cannot stop in the debugger while executing condition code,
+    // so make it looks like the debugger is already paused.
+    TemporaryPausedState pausedState(*this);
+
     JSValue exception;
     JSValue result = DebuggerCallFrame::evaluateWithCallFrame(m_currentCallFrame, breakpoints[i].condition, exception);
+
+    // We can lose the debugger while executing JavaScript.
+    if (!m_currentCallFrame)
+        return false;
+
     if (exception) {
         // An erroneous condition counts as "false".
@@ -311 +339 @@
         return false;
     }
+
     return result.toBoolean(m_currentCallFrame);
 }
@@ -428 +457 @@
     DebuggerCallFrameScope debuggerCallFrameScope(*this);
 
+    // Make sure we are not going to pause again on breakpoint actions by
+    // reseting the pause state before executing any breakpoint actions.
+    TemporaryPausedState pausedState(*this);
+    m_pauseOnCallFrame = 0;
+    m_pauseOnNextStatement = false;
+
     if (didHitBreakpoint) {
         handleBreakpointHit(breakpoint);
-        if (breakpoint.autoContinue)
+        // Note that the actions can potentially stop the debugger, so we need to check that
+        // we still have a current call frame when we get back.
+        if (breakpoint.autoContinue || !m_currentCallFrame)
             return;
     }
-
-    m_pauseOnCallFrame = 0;
-    m_pauseOnNextStatement = false;
-    m_isPaused = true;
 
     handlePause(m_reasonForPause, dynamicGlobalObject);
@@ -445 +478 @@
             m_currentCallFrame = 0;
     }
-
-    m_isPaused = false;
 }