Bug 17240 – Web Inspector: implement blackboxing of script resources

• Component: Web Inspector
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

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.

[Adam Roben (:aroben)]

<rdar://problem/5732847>

[Blaze Burg]

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.

[Blaze Burg]

*** Bug 59119 has been marked as a duplicate of this bug. ***

[Blaze Burg]

*** Bug 80383 has been marked as a duplicate of this bug. ***

[Yehuda Katz]

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).

[Timothy Hatcher]

We should support source map level blackboxing. Per-function blackboxing is interesting, but might make for a complex UX.

[Blaze Burg]

*** Bug 66632 has been marked as a duplicate of this bug. ***

[Emanuele Feliziani]

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.

[Devin Rousso]

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 -.-

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Devin Rousso]

Created attachment 372154 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Devin Rousso]

Created attachment 372168 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Devin Rousso]

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)`.

[Devin Rousso]

Created attachment 375517 [details]
Patch

[EWS Watchlist]

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.

[Blaze Burg]

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?

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 375853 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Devin Rousso]

Created attachment 375870 [details]
Patch

Never in my life have I hated a period as much as this moment -.-

[EWS Watchlist]

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.

[Devin Rousso]

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>.

[Joseph Pecoraro]

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.

[Devin Rousso]

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!

[Devin Rousso]

Created attachment 377655 [details]
Patch

[Devin Rousso]

Created attachment 377656 [details]
[Image] After Patch is applied

[Joseph Pecoraro]

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`);

[Devin Rousso]

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>.

[Devin Rousso]

Created attachment 377935 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 377935 [details]
Patch

Clearing flags on attachment: 377935

Committed r249450: <https://trac.webkit.org/changeset/249450>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.