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

Timestamp:

Mar 1, 2022, 10:42:27 PM (3 years ago)

Author:

Devin Rousso

Message:

Web Inspector: add a switch to control whether breakpoint evaluations (condition, ignore count, actions) are also affected by script blackboxing
​https://bugs.webkit.org/show_bug.cgi?id=235274

Reviewed by Patrick Angle.

Source/JavaScriptCore:

Being able to defer breakpoint evaluations until the next actual pause can sometimes be far
more useful than doing the breakpoint evaluations at the breakpoint's original location.

As an example, configuring the All Events breakpoint with a console.trace() action and
auto-continue enabled would not provide much useful information without also blackboxing
breakpoint evaluations as the original pause location of the All Events breakpoint is on the
first line of the event handler, meaning that if the script containing the event handler is
blackboxed then the console.trace() would still only show that location even though the
Sources Tab would show the first line of code outside of that script (due to the blackbox).
Being able to also blackbox breakpoint evaluations would instead cause the console.trace()
action to have the same output as the Sources Tab, since it's evaluation would be deferred
until execution actually paused, which is further inside the event handler.

• debugger/Debugger.cpp:

(JSC::Debugger::Debugger):
(JSC::Debugger::didHitBreakpoint):
(JSC::Debugger::evaluateBreakpointCondition):
(JSC::Debugger::continueProgram):
(JSC::Debugger::pauseIfNeeded):
(JSC::Debugger::didExecuteProgram):
(JSC::Debugger::setBlackboxBreakpointEvaluations): Added.
Keep track of every Breakpoint wants to pause but is deferred due to blackboxing.
Depending on whether breakpoint evaluations are also blackboxed (based on the flag set via
the piping code/logic below), check if the script attempting to be paused in is blackboxed
before or after handling breakpoint evaluations.
Side effects of this change are:

• breakpoint conditions are now evaluated right before that breakpoint's actions, instead of evaluating all breakpoint conditions before all breakpoint actions
• if breakpoint evaluations are blackboxed, more than two breakpoints can now be evaluated

during a single pause attempt depending on how may deferrals (with breakpoints) happened

• debugger/Debugger.h:

(JSC::Debugger::Observer::didDeferBreakpointPause): Added.
Add a way to notify InspectorDebuggerAgent of deferred breakpoint pauses so that the
correct originalReason will be used after a deferred pause.

• inspector/protocol/Debugger.json:
• inspector/agents/InspectorDebuggerAgent.h:
• inspector/agents/InspectorDebuggerAgent.cpp:

(Inspector::InspectorDebuggerAgent::setBlackboxBreakpointEvaluations): Added.
(Inspector::InspectorDebuggerAgent::didDeferBreakpointPause): Added.
Add a setBlackboxBreakpointEvaluations command that passes directly to Debugger.

Source/WebInspectorUI:

Being able to defer breakpoint evaluations until the next actual pause can sometimes be far
more useful than doing the breakpoint evaluations at the breakpoint's original location.

As an example, configuring the All Events breakpoint with a console.trace() action and
auto-continue enabled would not provide much useful information without also blackboxing
breakpoint evaluations as the original pause location of the All Events breakpoint is on the
first line of the event handler, meaning that if the script containing the event handler is
blackboxed then the console.trace() would still only show that location even though the
Sources Tab would show the first line of code outside of that script (due to the blackbox).
Being able to also blackbox breakpoint evaluations would instead cause the console.trace()
action to have the same output as the Sources Tab, since it's evaluation would be deferred
until execution actually paused, which is further inside the event handler

• UserInterface/Base/Setting.js:
• UserInterface/Controllers/DebuggerManager.js:

(WI.DebuggerManager.supportsBlackboxingBreakpointEvaluations): Added.
(WI.DebuggerManager.prototype.async initializeTarget):
(WI.DebuggerManager.prototype._setBlackboxBreakpointEvaluations): Added.
(WI.DebuggerManager.prototype._handleBlackboxBreakpointEvaluationsChange): Added.
Create a global WI.Setting for this feature and handle changes from the code below.

• UserInterface/Views/BlackboxSettingsView.js:

(WI.BlackboxSettingsView.prototype.initialLayout):

• UserInterface/Views/BlackboxSettingsView.css:

(.settings-view.blackbox > p > label): Added.
(.settings-view.blackbox > p > label > input): Added.
(.settings-view.blackbox > p > label > span): Added.
Add a checkbox and explanation of this new feature in the Blackbox pane of the Settings Tab.

• Localizations/en.lproj/localizedStrings.js:

LayoutTests:

• inspector/debugger/setBlackboxBreakpointEvaluations.html: Added.
• inspector/debugger/setBlackboxBreakpointEvaluations-expected.txt: Added.

File:

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

trunk/Source/JavaScriptCore/debugger/Debugger.cpp

@@ -114 +114 @@
 Debugger::Debugger(VM& vm)
     : m_vm(vm)
+    , m_blackboxBreakpointEvaluations(false)
     , m_pauseAtNextOpportunity(false)
     , m_pastFirstExpressionInStatement(false)
@@ -544 +545 @@
 }
 
-RefPtr<Breakpoint> Debugger::didHitBreakpoint(JSGlobalObject* globalObject, SourceID sourceID, const TextPosition& position)
+RefPtr<Breakpoint> Debugger::didHitBreakpoint(SourceID sourceID, const TextPosition& position)
 {
     if (!m_breakpointsActivated)
@@ -566 +567 @@
         // 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) || (line == breakLine && column == breakColumn)) {
-            if (breakpoint->shouldPause(*this, globalObject))
-                return breakpoint.copyRef();
-            break;
-        }
+        if ((line != m_lastExecutedLine && line == breakLine && !breakColumn) || (line == breakLine && column == breakColumn))
+            return breakpoint.copyRef();
     }
 
@@ -607 +605 @@
 bool Debugger::evaluateBreakpointCondition(Breakpoint& breakpoint, JSGlobalObject* globalObject)
 {
+    ASSERT(m_isPaused);
+    ASSERT(isAttached(globalObject));
+
     const String& condition = breakpoint.condition();
     if (condition.isEmpty())
         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);
 
     NakedPtr<Exception> exception;
@@ -782 +779 @@
 {
     clearNextPauseState();
+    m_deferredBreakpoints.clear();
 
     if (!m_isPaused)
@@ -884 +882 @@
     DebuggerPausedScope debuggerPausedScope(*this);
 
-    bool pauseNow = m_pauseAtNextOpportunity;
-    pauseNow |= (m_pauseOnCallFrame == m_currentCallFrame);
-
-    bool didPauseForStep = pauseNow;
+    bool afterBlackboxedScript = m_afterBlackboxedScript;
+    bool pauseNow = false;
+    bool didPauseForStep = false;
+    if (m_pauseAtNextOpportunity) {
+        pauseNow = true;
+        didPauseForStep = !afterBlackboxedScript;
+    } else if (m_pauseOnStepNext || m_pauseOnStepOut || (m_pauseOnCallFrame == m_currentCallFrame)) {
+        pauseNow = true;
+        didPauseForStep = true;
+    }
 
     TextPosition position = DebuggerCallFrame::positionForCallFrame(vm, m_currentCallFrame);
 
-    auto breakpoint = didHitBreakpoint(globalObject, sourceID, position);
-    if (breakpoint)
+    if (auto breakpoint = didHitBreakpoint(sourceID, position)) {
         pauseNow = true;
+        m_deferredBreakpoints.add(breakpoint.releaseNonNull());
+    }
 
     // Special breakpoints are only given one opportunity to pause.
-    auto specialBreakpoint = WTFMove(m_specialBreakpoint);
-    if (specialBreakpoint && specialBreakpoint->shouldPause(*this, globalObject))
+    if (m_specialBreakpoint) {
         pauseNow = true;
+        m_deferredBreakpoints.add(m_specialBreakpoint.releaseNonNull());
+    }
 
     m_lastExecutedLine = position.m_line.zeroBasedInt();
@@ -904 +910 @@
         return;
 
-    bool afterBlackboxedScript = m_afterBlackboxedScript;
     clearNextPauseState();
 
@@ -911 +916 @@
     TemporaryPausedState pausedState(*this);
 
-    if (breakpoint || specialBreakpoint) {
-        // 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.
-
-        bool autoContinue = false;
-
-        if (breakpoint) {
-            evaluateBreakpointActions(*breakpoint, globalObject);
-
+    auto shouldDeferPause = [&] () {
+        if (blackboxTypeIterator == m_blackboxedScripts.end())
+            return false;
+
+        if (blackboxTypeIterator->value != BlackboxType::Deferred)
+            return false;
+
+        m_afterBlackboxedScript = true;
+
+        if (m_pausingBreakpointID != noBreakpointID) {
+            dispatchFunctionToObservers([&] (Observer& observer) {
+                observer.didDeferBreakpointPause(m_pausingBreakpointID);
+            });
+
+            m_pausingBreakpointID = noBreakpointID;
+        }
+
+        schedulePauseAtNextOpportunity();
+        return true;
+    };
+
+    if (m_blackboxBreakpointEvaluations && shouldDeferPause())
+        return;
+
+    if (!m_deferredBreakpoints.isEmpty()) {
+        std::optional<BreakpointID> pausingBreakpointID;
+        bool hasEvaluatedSpecialBreakpoint = false;
+        bool shouldContinue = true;
+
+        for (auto&& deferredBreakpoint : std::exchange(m_deferredBreakpoints, { })) {
+            // Note that breakpoint evaluations can potentially stop the debugger, so we need to
+            // check that we still have a current call frame after evaluating them.
+
+            bool shouldPause = deferredBreakpoint->shouldPause(*this, globalObject);
             if (!m_currentCallFrame)
                 return;
-
-            if (breakpoint->isAutoContinue())
-                autoContinue = true;
-        }
-
-        if (specialBreakpoint) {
-            evaluateBreakpointActions(*specialBreakpoint, globalObject);
-
+            if (!shouldPause)
+                continue;
+
+            evaluateBreakpointActions(deferredBreakpoint, globalObject);
             if (!m_currentCallFrame)
                 return;
 
-            if (specialBreakpoint->isAutoContinue())
-                autoContinue = true;
+            if (deferredBreakpoint->isAutoContinue())
+                continue;
+
+            shouldContinue = false;
+
+            // Only propagate `PausedForBreakpoint` to the `InspectorDebuggerAgent` if the first
+            // line:column breakpoint hit was before the first special breakpoint, as the latter
+            // would already have set a unique reason before attempting to pause.
+            if (!deferredBreakpoint->isLinked())
+                hasEvaluatedSpecialBreakpoint = true;
+            else if (!hasEvaluatedSpecialBreakpoint && !pausingBreakpointID)
+                pausingBreakpointID = deferredBreakpoint->id();
         }
 
-        if (autoContinue) {
+        if (shouldContinue) {
             if (!didPauseForStep)
                 return;
-
-            breakpoint = nullptr;
-            specialBreakpoint = nullptr;
-        } else if (breakpoint)
-            m_pausingBreakpointID = breakpoint->id();
-    }
-
-    if (blackboxTypeIterator != m_blackboxedScripts.end() && blackboxTypeIterator->value == BlackboxType::Deferred) {
-        m_afterBlackboxedScript = true;
-        schedulePauseAtNextOpportunity();
-        return;
-    }
+        } else if (pausingBreakpointID)
+            m_pausingBreakpointID = *pausingBreakpointID;
+    }
+
+    if (!m_blackboxBreakpointEvaluations && shouldDeferPause())
+        return;
 
     {
@@ -957 +987 @@
         if (afterBlackboxedScript)
             reason = PausedAfterBlackboxedScript;
-        else if (breakpoint)
+        else if (m_pausingBreakpointID)
             reason = PausedForBreakpoint;
         PauseReasonDeclaration rauseReasonDeclaration(*this, reason);
@@ -1155 +1185 @@
 
     // Do not continue stepping into an unknown future program.
-    if (!m_currentCallFrame)
+    if (!m_currentCallFrame) {
         clearNextPauseState();
+        m_deferredBreakpoints.clear();
+    }
 }
 
@@ -1212 +1244 @@
 }
 
+void Debugger::setBlackboxBreakpointEvaluations(bool blackboxBreakpointEvaluations)
+{
+    m_blackboxBreakpointEvaluations = blackboxBreakpointEvaluations;
+}
+
 void Debugger::clearBlackbox()
 {