Bug 164136 – Web Inspector: Include DebuggerAgent in Workers - see, pause, and step through scripts

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

Summary:
Include DebuggerAgent in Workers - see, pause, and step through scripts

Should be able to see scripts that evaluated in a worker, set breakpoints, pause, and step through those scripts with Web Inspector.

[Joseph Pecoraro]

Created attachment 293182 [details]
[PATCH] Proposed Fix

There are a few remaining issues:

  - Duplicate resources show up in sidebar
  - Worker XHR/Fetch sub resources do not yet show up under workers in the sidebar
  - Pause / Resume should affect all targets
    - Have a more "Thread" like pause experience

[Build Bot]

Comment on attachment 293182 [details]
[PATCH] Proposed Fix

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

New failing tests:
inspector/worker/console-basic.html

[Build Bot]

Created attachment 293190 [details]
Archive of layout-test-results from ews101 for mac-yosemite

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

[Build Bot]

Comment on attachment 293182 [details]
[PATCH] Proposed Fix

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

New failing tests:
inspector/worker/console-basic.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 293182 [details]
[PATCH] Proposed Fix

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

New failing tests:
inspector/worker/console-basic.html

[Build Bot]

Created attachment 293193 [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

[Joseph Pecoraro]

Oops, easy fix.

[Joseph Pecoraro]

Created attachment 293206 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

(Bump Importer)

[Radar WebKit Bug Importer]

<rdar://problem/29028462>

[Blaze Burg]

Comment on attachment 293206 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/ChangeLog:21
> +        which is per-target, accessed through DebuggerManager's new
> +        dataForTarget(target) method.

Can we just call it TargetDebuggerData?

[Joseph Pecoraro]

Comment on attachment 293206 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/ChangeLog:21
>> +        dataForTarget(target) method.
> 
> Can we just call it TargetDebuggerData?

Originally I did, and then I simplified it.

[Blaze Burg]

Comment on attachment 293206 [details]
[PATCH] Proposed Fix

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

r=me

> LayoutTests/inspector/worker/debugger-pause.html:10
> +let worker = new Worker("resources/worker-debugger-pause.js");

Does this synchronously wait for the worker to be initialized? I am worried that it could race with test() executing in WebInspectorUI.

> LayoutTests/inspector/worker/debugger-scripts.html:31
> +            InspectorTest.expectThat(script instanceof WebInspector.Script, "Worker target's main resource should be a Script.");

Idea; we need InspectorTest.expectInstanceof so we can log the actual constructor if it fails.

> LayoutTests/inspector/worker/debugger-scripts.html:35
> +            InspectorTest.expectThat(workerDebuggerData.scriptsForURL(script.url).length === 1, "Worker DebuggerData should include script by URL.");

Nit: expectEqual

> Source/WebCore/inspector/WorkerDebuggerAgent.h:42
> +    // We don't need to mute console for workers.

(...because the worker's injected scripts cannot cause spurious security origin errors.)

> Source/WebInspectorUI/ChangeLog:24
> +        Finally we make a few changes to the UserInterface to make Workers
> +        and their scripts, group together better. The Resources sidebar

Grammaro

> Source/WebInspectorUI/ChangeLog:92
> +        When a new Target is created, syncronize frontend state with the target.

Speling

> Source/WebInspectorUI/UserInterface/Base/Main.js:2518
> +// Many places assume the main target because they cannot yet be

This is a great idea.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:91
>          DebuggerAgent.setBreakpointsActive(this._breakpointsEnabledSetting.value);
>  
> -        this._updateBreakOnExceptionsState();
> +        this._updateBreakOnExceptionsState(WebInspector.mainTarget);
>  
>          // COMPATIBILITY (iOS 10): DebuggerAgent.setPauseOnAssertions did not exist yet.
>          if (DebuggerAgent.setPauseOnAssertions)

See comment below, we should run this through initializeTarget too. We may need to defer restoring breakpoints til a later runloop, though.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:115
>      get activeCallFrame()

I don't understand why we have one active call frame here, each target should have its own(?)

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:227
>          this._updateBreakOnExceptionsState();

I would prefer to fold this into the loop above and remove special interpretation of passing no argument.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:257
> +        for (let [target, targetData] of this._targetDebuggerDataMap) {

'target' is not used.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:283
> +        // FIXME: Pause all

Nit: missing period

It would be great to file bugs for these tasks, since we plan to fix them anyway.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:306
> +        // FIXME: Resume all.

Ditto.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:474
> +        let DebuggerAgent = target.DebuggerAgent;

It would be great to refactor the code for MainTarget to take the same initialization path as WorkerTarget. This would require an explicit call to initializeTarget from the DebuggerManager constructor or in Main.js, since the target is created before the managers. This should be fine since we won't get any messages incoming while doing initial setup.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:493
> +        // FIXME: Pause the new target if needed.

Oops, delete the commented code.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:1033
> +        if (target)

Err, this seems rather bug prone, considering there's no default parameter value. I had to go look at all callers to figure out where worker targets get passed, turns out they don't!

> Source/WebInspectorUI/UserInterface/Models/DebuggerData.js:87
> +    pause(callFrames, pauseReason, pauseData)

Please rename to updateForPause() so the callsite is less ambiguous.

> Source/WebInspectorUI/UserInterface/Models/DebuggerData.js:94
> +    unpause()

Please rename to updateForResume() so the callsite is less ambiguous.

> Source/WebInspectorUI/UserInterface/Protocol/DebuggerObserver.js:73
> +        WebInspector.debuggerManager.debuggerDidResume(this.target);

Unrelated to this change, but this made me wonder how the debugger will transition in/out of pausing when multiple targets are involved. It looks like each of them sends a separate resume message, so maybe the UI should join on a promise per target?

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:676
> +        // FIXME: What is this, and is it still relevant?

https://trac.webkit.org/changeset/154998

" We now allow debugging in such a scenario and we filter out call frames coming from the
        Web Inspector injected script as well as the call frame for the console prompt such that
        we only pause in the debugger in case the exception is in code under the console prompt
        and not the console code prompt itself.

         Additionally, to prevent stepping out to call frames we may have filtered out, we disable
        the "step out" button in cases where there are no further frames in the frontend to go out to."

We don't add callFrames to the target in these cases:

            // FIXME: There may be useful call frames without a source code location (native callframes), should we include them?
            if (!sourceCodeLocation)
                continue;
545573            if (!sourceCodeLocation.sourceCode)
546574                continue;
547575
548576            // Exclude the case where the call frame is in the inspector code.
            if (!WebInspector.isDebugUIEnabled() && isWebKitInternalScript(sourceCodeLocation.sourceCode.sourceURL))
                continue;

> Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:380
> +                this._deferredTargetScripts.push(script);

Please do

if (...)
  abc
else
  def

return

> Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:383
> +            this._addTargetWithMainResource(script.target);

I see, this just creates the TargetTreeElement. If we try to find a tree element for `script`, what's going to happen? I think the representedObject code path would not work. (What do we do for main target's main resource? does it have the same issue?)

> Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:402
> +        let [deferredScriptForThisTarget, deferredScriptForAnotherTarget] = this._deferredTargetScripts.partition((script) => script.target === target);

Variables hold arrays, they should be plural.

> Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:413
> +        let target = event.data.target;

s/target/removedTarget/

> Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:419
> +        this._deferredTargetScripts = this._deferredTargetScripts.filter((script) => script.target !== target);

then, the check is script.target !== removedTarget

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:104
> +    get target()

o_O

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:112
> +        return WebInspector.mainTarget;

Nit: add newline prior to return

[Joseph Pecoraro]

Comment on attachment 293206 [details]
[PATCH] Proposed Fix

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

>> LayoutTests/inspector/worker/debugger-pause.html:10
>> +let worker = new Worker("resources/worker-debugger-pause.js");
> 
> Does this synchronously wait for the worker to be initialized? I am worried that it could race with test() executing in WebInspectorUI.

Nope it loads asynchronously. It may race, but I've never seen any of these tests fail with Missing Worker Target. If they do, then I suppose I would need to address this.

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:115
>>      get activeCallFrame()
> 
> I don't understand why we have one active call frame here, each target should have its own(?)

Each target has a list of call frames. The frontend has a single active (selected) call frame.

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:493
>> +        // FIXME: Pause the new target if needed.
> 
> Oops, delete the commented code.

We will actually need to do this with the other Pause/Resume work. So I'll at least leave a FIXME and bugzilla comment.

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:1033
>> +        if (target)
> 
> Err, this seems rather bug prone, considering there's no default parameter value. I had to go look at all callers to figure out where worker targets get passed, turns out they don't!

The only time this happens is in DebuggerManager.prototype.initializeTarget where this gets called with the single worker target. I'll rethink this.

>> Source/WebInspectorUI/UserInterface/Protocol/DebuggerObserver.js:73
>> +        WebInspector.debuggerManager.debuggerDidResume(this.target);
> 
> Unrelated to this change, but this made me wonder how the debugger will transition in/out of pausing when multiple targets are involved. It looks like each of them sends a separate resume message, so maybe the UI should join on a promise per target?

Yep, I think I'll end up with an approach like that.

>> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:676
>> +        // FIXME: What is this, and is it still relevant?
> 
> https://trac.webkit.org/changeset/154998
> 
> " We now allow debugging in such a scenario and we filter out call frames coming from the
>         Web Inspector injected script as well as the call frame for the console prompt such that
>         we only pause in the debugger in case the exception is in code under the console prompt
>         and not the console code prompt itself.
> 
>          Additionally, to prevent stepping out to call frames we may have filtered out, we disable
>         the "step out" button in cases where there are no further frames in the frontend to go out to."
> 
> We don't add callFrames to the target in these cases:
> 
>             // FIXME: There may be useful call frames without a source code location (native callframes), should we include them?
>             if (!sourceCodeLocation)
>                 continue;
> 545573            if (!sourceCodeLocation.sourceCode)
> 546574                continue;
> 547575
> 548576            // Exclude the case where the call frame is in the inspector code.
>             if (!WebInspector.isDebugUIEnabled() && isWebKitInternalScript(sourceCodeLocation.sourceCode.sourceURL))
>                 continue;

Then I don't think we should do this. Step out should be enabled, and clicking it will just complete the program. That seems totally fine to me.

>> Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:383
>> +            this._addTargetWithMainResource(script.target);
> 
> I see, this just creates the TargetTreeElement. If we try to find a tree element for `script`, what's going to happen? I think the representedObject code path would not work. (What do we do for main target's main resource? does it have the same issue?)

This is code that I'm rewriting now to support Resources in Workers.

In web pages this is solved by FrameTreeElement adding all its sub-resources and scripts. I'm aiming to do the same with TargetTreeElement adding all its sub-resources and scripts.

>> Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:402
>> +        let [deferredScriptForThisTarget, deferredScriptForAnotherTarget] = this._deferredTargetScripts.partition((script) => script.target === target);
> 
> Variables hold arrays, they should be plural.

And this code will hopefully just go away, since the TargetTreeElement would manage these sub-resources of the Target.

[Joseph Pecoraro]

https://trac.webkit.org/changeset/208304

[Joseph Pecoraro]

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

[Joseph Pecoraro]

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