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
<rdar://problem/50049004>
Created attachment 374644 [details] Patch
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 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 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
Created attachment 374655 [details] Patch
Comment on attachment 374655 [details] Patch r-me
> 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 on attachment 374655 [details] Patch Clearing flags on attachment: 374655 Committed r247715: <https://trac.webkit.org/changeset/247715>
All reviewed patches have been landed. Closing bug.