Bug 197034 – Web Inspector: Uncaught Exception: Breakpoint at specified location already exists.

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

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

[Radar WebKit Bug Importer]

<rdar://problem/50049004>

[Devin Rousso]

Created attachment 374644 [details]
Patch

[Joseph Pecoraro]

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).

[Joseph Pecoraro]

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?

[Devin Rousso]

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

[Devin Rousso]

Created attachment 374655 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 374655 [details]
Patch

r-me

[Joseph Pecoraro]

> 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!

[WebKit Commit Bot]

Comment on attachment 374655 [details]
Patch

Clearing flags on attachment: 374655

Committed r247715: <https://trac.webkit.org/changeset/247715>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.