Bug 197034

Summary: Web Inspector: Uncaught Exception: Breakpoint at specified location already exists.
Product: WebKit Reporter: Devin Rousso <drousso>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, drousso, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
URL: https://docs.google.com/spreadsheets/d/1ctYXqr9pEmQLC4X6UkKTKWGSWXquhlIbCLrQXCqUpP0/edit#gid=0
Attachments:
Description Flags
Patch
none
Patch none

Description Devin Rousso 2019-04-17 13:52:22 PDT
Inspected URL:        https://docs.google.com/spreadsheets/d/1ctYXqr9pEmQLC4X6UkKTKWGSWXquhlIbCLrQXCqUpP0/edit#gid=0
Loading completed:    true
Frontend User Agent:  Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15) AppleWebKit/605.1.15 (KHTML, like Gecko)
Dispatch Source:      Protocol Event

Protocol Event:
{"method":"Target.dispatchMessageFromTarget","params":{"targetId":"page-1","message":"{\"error\":{\"code\":-32000,\"message\":\"Breakpoint at specified location already exists.\",\"data\":[{\"code\":-32000,\"message\":\"Breakpoint at specified location already exists.\"}]},\"id\":535}"}}
Dispatch Source:      Protocol Command Response

Protocol Command Response:
{"error":{"code":-32000,"message":"Breakpoint at specified location already exists.","data":[{"code":-32000,"message":"Breakpoint at specified location already exists."}]},"id":535}

Protocol Command Request:
{"id":535,"method":"Debugger.setBreakpointByUrl","params":{"lineNumber":3960,"url":"https://docs.google.com/static/spreadsheets2/client/js/1864039502-waffle_js_prod_core.js","columnNumber":460,"options":{"condition":"","ignoreCount":0,"autoContinue":false,"actions":[]}}}

Uncaught Exceptions:
 - Breakpoint at specified location already exists. (at Main.js:3185:75)
    reportInternalError @ Main.js:3185:75
    didSetBreakpoint @ DebuggerManager.js:934:39
    didSetBreakpoint @ [native code]
    _dispatchResponseToCallback @ Connection.js:148:27
    _dispatchResponse @ Connection.js:118:45
    dispatch @ Connection.js:70:35
    dispatchMessageFromTarget @ TargetManager.js:101:35
    dispatchMessageFromTarget @ TargetObserver.js:42:51
    dispatchEvent @ InspectorBackend.js:340:42
    _dispatchEvent @ Connection.js:195:32
    dispatch @ Connection.js:72:32
    dispatch @ InspectorBackend.js:178:52
    dispatchNextQueuedMessageFromBackend @ MessageDispatcher.js:42:34
Comment 1 Radar WebKit Bug Importer 2019-04-19 09:00:40 PDT
<rdar://problem/50049004>
Comment 2 Devin Rousso 2019-07-22 15:49:37 PDT
Created attachment 374644 [details]
Patch
Comment 3 Joseph Pecoraro 2019-07-22 16:20:03 PDT
Comment on attachment 374644 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:11
> +        When "adjusting" a `WI.Breakpoint` (e.g. removing and then re-adding with a different
> +        configuration), make sure to only re-add the `WI.Breakpoint` to the `WI.Target` it was just
> +        removed from, rather than iterate over all `WI.targets`.

I'm not sure I understand.

We set each breakpoint in all targets, so if we remove and re-add a breakpoint it should be removed and re-added to all targets.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:931
> +    _setBreakpoint(breakpoint, target)

I still prefer `specificTarget` to avoid the shadow below (let target of targets).
Comment 4 Joseph Pecoraro 2019-07-22 16:20:46 PDT
Comment on attachment 374644 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:11
>> +        removed from, rather than iterate over all `WI.targets`.
> 
> I'm not sure I understand.
> 
> We set each breakpoint in all targets, so if we remove and re-add a breakpoint it should be removed and re-added to all targets.

Unless you mean we remove from all target and individually add back to each target?
Comment 5 Devin Rousso 2019-07-22 16:48:31 PDT
Comment on attachment 374644 [details]
Patch

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

>>> Source/WebInspectorUI/ChangeLog:11
>>> +        removed from, rather than iterate over all `WI.targets`.
>> 
>> I'm not sure I understand.
>> 
>> We set each breakpoint in all targets, so if we remove and re-add a breakpoint it should be removed and re-added to all targets.
> 
> Unless you mean we remove from all target and individually add back to each target?

Previously, the pseudo-code looked like this:

for each target (tA):                        <WI.DebuggerManager.prototype._removeBreakpoint>
  remove breakpoint from tA -> callback {    <WI.DebuggerManager.prototype._removeBreakpoint>
    for each target (tB):                    <WI.DebuggerManager.prototype._addBreakpoint>
        add breakpoint to tB                 <WI.DebuggerManager.prototype._addBreakpoint>
  }

This is a problem because each iteration of the inner `for` is re-adding the breakpoint to ALL targets (one-to-many), not just the one it was just removed from.

The new pseudo-code looks like this:

for each target (tA):                        <WI.DebuggerManager.prototype._removeBreakpoint>
  remove breakpoint from tA -> callback {    <WI.DebuggerManager.prototype._removeBreakpoint>
    add breakpoint to tA                     <WI.DebuggerManager.prototype._addBreakpoint>
  }

Now, we only re-add the breakpoint to the target it was just removed from (one-to-one).

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:931
>> +    _setBreakpoint(breakpoint, target)
> 
> I still prefer `specificTarget` to avoid the shadow below (let target of targets).

Oh oops :P
I'll change it back
Comment 6 Devin Rousso 2019-07-22 16:57:50 PDT
Created attachment 374655 [details]
Patch
Comment 7 Joseph Pecoraro 2019-07-22 17:42:21 PDT
Comment on attachment 374655 [details]
Patch

r-me
Comment 8 Joseph Pecoraro 2019-07-22 17:42:42 PDT
> Now, we only re-add the breakpoint to the target it was just removed from
> (one-to-one).

Okay good that is what I was expecting!
Comment 9 WebKit Commit Bot 2019-07-22 20:45:34 PDT
Comment on attachment 374655 [details]
Patch

Clearing flags on attachment: 374655

Committed r247715: <https://trac.webkit.org/changeset/247715>
Comment 10 WebKit Commit Bot 2019-07-22 20:45:35 PDT
All reviewed patches have been landed.  Closing bug.