Changeset 159110 in webkit


Ignore:
Timestamp:
Nov 12, 2013 7:41:55 AM (10 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.
Location:
trunk
Files:
12 added
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r159107 r159110  
     12013-11-12  Alexandru Chiculita  <achicu@adobe.com>
     2
     3        Web Inspector: Crash when closing the Inspector while debugging an exception inside a breakpoint condition.
     4        https://bugs.webkit.org/show_bug.cgi?id=124078
     5
     6        Reviewed by Joseph Pecoraro.
     7
     8        Checking that the debugger will not crash nor stall when exceptions are throw while the debugger
     9        is already paused. The cases when that can happen include breakpoint conditions, actions, eval
     10        or runtime object inspection.
     11
     12        The current behavior was to ignore the exceptions or breakpoints while executing "console commands"
     13        when the debugger was already paused. I'm extending this mechanism to breakpoint conditions and
     14        actions as the Debugger is not designed to support nested "debugger breaks".
     15
     16        * http/tests/inspector-protocol/resources/protocol-test.js:
     17        (closeTest): Avoid having internals.closeDummyInspectorFrontend and testRunner.notifyDone
     18        in the same function. The debugger will not have a chance to exit the temporary EventLoop
     19        before loading the next test.
     20        * inspector-protocol/debugger/breakpoint-action-detach-expected.txt: Added.
     21        * inspector-protocol/debugger/breakpoint-action-detach.html: Added.
     22        * inspector-protocol/debugger/breakpoint-action-with-exception-expected.txt: Added.
     23        * inspector-protocol/debugger/breakpoint-action-with-exception.html: Added.
     24        * inspector-protocol/debugger/breakpoint-condition-detach-expected.txt: Added.
     25        * inspector-protocol/debugger/breakpoint-condition-detach.html: Added.
     26        * inspector-protocol/debugger/breakpoint-condition-with-exception-expected.txt: Added.
     27        * inspector-protocol/debugger/breakpoint-condition-with-exception.html: Added.
     28        * inspector-protocol/debugger/breakpoint-eval-with-exception-expected.txt: Added.
     29        * inspector-protocol/debugger/breakpoint-eval-with-exception.html: Added.
     30        * inspector-protocol/debugger/breakpoint-inside-conditons-and-actions-expected.txt: Added.
     31        * inspector-protocol/debugger/breakpoint-inside-conditons-and-actions.html: Added.
     32
    1332013-11-12  Mario Sanchez Prada  <mario.prada@samsung.com>
    234

  • trunk/LayoutTests/http/tests/inspector-protocol/resources/protocol-test.js

    r154829 r159110  
    5959{
    6060    window.internals.closeDummyInspectorFrontend();
    61     testRunner.notifyDone();
     61    // This code might be executed while the debugger is still running through a stack based EventLoop.
     62    // Use a setTimeout to defer to a clean stack before letting the testRunner load the next test.
     63    setTimeout(function() {
     64        testRunner.notifyDone();
     65    }, 0);
    6266}
    6367

  • trunk/Source/JavaScriptCore/ChangeLog

    r159099 r159110  
     12013-11-12  Alexandru Chiculita  <achicu@adobe.com>
     2
     3        Web Inspector: Crash when closing the Inspector while debugging an exception inside a breakpoint condition.
     4        https://bugs.webkit.org/show_bug.cgi?id=124078
     5
     6        Reviewed by Joseph Pecoraro.
     7
     8        The crash would happen because the Debugger is not designed to support nested
     9        breaks. For example, when the debugger handles a breakpoint and the Inspector
     10        executes a console command that would hit the breakpoint again, the Debugger
     11        will just ignore the breakpoint.
     12
     13        There were no checks for conditions and actions. Because of that conditions and actions
     14        could trigger exceptions and breakpoints. This patch disables that functionality as it
     15        cannot be supported without a bigger rewrite of the code.
     16
     17        * debugger/Debugger.cpp:
     18        (JSC::TemporaryPausedState::TemporaryPausedState):
     19        (JSC::TemporaryPausedState::~TemporaryPausedState):
     20        (JSC::Debugger::hasBreakpoint):
     21        (JSC::Debugger::pauseIfNeeded):
     22        * debugger/Debugger.h:
     23
    1242013-11-12  Julien Brianceau  <jbriance@cisco.com>
    225

  • trunk/Source/JavaScriptCore/debugger/Debugger.cpp

    r158937 r159110  
    116116};
    117117
     118// This is very similar to TemporaryChange<bool>, but that cannot be used
     119// as the m_isPaused field uses only one bit.
     120class TemporaryPausedState {
     121public:
     122    TemporaryPausedState(Debugger& debugger)
     123        : m_debugger(debugger)
     124    {
     125        ASSERT(!m_debugger.m_isPaused);
     126        m_debugger.m_isPaused = true;
     127    }
     128
     129    ~TemporaryPausedState()
     130    {
     131        m_debugger.m_isPaused = false;
     132    }
     133
     134private:
     135    Debugger& m_debugger;
     136};
    118137
    119138Debugger::Debugger(bool isInWorkerThread)
    … …  
    265284}
    266285
    267 bool Debugger::hasBreakpoint(SourceID sourceID, const TextPosition& position, Breakpoint *hitBreakpoint) const
     286bool Debugger::hasBreakpoint(SourceID sourceID, const TextPosition& position, Breakpoint *hitBreakpoint)
    268287{
    269288    if (!m_breakpointsActivated)
    … …  
    304323        return true;
    305324
     325    // We cannot stop in the debugger while executing condition code,
     326    // so make it looks like the debugger is already paused.
     327    TemporaryPausedState pausedState(*this);
     328
    306329    JSValue exception;
    307330    JSValue result = DebuggerCallFrame::evaluateWithCallFrame(m_currentCallFrame, breakpoints[i].condition, exception);
     331
     332    // We can lose the debugger while executing JavaScript.
     333    if (!m_currentCallFrame)
     334        return false;
     335
    308336    if (exception) {
    309337        // An erroneous condition counts as "false".
    … …  
    311339        return false;
    312340    }
     341
    313342    return result.toBoolean(m_currentCallFrame);
    314343}
    … …  
    428457    DebuggerCallFrameScope debuggerCallFrameScope(*this);
    429458
     459    // Make sure we are not going to pause again on breakpoint actions by
     460    // reseting the pause state before executing any breakpoint actions.
     461    TemporaryPausedState pausedState(*this);
     462    m_pauseOnCallFrame = 0;
     463    m_pauseOnNextStatement = false;
     464
    430465    if (didHitBreakpoint) {
    431466        handleBreakpointHit(breakpoint);
    432         if (breakpoint.autoContinue)
     467        // Note that the actions can potentially stop the debugger, so we need to check that
     468        // we still have a current call frame when we get back.
     469        if (breakpoint.autoContinue || !m_currentCallFrame)
    433470            return;
    434471    }
    435 
    436     m_pauseOnCallFrame = 0;
    437     m_pauseOnNextStatement = false;
    438     m_isPaused = true;
    439472
    440473    handlePause(m_reasonForPause, dynamicGlobalObject);
    … …  
    445478            m_currentCallFrame = 0;
    446479    }
    447 
    448     m_isPaused = false;
    449480}
    450481

  • trunk/Source/JavaScriptCore/debugger/Debugger.h

    r158937 r159110  
    145145    };
    146146
    147     bool hasBreakpoint(SourceID, const TextPosition&, Breakpoint* hitBreakpoint) const;
     147    bool hasBreakpoint(SourceID, const TextPosition&, Breakpoint* hitBreakpoint);
    148148
    149149    bool shouldPause() const { return m_shouldPause; }
    … …  
    186186
    187187    friend class DebuggerCallFrameScope;
     188    friend class TemporaryPausedState;
    188189    friend class LLIntOffsetsExtractor;
    189190};
Note: See TracChangeset for help on using the changeset viewer.