Bug 17240 - Web Inspector: implement blackboxing of script resources
Summary: Web Inspector: implement blackboxing of script resources
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Devin Rousso
URL:
Keywords: InRadar
: 59119 66632 80383 (view as bug list)
Depends on:
Blocks: 198855
  Show dependency treegraph
 
Reported: 2008-02-08 14:04 PST by Adam Roben (:aroben)
Modified: 2019-09-03 19:26 PDT (History)
20 users (show)

See Also:


Attachments
Patch (36.58 KB, patch)
2019-06-14 01:28 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.92 MB, application/zip)
2019-06-14 02:24 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-highsierra (3.18 MB, application/zip)
2019-06-14 02:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-highsierra (3.03 MB, application/zip)
2019-06-14 03:18 PDT, Build Bot
no flags Details
Patch (67.23 KB, patch)
2019-06-14 17:29 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (3.20 MB, application/zip)
2019-06-14 18:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.77 MB, application/zip)
2019-06-14 18:49 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (3.04 MB, application/zip)
2019-06-14 19:20 PDT, Build Bot
no flags Details
Patch (67.65 KB, patch)
2019-06-14 19:34 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.72 MB, application/zip)
2019-06-14 20:53 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-highsierra (2.98 MB, application/zip)
2019-06-14 21:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-highsierra (3.18 MB, application/zip)
2019-06-15 02:23 PDT, Build Bot
no flags Details
Patch (54.96 KB, patch)
2019-08-04 21:08 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (68.21 KB, patch)
2019-08-08 16:14 PDT, Devin Rousso
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (3.48 MB, application/zip)
2019-08-08 17:27 PDT, Build Bot
no flags Details
Patch (68.21 KB, patch)
2019-08-08 17:43 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (73.53 KB, patch)
2019-08-29 17:12 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (1.16 MB, image/png)
2019-08-29 17:12 PDT, Devin Rousso
no flags Details
Patch (74.58 KB, patch)
2019-09-03 17:19 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2008-02-08 14:04:02 PST
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.
Comment 1 Adam Roben (:aroben) 2008-02-08 14:19:33 PST
<rdar://problem/5732847>
Comment 2 Brian Burg 2014-01-12 14:53:02 PST
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.
Comment 3 Brian Burg 2014-01-25 15:10:58 PST
*** Bug 59119 has been marked as a duplicate of this bug. ***
Comment 4 Brian Burg 2014-01-26 10:29:49 PST
*** Bug 80383 has been marked as a duplicate of this bug. ***
Comment 5 Yehuda Katz 2014-01-26 21:19:18 PST
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).
Comment 6 Timothy Hatcher 2016-02-10 15:53:23 PST
We should support source map level blackboxing. Per-function blackboxing is interesting, but might make for a complex UX.
Comment 7 Brian Burg 2017-05-24 09:11:03 PDT
*** Bug 66632 has been marked as a duplicate of this bug. ***
Comment 8 Emanuele Feliziani 2018-12-11 06:26:41 PST
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.
Comment 9 Devin Rousso 2019-06-14 01:28:01 PDT
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 -.-
Comment 10 Build Bot 2019-06-14 01:29:17 PDT
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 11 Build Bot 2019-06-14 01:29:28 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2019-06-14 02:24:00 PDT Comment hidden (obsolete)
Comment 13 Build Bot 2019-06-14 02:24:02 PDT Comment hidden (obsolete)
Comment 14 Build Bot 2019-06-14 02:40:04 PDT Comment hidden (obsolete)
Comment 15 Build Bot 2019-06-14 02:40:06 PDT Comment hidden (obsolete)
Comment 16 Build Bot 2019-06-14 03:18:57 PDT Comment hidden (obsolete)
Comment 17 Build Bot 2019-06-14 03:18:59 PDT Comment hidden (obsolete)
Comment 18 Devin Rousso 2019-06-14 17:29:21 PDT
Created attachment 372154 [details]
Patch
Comment 19 Build Bot 2019-06-14 17:31:39 PDT Comment hidden (obsolete)
Comment 20 Build Bot 2019-06-14 18:41:16 PDT Comment hidden (obsolete)
Comment 21 Build Bot 2019-06-14 18:41:18 PDT Comment hidden (obsolete)
Comment 22 Build Bot 2019-06-14 18:49:18 PDT Comment hidden (obsolete)
Comment 23 Build Bot 2019-06-14 18:49:20 PDT Comment hidden (obsolete)
Comment 24 Build Bot 2019-06-14 19:20:54 PDT Comment hidden (obsolete)
Comment 25 Build Bot 2019-06-14 19:20:56 PDT Comment hidden (obsolete)
Comment 26 Build Bot 2019-06-14 19:28:29 PDT Comment hidden (obsolete)
Comment 27 Devin Rousso 2019-06-14 19:34:51 PDT
Created attachment 372168 [details]
Patch
Comment 28 Build Bot 2019-06-14 19:38:03 PDT Comment hidden (obsolete)
Comment 29 Build Bot 2019-06-14 20:53:28 PDT Comment hidden (obsolete)
Comment 30 Build Bot 2019-06-14 20:53:30 PDT Comment hidden (obsolete)
Comment 31 Build Bot 2019-06-14 21:25:49 PDT Comment hidden (obsolete)
Comment 32 Build Bot 2019-06-14 21:25:51 PDT Comment hidden (obsolete)
Comment 33 Build Bot 2019-06-15 02:23:01 PDT Comment hidden (obsolete)
Comment 34 Build Bot 2019-06-15 02:23:03 PDT Comment hidden (obsolete)
Comment 35 Devin Rousso 2019-06-24 19:39:26 PDT
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)`.
Comment 36 Devin Rousso 2019-08-04 21:08:48 PDT
Created attachment 375517 [details]
Patch
Comment 37 Build Bot 2019-08-04 21:11:52 PDT Comment hidden (obsolete)
Comment 38 Brian Burg 2019-08-08 13:21:37 PDT
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 39 Devin Rousso 2019-08-08 16:12:03 PDT
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.
Comment 40 Devin Rousso 2019-08-08 16:14:54 PDT
Created attachment 375853 [details]
Patch
Comment 41 Build Bot 2019-08-08 16:18:14 PDT Comment hidden (obsolete)
Comment 42 Build Bot 2019-08-08 17:27:19 PDT Comment hidden (obsolete)
Comment 43 Build Bot 2019-08-08 17:27:21 PDT Comment hidden (obsolete)
Comment 44 Devin Rousso 2019-08-08 17:43:49 PDT
Created attachment 375870 [details]
Patch

Never in my life have I hated a period as much as this moment -.-
Comment 45 Build Bot 2019-08-08 17:45:00 PDT Comment hidden (obsolete)
Comment 46 Devin Rousso 2019-08-15 14:13:11 PDT
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 47 Joseph Pecoraro 2019-08-29 16:23:18 PDT
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 48 Devin Rousso 2019-08-29 17:10:25 PDT
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!
Comment 49 Devin Rousso 2019-08-29 17:12:27 PDT
Created attachment 377655 [details]
Patch
Comment 50 Devin Rousso 2019-08-29 17:12:42 PDT
Created attachment 377656 [details]
[Image] After Patch is applied
Comment 51 Joseph Pecoraro 2019-09-03 16:01:07 PDT
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 52 Devin Rousso 2019-09-03 17:18:00 PDT
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>.
Comment 53 Devin Rousso 2019-09-03 17:19:02 PDT
Created attachment 377935 [details]
Patch
Comment 54 WebKit Commit Bot 2019-09-03 19:26:44 PDT
Comment on attachment 377935 [details]
Patch

Clearing flags on attachment: 377935

Committed r249450: <https://trac.webkit.org/changeset/249450>
Comment 55 WebKit Commit Bot 2019-09-03 19:26:47 PDT
All reviewed patches have been landed.  Closing bug.