Changeset 252730 in webkit

Timestamp:

Nov 21, 2019, 4:14:59 AM (7 years ago)

Author:

Devin Rousso

Message:

Web Inspector: removing the blackbox for a specific script doesn't actually remove the blackbox
​https://bugs.webkit.org/show_bug.cgi?id=204428

Reviewed by Timothy Hatcher.

Source/JavaScriptCore:

Previously, when updating the blackbox state of each existing script, we would only tell the
Debugger about when scripts should be blackboxed, not when they shouldn't. This means that
when a given script is un-blackboxed, the Debugger would never get told about it and would
therefore still defer pauses as if it was blackboxed.

The solution to this is simple; update the blackboxed state of every script, not just those
that should be blackboxed, and tell the Debugger about each.

• inspector/agents/InspectorDebuggerAgent.cpp:

(Inspector::InspectorDebuggerAgent::setShouldBlackboxURL):

LayoutTests:

• inspector/debugger/setShouldBlackboxURL.html:
• inspector/debugger/setShouldBlackboxURL-expected.txt:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/inspector/debugger/setShouldBlackboxURL-expected.txt (modified) (1 diff)
• LayoutTests/inspector/debugger/setShouldBlackboxURL.html (modified) (1 diff)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2019-11-21  Devin Rousso  <drousso@apple.com>
+
+        Web Inspector: removing the blackbox for a specific script doesn't actually remove the blackbox
+        https://bugs.webkit.org/show_bug.cgi?id=204428
+
+        Reviewed by Timothy Hatcher.
+
+        * inspector/debugger/setShouldBlackboxURL.html:
+        * inspector/debugger/setShouldBlackboxURL-expected.txt:
+
 2019-11-20  Simon Fraser  <simon.fraser@apple.com>
 

trunk/LayoutTests/inspector/debugger/setShouldBlackboxURL-expected.txt

@@ -301 +301 @@
 PASS: Should pause in 'CaseSensitiveRegex_PauseInCallee_Middle'.
 PASS: Should not pause in 'CaseSensitiveRegex_PauseInCallee_Outer'.
+
+
+-- Running test case: Debugger.setShouldBlackboxURL.Toggle
+Evaluating 'createScripts("Toggle")'...
+Setting breakpoint in 'Toggle_Inner.js'...
+
+Blackboxing 'toggle_middle.js'...
+Evaluating 'Toggle_Outer(10)'...
+
+PAUSED: 'Breakpoint' at 'Toggle_Inner:3:1'.
+{
+  "breakpointId": "Toggle_Inner.js:3:0"
+}
+Stepping over...
+
+PAUSED: 'BlackboxedScript' at 'Toggle_Outer:3:1'.
+{
+  "originalReason": "other",
+  "originalData": {
+    "breakpointId": "Toggle_Inner.js:3:0"
+  }
+}
+Stepping over...
+
+Resuming...
+PASS: Resumed.
+PASS: Should not pause in 'Toggle_Middle'.
+
+Removing blackbox for 'toggle_middle.js'...
+Evaluating 'Toggle_Outer(10)'...
+
+PAUSED: 'Breakpoint' at 'Toggle_Inner:3:1'.
+{
+  "breakpointId": "Toggle_Inner.js:3:0"
+}
+Stepping over...
+
+PAUSED: 'other' at 'Toggle_Middle:3:1'.
+Stepping over...
+
+PAUSED: 'other' at 'Toggle_Outer:3:1'.
+Stepping over...
+
+Resuming...
+PASS: Resumed.
+PASS: Should pause in 'Toggle_Middle'.
 
 

trunk/LayoutTests/inspector/debugger/setShouldBlackboxURL.html

@@ -461 +461 @@
 
     suite.addTestCase({
+        name: "Debugger.setShouldBlackboxURL.Toggle",
+        description: "Check that the URL does not remain blackboxed if it's blackboxed state is toggled on and then off.",
+        async test() {
+            let resumePromise = null;
+
+            let [innerSourceURL, middleSourceURL, outerSourceURL] = await Promise.all([
+                listenForSourceParsed(/Toggle_Inner\.js$/),
+                listenForSourceParsed(/Toggle_Middle\.js$/),
+                listenForSourceParsed(/Toggle_Outer\.js$/),
+                evaluate(`createScripts("Toggle")`),
+            ]);
+            await setBreakpoint(innerSourceURL, 3); // last line of function, so it only pauses once
+
+            ProtocolTest.newline();
+
+            resumePromise = new Promise((resolve, reject) => {
+                resumeCallback = function() {
+                    ProtocolTest.expectThat(!pausedFunctionNames.includes("Toggle_Middle"), "Should not pause in 'Toggle_Middle'.");
+                    resolve();
+                };
+            });
+
+            await setBlackbox(middleSourceURL);
+            await evaluate(`Toggle_Outer(10)`);
+
+            ProtocolTest.newline();
+
+            await resumePromise;
+
+            resumePromise = new Promise((resolve, reject) => {
+                resumeCallback = function() {
+                    ProtocolTest.expectThat(pausedFunctionNames.includes("Toggle_Middle"), "Should pause in 'Toggle_Middle'.");
+                    resolve();
+                };
+            });
+
+            ProtocolTest.log(`Removing blackbox for '${middleSourceURL.toLowerCase()}'...`);
+            await InspectorProtocol.awaitCommand({
+                method: "Debugger.setShouldBlackboxURL",
+                params: {url: middleSourceURL.toLowerCase(), shouldBlackbox: false},
+            });
+
+            await evaluate(`Toggle_Outer(10)`);
+
+            ProtocolTest.newline();
+
+            await resumePromise;
+        },
+    });
+
+    suite.addTestCase({
         name: "Debugger.setShouldBlackboxURL.Invalid.emptyURL",
         description: "Check that an error is thrown if the given url is empty.",

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-11-21  Devin Rousso  <drousso@apple.com>
+
+        Web Inspector: removing the blackbox for a specific script doesn't actually remove the blackbox
+        https://bugs.webkit.org/show_bug.cgi?id=204428
+
+        Reviewed by Timothy Hatcher.
+
+        Previously, when updating the blackbox state of each existing script, we would only tell the
+        `Debugger` about when scripts should be blackboxed, not when they shouldn't. This means that
+        when a given script is un-blackboxed, the `Debugger` would never get told about it and would
+        therefore still defer pauses as if it was blackboxed.
+
+        The solution to this is simple; update the blackboxed state of every script, not just those
+        that should be blackboxed, and tell the `Debugger` about each.
+
+        * inspector/agents/InspectorDebuggerAgent.cpp:
+        (Inspector::InspectorDebuggerAgent::setShouldBlackboxURL):
+
 2019-11-20  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp

@@ -912 +912 @@
         m_blackboxedURLs.removeAll(config);
 
-    auto blackboxType = shouldBlackbox ? Optional<JSC::Debugger::BlackboxType>(JSC::Debugger::BlackboxType::Deferred) : WTF::nullopt;
     for (auto& [sourceID, script] : m_scripts) {
         if (isWebKitInjectedScript(script.sourceURL))
             continue;
-        if (!shouldBlackboxURL(script.sourceURL) && !shouldBlackboxURL(script.url))
-            continue;
+
+        Optional<JSC::Debugger::BlackboxType> blackboxType;
+        if (shouldBlackboxURL(script.sourceURL) || shouldBlackboxURL(script.url))
+            blackboxType = JSC::Debugger::BlackboxType::Deferred;
         m_scriptDebugServer.setBlackboxType(sourceID, blackboxType);
     }