1097141 - Various things that take a breakpoint/location should make a copy, rather than take ownership

Status: RESOLVED WONTFIX

(DevTools :: Debugger, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

We end up with really confusing bugs due to shared, mutable state. All of these APIs accept a breakpoint description (url, line, column, condition, actor) and don't just use the properties, but actually keep the same object around and it ends up being "accidentally" shared everywhere.

Instead, of taking ownership of the object passed in, these APIs should destructure the breakpoint description into its individual properties at the parameter level, and then allocate a copy to be used internally if need be.

    function takesBreakpointParam({ url, line, column, condtion, actor }) {
      // Just use the various properties here ...
      foo(line);
      // Make copies, if needed:
      if (breakpointStore.hasBreakpoint({ url, line, column })) ...
    }

Off the top of my head, here are some of the APIs that have this bad behavior:

ThreadActor.prototype._setBreakpoint

BreakpointStore.prototype.addBreakpoint

new BreakpointActor

breakpointActor.location = ... (this should become an actual setter or a method)

Comment 1

[Eddy Bruel [:ejpbruel]]

This probably won't be an issue anymore once I finish up my cleanup of the breakpoint code, but since that hasn't landed yet, I'm keeping this bug open until I do.

Comment 2

[Jason Laster [:jlast]]

I don't think is an issue any more after our refactoring of breakpoints.