Bug 152445 – Web Inspector: Make it possible to debug injected scripts when the Debug UI is enabled

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

* SUMMARY
Make it possible to debug injected scripts when the Debug UI is enabled.

[Matt Baker]

Created attachment 267660 [details]
[Patch] Proposed Fix

[Matt Baker]

Comment on attachment 267660 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=267660&action=review

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:67
> +        this._excludedScripts = [];

This should be renamed _inspectorDebugScripts, since they aren't initially excluded if the Inspector loads in debug UI mode.

[Joseph Pecoraro]

Comment on attachment 267660 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=267660&action=review

r- for the open questions, but this patch does look good. One more iteration and this should be good!

> Source/WebInspectorUI/UserInterface/Base/Main.js:447
> +        if (WebInspector.showDebugUISetting) {
> +            WebInspector.showDebugUISetting.addEventListener(WebInspector.Setting.Event.Changed, () => {
> +                this.notifications.dispatchEventToListeners(WebInspector.Notification.DebugUIEnabledDidChange);
> +            });
> +        }
> +    }

Why not just have Debug/Bootstrap.js do this? Seems weird to have Main.js do it.

> Source/WebInspectorUI/UserInterface/Base/Main.js:1405
> +WebInspector.isInternalScript = function(url)
> +{
> +    if (!url)
> +        return false;
> +
> +    return url === "__WebInspectorInternal__";
> +}

Nit: Early return here won't actually matter.

> Source/WebInspectorUI/UserInterface/Base/Object.js:212
> +    DebugUIEnabledDidChange: "debug-ui-enabled-did-change"

Nit: Add the trailing comma, it makes future diffs nicer.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:67
> +        this._excludedScripts = [];

This name can be improved, right now it doesn't indicate that it is purely for debug UI purposes. How about: this._excludedDebugScripts or even better this._debugScripts, since they may not be excluded.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:-491
> -            // Exclude the case where the call frame is in the inspector code.
> -            if (sourceCodeLocation.sourceCode.url && sourceCodeLocation.sourceCode.url.startsWith("__WebInspector"))
> -                continue;

I don't think this is right. Debug Scripts are always included in DebuggerManager's script maps, even in Non-Debug mode. So we should get a sourceCodeLocation with a URL that startsWith "__WebInspector".

Seems we might still want this code but with a WebInspector.isDebugUIEnabled() check.

To test, just type this into the console:
js> function foo() { debugger; } foo()

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:534
> +        if (WebInspector.isInternalScript(script.url))
> +            return;

This can be done above the construction of the WebInspector.Script, and could save us a bunch of unnecessary work!

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:514
> -            if (!sourceCode || sourceCode.url.startsWith("__WebInspector")) {
> +            if (!sourceCode) {

This still seems relevant. In Non-Debug UI we probably do not want to link users into InjectedScriptSource code. Would that even work!?

To test this, you could add a log into InjectedScriptSource.js (inspectedGlobalObject.console.log("test")) and ensure in Non-Debug it doesn't have a location link, but in Debug it does.

Seems we would again want a WebInspector.isDebugUIEnabled check here.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:423
> +        console.assert(scriptTreeElement.parent);

I don't think this assert wins us anything. If it fails the next line will give us an exception with the same information.

> Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:317
> +        scriptTreeElement.parent.removeChild(scriptTreeElement);

I don't think this assert wins us anything. If it fails the next line will give us an exception with the same information.

[Matt Baker]

Comment on attachment 267660 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=267660&action=review

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:-491
>> -                continue;
> 
> I don't think this is right. Debug Scripts are always included in DebuggerManager's script maps, even in Non-Debug mode. So we should get a sourceCodeLocation with a URL that startsWith "__WebInspector".
> 
> Seems we might still want this code but with a WebInspector.isDebugUIEnabled() check.
> 
> To test, just type this into the console:
> js> function foo() { debugger; } foo()

Nice catch! In non-debug UI mode the test should cause us to pause in the debugger, with Call Stack -> No Call Frames. Do we care about leaving the inspector call frames visible in the following situation:

1. Enable debug UI mode
2. js> function foo() { debugger; } foo()
3. Disable debug UI mode
  => Injected script call frames still visible

[Joseph Pecoraro]

(In reply to comment #4)
> Comment on attachment 267660 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267660&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:-491
> >> -                continue;
> > 
> > I don't think this is right. Debug Scripts are always included in DebuggerManager's script maps, even in Non-Debug mode. So we should get a sourceCodeLocation with a URL that startsWith "__WebInspector".
> > 
> > Seems we might still want this code but with a WebInspector.isDebugUIEnabled() check.
> > 
> > To test, just type this into the console:
> > js> function foo() { debugger; } foo()
> 
> Nice catch! In non-debug UI mode the test should cause us to pause in the
> debugger, with Call Stack -> No Call Frames. Do we care about leaving the
> inspector call frames visible in the following situation:
> 
> 1. Enable debug UI mode
> 2. js> function foo() { debugger; } foo()
> 3. Disable debug UI mode
>   => Injected script call frames still visible

No I don't care about leaving them around. We want enough debug UI that things are useful, but not enough that it adds complexity to the code base.

[Joseph Pecoraro]

> > js> function foo() { debugger; } foo()
> 
> Nice catch! In non-debug UI mode the test should cause us to pause in the
> debugger, with Call Stack -> No Call Frames.

This is interesting to me. Why isn't there a "foo" call frame? This might be exposing an issue with isInternalScript(...), since all of this will be included in an internal script from the console:

> with ((this && (this.console ? this.console.__commandLineAPI : this.__commandLineAPI)) || {}) {
>     function foo() { debugger; }; foo();
>     //# sourceURL=__WebInspectorInternal__
> 
> }

[Matt Baker]

Created attachment 267666 [details]
[Patch] Proposed Fix

[Build Bot]

Comment on attachment 267666 [details]
[Patch] Proposed Fix

Attachment 267666 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/577894

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 267669 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 267666 [details]
[Patch] Proposed Fix

Attachment 267666 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/577884

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 267670 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 267666 [details]
[Patch] Proposed Fix

Attachment 267666 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/577899

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 267673 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Joseph Pecoraro]

Comment on attachment 267666 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=267666&action=review

r- because this broke tests and 1 more open question. Suggestions below on how to address them.

> Source/WebInspectorUI/UserInterface/Base/Main.js:1389
> +    return WebInspector.showDebugUISetting && WebInspector.showDebugUISetting.value

Style: Semicolon.

> Source/WebInspectorUI/UserInterface/Base/Main.js:1400
> +WebInspector.isInternalScript = function(url)
> +{
> +    return url === "__WebInspectorInternal__";
> +}
> +
> +WebInspector.isDebugScript = function(url)
> +{
> +    return url && url.startsWith("__WebInspector");
> +}

You may need to re-implement these in Test.js, or share the code somehow. Otherwise, LayoutTests (as we've seen on the EWS bots) will fail because of code using them.

You may even want to drop the "WebInspector." prefix and just put them in Utilities.js alongside "appendWebInspectorSourceURL()"!

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:-323
> -            if (script.url && script.url.startsWith("__WebInspector"))
> -                continue;

This part still needs to exist with WebInspector.isDebugUIEnabled(). Otherwise I think Internal scripts can show in the Debugger tab when the inspector first opens.

Steps to reproduce:
1. Inspect <body> of about:blank (so Elements tab)
2. Evaluate 1+1 in the console
3. Close inspector
4. Open inspector (should still be Elements tab)
5. Show Debugger tab
6. Ensure sidebar is showing all scripts
  => I think this would show the Debug Scripts in the Debugger's sidebar

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:401
>          if (!script.url)
>              return;

There is a FIXME above this that I think can be eliminated now, or at least updated. We should not be told about any JSC internal scripts at this point. That said, we probably should keep this code for compatibility, maybe even with a COMPATIBILITY(iOS 9): Backends could send the frontend built-in code. But certainly the part about WebInspector internals can be removed, as we control that.

[Matt Baker]

Created attachment 267680 [details]
[Patch] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 267680 [details]
[Patch] Proposed Fix

r=me!

[WebKit Commit Bot]

Comment on attachment 267680 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 267680

Committed r194311: <http://trac.webkit.org/changeset/194311>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.