The JS call stack can easily become very long and filled with frames you don't really care about if you're using a JS framework on your site (e.g., jQuery, Protoype, etc.). It would be nice if Drosera let you collapse/omit the frames from a specific framework. Maybe we could just show the entry/exits points to/from a particular file and skip any calls within that file.
<rdar://problem/5732847>
Firefox devtools implements this: https://hacks.mozilla.org/2013/08/new-features-of-firefox-developer-tools-episode-25/ It allows you to blackbox JS frameworks, making their call frames omitted or reduced when paused in the debugger.
*** Bug 59119 has been marked as a duplicate of this bug. ***
*** Bug 80383 has been marked as a duplicate of this bug. ***
I added this comment to a bug that was marked as a duplicate of this one: Brian, I worked with Firefox on this feature and opened up a number of subsequent requests for more granular control over the black-boxing. For example, when I'm debugging Ember itself, even though I want to step through Ember source, I still want to jump through our implementation of forEach and the extra function wrapper we need to create for super (which are *never* the source of bugs). My ideal implementation would support whole-file blackboxing, blackboxing based on an identifier provided by the script (//# blackboxId=ember), and finally per-function blackboxing via sourcemaps (or if that's not possible, via dynamic tagging).
We should support source map level blackboxing. Per-function blackboxing is interesting, but might make for a complex UX.
*** Bug 66632 has been marked as a duplicate of this bug. ***
I'm jumping in to suggest folder-level blackboxing or even pattern-based, just as you have in Chrome. With today's JavaScript stacks, it's more convenient to just blackbox everything within node_modules.
Created attachment 372117 [details] Patch I'm going to add support for regex/pattern blackboxing via the Settings tab in a followup. I'll also add more tests when I'm more awake -.-
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Attachment 372117 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:1145: More than one command on the same line [whitespace/newline] [4] Total errors found: 1 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 372117 [details] Patch Attachment 372117 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12472689 New failing tests: inspector/debugger/pause-for-internal-scripts.html
Created attachment 372119 [details] Archive of layout-test-results from ews105 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 372117 [details] Patch Attachment 372117 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12472796 New failing tests: inspector/debugger/pause-for-internal-scripts.html
Created attachment 372120 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 372117 [details] Patch Attachment 372117 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12472831 New failing tests: inspector/debugger/pause-for-internal-scripts.html
Created attachment 372123 [details] Archive of layout-test-results from ews112 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 372154 [details] Patch
Attachment 372154 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:1073: 'data' is incorrectly named. It should be named 'protector' or 'protectedNullptr'. [readability/naming/protected] [4] ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:1179: More than one command on the same line [whitespace/newline] [4] Total errors found: 2 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 372154 [details] Patch Attachment 372154 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12478703 New failing tests: inspector/model/remote-object-api.html inspector/debugger/setShouldBlackboxURL.html inspector/debugger/pause-reason.html
Created attachment 372162 [details] Archive of layout-test-results from ews103 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 372154 [details] Patch Attachment 372154 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12478733 New failing tests: inspector/model/remote-object-api.html inspector/debugger/setShouldBlackboxURL.html inspector/debugger/pause-reason.html
Created attachment 372163 [details] Archive of layout-test-results from ews105 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 372154 [details] Patch Attachment 372154 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12478775 New failing tests: inspector/model/remote-object-api.html inspector/debugger/setShouldBlackboxURL.html inspector/debugger/pause-reason.html
Created attachment 372166 [details] Archive of layout-test-results from ews114 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 372154 [details] Patch Attachment 372154 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12478721 New failing tests: mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases apiTests
Created attachment 372168 [details] Patch
Attachment 372168 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:1073: 'data' is incorrectly named. It should be named 'protector' or 'protectedNullptr'. [readability/naming/protected] [4] ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:1179: More than one command on the same line [whitespace/newline] [4] Total errors found: 2 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 372168 [details] Patch Attachment 372168 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12479619 New failing tests: inspector/model/remote-object-api.html inspector/debugger/pause-reason.html
Created attachment 372172 [details] Archive of layout-test-results from ews105 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 372168 [details] Patch Attachment 372168 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12479640 New failing tests: inspector/model/remote-object-api.html inspector/debugger/pause-reason.html
Created attachment 372176 [details] Archive of layout-test-results from ews112 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 372168 [details] Patch Attachment 372168 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12481058 New failing tests: inspector/model/remote-object-api.html inspector/debugger/pause-reason.html
Created attachment 372189 [details] Archive of layout-test-results from ews103 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 372168 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372168&action=review > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:179 > + return InspectorBackend.domains.Debugger.setShouldBlackboxURL; This should `!!` to not return the actual function. > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:404 > + target.DebuggerAgent.setShouldBlackboxURL(sourceCode.contentIdentifier, !!shouldBlackbox); This should check `if (target.DebuggerAgent.setShouldBlackboxURL)`.
Created attachment 375517 [details] Patch
Attachment 375517 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:1076: 'data' is incorrectly named. It should be named 'protector' or 'protectedNullptr'. [readability/naming/protected] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 375517 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375517&action=review You forgot to include SourceCodeTreeElement.css in your patch, so I can't test it out (rendering is really off). > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:184 > + // FIXME: Handle other backend pause reasons. Like what?
Comment on attachment 375517 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375517&action=review >> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:184 >> + // FIXME: Handle other backend pause reasons. > > Like what? Not sure :P This was moved from below. I'll remove it.
Created attachment 375853 [details] Patch
Attachment 375853 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:1076: 'data' is incorrectly named. It should be named 'protector' or 'protectedNullptr'. [readability/naming/protected] [4] Total errors found: 1 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 375853 [details] Patch Attachment 375853 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12881794 New failing tests: inspector/debugger/setShouldBlackboxURL.html
Created attachment 375869 [details] Archive of layout-test-results from ews102 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 375870 [details] Patch Never in my life have I hated a period as much as this moment -.-
Attachment 375870 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:1076: 'data' is incorrectly named. It should be named 'protector' or 'protectedNullptr'. [readability/naming/protected] [4] Total errors found: 1 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 375870 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375870&action=review > Source/WebInspectorUI/UserInterface/Views/SourceCodeTreeElement.css:29 > + content: url(../Images/Eye.svg); This should use `Hide.svg` from <https://webkit.org/b/200736>.
Comment on attachment 375870 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375870&action=review > Source/JavaScriptCore/debugger/Debugger.cpp:742 > + if (blackboxTypeIterator != m_blackboxedScripts.end() && blackboxTypeIterator->value == BlackboxType::Deferred) { The only type the blackboxTypeIterator value can be at this point is Deferred. So we could assert instead. Up to you though. ASSERT(blackboxTypeIterator->value == BlackboxType::Deferred); > Source/JavaScriptCore/debugger/Debugger.cpp:749 > - PauseReasonDeclaration reason(*this, didHitBreakpoint ? PausedForBreakpoint : m_reasonForPause); > + PauseReasonDeclaration reason(*this, afterBlackboxedScript ? PausedAfterBlackboxedScript : (didHitBreakpoint ? PausedForBreakpoint : m_reasonForPause)); I find double ternaries very hard to read. Could we pull this out? auto pauseReason = m_reasonForPause; if (afterBlackboxedScript) pauseReason = PausedAfterBlackboxedScript; else if (didHitBreakpoint) pauseReason = PausedForBreakpoint; > Source/JavaScriptCore/debugger/Debugger.h:114 > + enum class BlackboxType { For very few values like this I feel it is often better to put it on a single line. It simplifies wasted vertical space and it also helps with code search; searching for "BlackboxType" you get back a single line with all the info you need. > Source/JavaScriptCore/debugger/Debugger.h:246 > + bool m_afterBlackboxedScript { false }; This really didn't read well for me above. Something like `m_steppingThroughBlackboxedScript` seems clearer. > Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:194 > + m_lastBreakReason = m_breakReason; > + m_lastBreakData = WTFMove(m_breakData); These could be named `m_preBlackboxBreakReason` etc given they are not always the "last" value. > Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:894 > + errorString = "Blackboxing of internal scripts is controlled by 'setPauseForInternalScripts'"; Nit: `_s` suffix > Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:952 > + if (m_pauseForInternalScripts) > + m_scriptDebugServer.setBlackboxType(sourceID, WTF::nullopt); > + else > + m_scriptDebugServer.setBlackboxType(sourceID, JSC::Debugger::BlackboxType::Ignored); > + } You could avoid this if by pulling the Blackbox type out of the loop! auto type = m_pauseForInternalScripts ? JSC::Debugger::BlackboxType::Ignored : WTF::nullopt; for (auto& [sourceID, script] : m_scripts) { if (!isWebKitInjectedScript(script.sourceURL)) continue; m_scriptDebugServer.setBlackboxType(sourceId, type); } > Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:1081 > + data->setString("originalReason", Protocol::InspectorHelpers::getEnumConstantValue(DebuggerFrontendDispatcher::Reason::Breakpoint)); > + data->setValue("originalData", buildBreakpointPauseReason(debuggerBreakpointId)); These setter strings could all use `_s` suffix. The `_s` ASCIILiteral path lets us avoid string copies. > Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.h:168 > + void updateBreakReasonAndData(DebuggerFrontendDispatcher::Reason, RefPtr<JSON::Object>&& data); I wonder if now would be a good time to rename all these "break" things to "pause". `updatePauseReasonAndData`, `m_pauseReason`, etc. > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:409 > + this.dispatchEventToListeners(DebuggerManager.Event.BlackboxedURLsChanged); We generally prefer dispatching the event after the work (so after the target messages below). > Source/WebInspectorUI/UserInterface/Test/TestHarness.js:214 > if (e) Style: this `if` should have braces.
Comment on attachment 375870 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375870&action=review >> Source/JavaScriptCore/debugger/Debugger.cpp:742 >> + if (blackboxTypeIterator != m_blackboxedScripts.end() && blackboxTypeIterator->value == BlackboxType::Deferred) { > > The only type the blackboxTypeIterator value can be at this point is Deferred. So we could assert instead. Up to you though. > > ASSERT(blackboxTypeIterator->value == BlackboxType::Deferred); I'd rather not, just in case we add more types in the future (less code would need to be changed). >> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:1081 >> + data->setValue("originalData", buildBreakpointPauseReason(debuggerBreakpointId)); > > These setter strings could all use `_s` suffix. The `_s` ASCIILiteral path lets us avoid string copies. Oops. My mistake. >> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.h:168 >> + void updateBreakReasonAndData(DebuggerFrontendDispatcher::Reason, RefPtr<JSON::Object>&& data); > > I wonder if now would be a good time to rename all these "break" things to "pause". `updatePauseReasonAndData`, `m_pauseReason`, etc. I like it!
Created attachment 377655 [details] Patch
Created attachment 377656 [details] [Image] After Patch is applied
Comment on attachment 377655 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377655&action=review r=me! > Source/WebInspectorUI/UserInterface/Views/SourceCodeTreeElement.js:168 > + WI.debuggerManager.addEventListener(WI.DebuggerManager.Event.BlackboxedURLsChanged, this._updateToggleBlackboxImageElementState, this); It looks like this (updateStatus) can get called multiple times and might add multiple event listeners. I also don't see this event listener getting removed, so these would leak. Perhaps this should create the image element, listener, and set the status once. Then just call update in the general case. > LayoutTests/inspector/debugger/setShouldBlackboxURL.html:63 > + InspectorProtocol.sendCommand(`Debugger.resume`, {}, function(response) { > + InspectorProtocol.checkForError(response); > + }); I think this pattern can be written in a few places like so: InspectorProtocol.sendCommand(`Debugger.resume`, {}, InspectorProtocol.checkForError); Perhaps even making a `InspectorProtocol` convenience: InspectorProtocol.sendCommandAndCheckForError(`Debugger.resume`);
Comment on attachment 377655 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377655&action=review >> Source/WebInspectorUI/UserInterface/Views/SourceCodeTreeElement.js:168 >> + WI.debuggerManager.addEventListener(WI.DebuggerManager.Event.BlackboxedURLsChanged, this._updateToggleBlackboxImageElementState, this); > > It looks like this (updateStatus) can get called multiple times and might add multiple event listeners. > > I also don't see this event listener getting removed, so these would leak. > > Perhaps this should create the image element, listener, and set the status once. Then just call update in the general case. Good catch! I'll rework this a bit to be slightly more smart about when it listens for `WI.DebuggerManager.Event.BlackboxedURLsChanged`. As far as the leak issue, that's not really something that's "supported" with the current design of our tree elements/outlines (e.g. `ondetached` only gets called when that tree element gets removed from its parent, not whenever it's removed from the DOM). I think this is better handled by <https://webkit.org/b/196956>.
Created attachment 377935 [details] Patch
Comment on attachment 377935 [details] Patch Clearing flags on attachment: 377935 Committed r249450: <https://trac.webkit.org/changeset/249450>
All reviewed patches have been landed. Closing bug.