Bug 165578

Summary: Web Inspector: Unable to delete breakpoint from worker script
Product: WebKit Reporter: Matt Baker <mattbaker>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed Fix
none
[PATCH] Proposed Fix mattbaker: review+

Description Matt Baker 2016-12-07 18:38:47 PST
Summary:
Unable to delete breakpoint from worker script.

Steps to Reproduce:
1. Goto https://nerget.com/rayjs-mt/rayjs.html
2. Set a breakpoint at render-task.js:17
3. Hit Go!
  => Pauses at breakpoint
4. Delete breakpoint and resume
  => Pauses at line where breakpoint once lived
Comment 1 Joseph Pecoraro 2016-12-07 19:04:47 PST
Other code uses this pattern:

    if (breakpoint.contentIdentifier) {
        // Affect all targets
    } else if (breakpoint.scriptIdentifier) {
        // Affect only breakpoint.target
    }

I should be using this pattern in DebuggerManager.prototype._removeBreakpoint just like we do in DebuggerManager.prototype._setBreakpoint. Testing a fix now.
Comment 2 Joseph Pecoraro 2016-12-07 19:08:41 PST
Yep, that addresses the issue. I'll should be able to write a test for this.
Comment 3 Joseph Pecoraro 2016-12-07 20:00:57 PST
To clarify, the breakpoint is removed from only 1 Worker and not all Workers that shared a resource with the same URL.
Comment 4 Joseph Pecoraro 2016-12-07 20:03:20 PST
Created attachment 296468 [details]
[PATCH] Proposed Fix
Comment 5 Joseph Pecoraro 2016-12-07 20:16:36 PST
Comment on attachment 296468 [details]
[PATCH] Proposed Fix

Forgot to update test results in patch.
Comment 6 Joseph Pecoraro 2016-12-07 20:17:24 PST
Created attachment 296472 [details]
[PATCH] Proposed Fix
Comment 7 Matt Baker 2016-12-07 23:40:41 PST
Comment on attachment 296472 [details]
[PATCH] Proposed Fix

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

Nice test! r=me

> LayoutTests/inspector/worker/debugger-shared-breakpoint.html:33
> +    // pause on the next statement. Do an InspectorTest.log, which evalutes

evaluates

> LayoutTests/inspector/worker/debugger-shared-breakpoint.html:74
> +            InspectorTest.evaluateInPage(`triggerCreateWorkers(); triggerWorkerResponse();`);

Seems like a template literal isn't needed here.

> LayoutTests/inspector/worker/debugger-shared-breakpoint.html:100
> +            InspectorTest.evaluateInPage(`triggerWorkerBreakpointPauses()`);

Ditto.

> LayoutTests/inspector/worker/debugger-shared-breakpoint.html:126
> +            InspectorTest.evaluateInPage(`triggerWorkerResponse()`);

Ditto.
Comment 8 Joseph Pecoraro 2016-12-08 11:34:26 PST
Comment on attachment 296472 [details]
[PATCH] Proposed Fix

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

>> LayoutTests/inspector/worker/debugger-shared-breakpoint.html:74
>> +            InspectorTest.evaluateInPage(`triggerCreateWorkers(); triggerWorkerResponse();`);
> 
> Seems like a template literal isn't needed here.

It is easy to use a template literal for code to not have to worry about strings that may appear in the code.
Comment 9 Joseph Pecoraro 2016-12-08 12:25:10 PST
<https://trac.webkit.org/changeset/209557>