1102441 - Clean up the BreakpointStore

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Comment 1

[Eddy Bruel [:ejpbruel]]

The BreakpointStore currently stores breakpoint locations to which actors are associated. These breakpoint locations are shared between other objects. Moreover, they are mutable, which has led to some problems throughout our code.

It would make much more sense to think of BreakpointStore as a map from breakpoint locations to breakpoint actors, only explicitly storing the latter, and only using the former as keys.

This refactor is also the first step in cleaning up the way we handle breakpoints when source maps are present.

Comment 2

[Eddy Bruel [:ejpbruel]]

Attachment: Step 1. Remove ThreadActor.prototype._createAndStoreBreakpoint

_createAndStoreBreakpoint creates a breakpoint in the breakpoint store before calling _setBreakpoint. The latter then ends up calling _getOrCreateBreakpointActor, which 
expects the breakpoint to be there. The only reason _createAndStoreBreakpointActor exists is to create the breakpoint for _getOrCreateBreakpointActor. If we moved creation of the breakpoint to _getOrCreateBreakpointActor itself (only doing so if the breakpoint does not already exist), _createAndStoreBreakpoint would no longer be necessary.

Comment 3

[Eddy Bruel [:ejpbruel]]

Attachment: Step 2. Remove BreakpointStore.prototype.moveBreakpoint

Comment 4

[Eddy Bruel [:ejpbruel]]

This should have gone with comment 2 :-(

moveBreakpoint is only called from _setBreakpoint. When thinking  about the BreakpointStore as a 
map from locations to actors, moving a breakpoint can be thought of us a deleting the actor
from the previous location and then setting it to the new location. I think it would be clearer to make
this explicit, in which case this method would no longer be necessary.

Comment 5

[Eddy Bruel [:ejpbruel]]

Attachment: Step 3. Remove BreakpointStore.prototype.getBreakpoint

getBreakpoint can be thought of as an variant of hasBreakpoint that throws if no 
breakpoint exists at the given location. However, all of our calls to getBreakpoint 
are actually infallible in the sense that they will never throw unless we have a 
programmer error. For programmer errors, assertions are usually better than 
exceptions, but having a separate method for that is overkill.

Comment 6

[Eddy Bruel [:ejpbruel]]

Attachment: Step 4. Rename BreakpointStore.prototype.hasBreakpoint

Now that getBreakpoint is gone, getBreakpoint would actually be a better name for hasBreakpoint. The latter suggests that it returns a boolean, while it actually returns the breakpoint itself.

Comment 7

[Eddy Bruel [:ejpbruel]]

Attachment: Step 5. Refactor ThreadActor.prototype._getOrCreateBreakpointActor

Now that we create the breakpoint in _getOrCreateBreakpointActor, there is never a case where a breakpoint exists, but doesn’t have an actor associated with it, so we can simplify some of the checks in this method.

Comment 8

[Eddy Bruel [:ejpbruel]]

Attachment: Step 6. Refactor ThreadActor.prototype.addBreakpoint

Now that we’ve simplified _getOrCreateBreakpointActor, whenever addBreakpoint is 
called to create a breakpoint, we immediately associate an actor with it. By moving 
the latter operation to addBreakpoint we can think of it as associating an actor with the given breakpoint/location.

Comment 9

[Eddy Bruel [:ejpbruel]]

Attachment: Step 7. Refactor ThreadActor.prototype.getBreakpoint

Finally, by making getBreakpoint return the actor associated with the breakpoint 
rather than the breakpoint itself, we no longer have to store the breakpoint passed 
to addBreakpoint and associate an actor with it: instead we can just store the 
actor itself.

Comment 11

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

Comment on attachment 8526232 [details] [diff] [review]
Step 1. Remove ThreadActor.prototype._createAndStoreBreakpoint

Review of attachment 8526232 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/script.js
@@ +1448,5 @@
>      let actor;
> +    let storedBp = this.breakpointStore.hasBreakpoint(location);
> +    if (!storedBp) {
> +      storedBp = this.breakpointStore.addBreakpoint(location);
> +    }

BreakpointStore.prototype.getOrAddBreakpoint?

Comment 12

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

Comment on attachment 8526233 [details] [diff] [review]
Step 2. Remove BreakpointStore.prototype.moveBreakpoint

Review of attachment 8526233 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not convinced this is ok -- what about the case where we set a "pending" BP b/c we don't have any scripts, and then later, when we actually get the script and set the BP, we discover we have to slide the BP down? In this case, I think we still need to move the BP in the store. Convince me that this case is handled, and I'll r+.

Comment 13

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

Comment on attachment 8526237 [details] [diff] [review]
Step 3. Remove BreakpointStore.prototype.getBreakpoint

Review of attachment 8526237 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with assertions added.

::: toolkit/devtools/server/actors/script.js
@@ +1198,5 @@
>          if (resp.error) {
>            reportError(new Error("Unable to set breakpoint on event listener"));
>            return;
>          }
> +        let bp = this.breakpointStore.hasBreakpoint(location);

Ok, runtime assertions are better for this, but where are they? Here and below, add:

  dbg_assert(bp, "should have bp or whatever lol...");

Comment 14

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

Comment on attachment 8526243 [details] [diff] [review]
Step 5. Refactor ThreadActor.prototype._getOrCreateBreakpointActor

Review of attachment 8526243 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/script.js
@@ +1412,5 @@
>        return actor;
>      }
>  
> +    actor = storedBp.actor;
> +    actor.condition = location.condition;

Kind of nitty, but while we're here, let's nix the actor binding:

  storedBP.actor.condition = location.condition;
  return storedBP.actor;

The actor variable looks like its going to be shared due to the block its defined in, but isn't, and this is mostly just historical from all the refactorings this code has been through and moved around.

Alternatively, we could create two different actor bindings:

  if (!storedBP) {
    ...
    let actor = storedBP.actor = new BreakpointActor(...);
    ...
    return actor;
  }

  let actor = storedBP.actor;
  ...
  return actor;

The point is that right now it's funky and misleading, like its going to be shared between the if branches (like it used to be, long ago) when it really isn't.

I realize I should have done this when I first moved this code, but alas...

Comment 15

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

Comment on attachment 8526245 [details] [diff] [review]
Step 6. Refactor ThreadActor.prototype.addBreakpoint

Review of attachment 8526245 [details] [diff] [review]:
-----------------------------------------------------------------

Really like the spirit of this change, but want the API cleaned up a little. Want to take a quick look at it again once you've updated it, before I rubber stamp it.

::: toolkit/devtools/server/actors/script.js
@@ +113,3 @@
>      let { url, line, column } = aBreakpoint;
>  
> +    aBreakpoint.actor = aActor;

Either use the actor property we document `aBreakpoint` to optionally (no longer optional?) have, or remove that property from usage and the docs.

We shouldn't have both an actor parameter and an actor property.

In fact, could this take only a breakpoint actor, since the breakpoint actor has its location as a public property that we can just use directly?

Comment 16

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

Comment on attachment 8526247 [details] [diff] [review]
Step 7. Refactor ThreadActor.prototype.getBreakpoint

Review of attachment 8526247 [details] [diff] [review]:
-----------------------------------------------------------------

r+ asssuming we fix the API stuff I commented about in the last patch.

Comment 17

[Eddy Bruel [:ejpbruel]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #12)
> Comment on attachment 8526233 [details] [diff] [review]
> Step 2. Remove BreakpointStore.prototype.moveBreakpoint
> 
> Review of attachment 8526233 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not convinced this is ok -- what about the case where we set a "pending"
> BP b/c we don't have any scripts, and then later, when we actually get the
> script and set the BP, we discover we have to slide the BP down? In this
> case, I think we still need to move the BP in the store. Convince me that
> this case is handled, and I'll r+.

In the scenario you outlined, we would first end up in setBreakpoint via onSetBreakpoint. setBreakpoint would create a new breakpoint actor, and store it in the breakpoint store at the original location. We don't have any scripts that match the given original location yet, so we are done for now.

When a new script arrives, we end up in _addScript. We then query the breakpoint store for all actors at locations in range of the new script, and call _setBreakpoint again, getting the exact location from the actor, and passing the script as an extra parameter, so that we will only try to add the actor to that script.

For the second call to _setBreakpoint, we know there will be at least one script that matches the given original location (otherwise, _setBreakpoint would not have been called in the first place). Now we figure out that there aren't actually any offsets in any of these scripts for the original location, so we compute the actual location, destroy the existing actor, remove it from the breakpoint store, create a new actor for the actual location, and add that to the breakpoint store.

This is how things happen right now. This patch actually does not affect that at all. It simply replaces the call to moveBreakpoint with an explicit call to removeBreakpoint and setBreakpoint. This is more in the spirit of treating the BreakpointStore as a location -> actor map. But that's all it does.

As an aside, what happens if somebody sends a request to set breakpoint on a location for which we don't have any scripts, gets back an actor, and later we learn that the breakpoint should have been moved. Right now, the original actor is destroyed, and we create a new one for the actual location. But is there a way for the client to know about this and get a handle to the new actor?

Comment 18

[Eddy Bruel [:ejpbruel]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #15)
> Comment on attachment 8526245 [details] [diff] [review]
> Step 6. Refactor ThreadActor.prototype.addBreakpoint
> 
> Review of attachment 8526245 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Really like the spirit of this change, but want the API cleaned up a little.
> Want to take a quick look at it again once you've updated it, before I
> rubber stamp it.
> 
> ::: toolkit/devtools/server/actors/script.js
> @@ +113,3 @@
> >      let { url, line, column } = aBreakpoint;
> >  
> > +    aBreakpoint.actor = aActor;
> 
> Either use the actor property we document `aBreakpoint` to optionally (no
> longer optional?) have, or remove that property from usage and the docs.
> 
> We shouldn't have both an actor parameter and an actor property.
> 
> In fact, could this take only a breakpoint actor, since the breakpoint actor
> has its location as a public property that we can just use directly?

Yes, you are right, that was just an oversight. We don't need to store the actor as a property on the key, that was the whole point of this refactor! :-)

Comment 19

[Eddy Bruel [:ejpbruel]]

(In reply to Eddy Bruel [:ejpbruel] from comment #18)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #15)
> > Comment on attachment 8526245 [details] [diff] [review]
> > Step 6. Refactor ThreadActor.prototype.addBreakpoint
> > 
> > Review of attachment 8526245 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Really like the spirit of this change, but want the API cleaned up a little.
> > Want to take a quick look at it again once you've updated it, before I
> > rubber stamp it.
> > 
> > ::: toolkit/devtools/server/actors/script.js
> > @@ +113,3 @@
> > >      let { url, line, column } = aBreakpoint;
> > >  
> > > +    aBreakpoint.actor = aActor;
> > 
> > Either use the actor property we document `aBreakpoint` to optionally (no
> > longer optional?) have, or remove that property from usage and the docs.
> > 
> > We shouldn't have both an actor parameter and an actor property.
> > 
> > In fact, could this take only a breakpoint actor, since the breakpoint actor
> > has its location as a public property that we can just use directly?
> 
> Yes, you are right, that was just an oversight. We don't need to store the
> actor as a property on the key, that was the whole point of this refactor!
> :-)

Actually, this was deliberate. I still have to store the actor as a property on the key at this point, because getBreakpoint still returns the key instead of the actor. Step 7 addresses that problem, so at that point I can avoid redundantly storing the actor on the key as well as separately.

I want to keep the location to which the actor is to be set as a separate argument for now, because we're going to end up having a N:1 relationship between locations and actors in the case of generated locations, and it's not yet clear how the API will evolve. For now, sticking as close to the Map interface as possible seemed like the simplest/cleanest thing to do.

Since all your review comments are either already addressed in the next patch, or in the case of the location argument, would be best left untouched for now, could you r+ it? I could put up the exact same patch again, but that wouldn't be very helpful ;-)

Comment 20

[Eddy Bruel [:ejpbruel]]

Attachment: Step 8. Rename BreakpointStore to BreakpointActorMap

I've held off from putting the next patches up for review because I wanted to make sure you liked the prerequisite changes first.

This patch renames BreakpointStorage to BreakpointActorMap, changes the method names to more closely resemble that of Map, and also improves the comments a bit while we're at it.

The idea behind this change is that instead of a single BreakpointStorage, we will end up having two BreakpointActorMaps. One from original locations to actors (i.e. the one we currently have), which is 1:1, and one from generated locations to actors, which is N:1.

Comment 21

[Eddy Bruel [:ejpbruel]]

Attachment: Step 9. Refactor BreakpointActorMap to use colspans internally

This patch refactors BreakpointActorMap to use colspans internally. This change is necessary because we eventually want to map generated colspans instead of locations to actors.

This change has the additional advantage that it allows us to get rid of the distinction between whole line breakpoints and column breakpoints. A whole line breakpoint is equivalent to a breakpoint with a column span [0, Infinity]. A column breakpoint is equivalent to a breakpoint with a column span [column, column + 1] (Note that the latter will change to [beginColumn, endColumn] once we can query source maps for the column span of each mapping).

Note that the interface still requires locations rather than colspans to be passed as keys. The actual colspan is then computed from the location using the above observation. Changing the interface would require several callers to be updated as well, so I will do that in a future patch.

Comment 22

[Eddy Bruel [:ejpbruel]]

Another reason I didn't update the interface yet is that I couldn't really come up with a good name for the concept of url + line + column span. In my mind, a column span is simply a range of columns without any additional location information, so using colspan for the former seems like a misnomer. I can't come up with anything better though.

Nick, do you have a good name in mind? Or should we just use 'colspan', even though that isn't really accurate?

Comment 23

[Eddy Bruel [:ejpbruel]]

Attachment: Step 1 - 7. Rebased patches

I had to do a fairly substantial rebase due to Debugger.Source landing, so I wanted to give you the opportunity to look over the rebased patches one more time before landing them.

Comment 24

[Eddy Bruel [:ejpbruel]]

Attachment: Step 8. Clean up the BreakpointStore interface (rebased)

Comment 25

[Eddy Bruel [:ejpbruel]]

Attachment: Step 9. Clean up the BreakpointActorMap implementation (rebased)

Comment 26

[Jim Blandy :jimb]

Comment on attachment 8529614 [details] [diff] [review]
Step 1 - 7. Rebased patches

Review of attachment 8529614 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/script.js
@@ +106,5 @@
>     *          - actor (optional)
>     * @returns Object aBreakpoint
>     *          The new or existing breakpoint.
>     */
> +  addBreakpoint: function (aBreakpoint, aActor) {

If I'm reading the code right, we are now careful to check that no actor exists before calling this function. Indeed, if there is an existing actor, we don't do anything at all with aActor, so the caller would need to be sure to remove it from the pool; the callers we have now don't.

So, would it make sense for this function to dbg_assert that there is no extant actor? Granted, that's a deviation from the 'Map'-like interface, but it seems like the 'replace existing' behavior is never actually desired.

Comment 27

[Jim Blandy :jimb]

Comment on attachment 8529615 [details] [diff] [review]
Step 8. Clean up the BreakpointStore interface (rebased)

Review of attachment 8529615 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/script.js
@@ +57,5 @@
>    };
>  };
>  
>  /**
> + * A BreakpointActorMap is a map from locations to instances of BreakpointActor.

I think "instances of" can go here, and elsewhere.

@@ +92,5 @@
> +   * BreakpointActorMap.
> +   *
> +   * @returns Number
> +   *          The number of instances of BreakpointACtor in this instance of
> +   *          BreakpointActorMap.

Yeah, let's drop "instance" as much as possible. It's totally okay to use the category noun ("BreakpointActorMap"; "BreakpointActor") to refer to instances of the category.

@@ +177,5 @@
> +   * @param BreakpointActor actor
> +   *        The instance of BreakpointActor to be set to the given location.
> +   */
> +  setActor: function (location, actor) {
> +    let { source, line, column } = location;

Do we want a dbg_assert(source.actor) here, to avoid creating entries named "undefined" in the case of a bug which would be a compile-time error in a reasonable language?

@@ +208,5 @@
>    },
>  
>    /**
> +   * Delete the instance of BreakpointActor from the given location in this
> +   * instance of BreakpointActorMap.

Instances everywhere!

Comment 28

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/fx-team/rev/49218eb557ad

Comment 29

[Carsten Book [:Tomcat]]

(In reply to Eddy Bruel [:ejpbruel] from comment #28)
> https://hg.mozilla.org/integration/fx-team/rev/49218eb557ad

sorry had to back this out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1343284&repo=fx-team

Comment 30

[Eddy Bruel [:ejpbruel]]

Sorry about that, this patch should work, I forgot to include a single line fix.

This patch should work:
https://hg.mozilla.org/integration/fx-team/rev/8c23d884cd91

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c23d884cd91

Comment 32

[Eddy Bruel [:ejpbruel]]

Attachment: Step 10. Use source actors instead of source forms in locations

The BreakpointStore uses locations with a source property, representing a source form. In most other places, we use locations with a sourceActor property, representing a source actor. This inconsistenty leads to confusion, so this patch refactors the breakpoint store to use the latter form as well.

Comment 33

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

Comment on attachment 8529616 [details] [diff] [review]
Step 9. Clean up the BreakpointActorMap implementation (rebased)

Review of attachment 8529616 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/script.js
@@ +61,5 @@
>   * A BreakpointActorMap is a map from locations to instances of BreakpointActor.
>   */
>  function BreakpointActorMap() {
>    this._size = 0;
> +  this._actors = Object.create(null);

We had nice comments describing the form of the data we stored before, and I'd love to continue having a comment here. Something that tells me what the object keys are and what the form of the values are.

@@ +96,5 @@
>          }
> +      }
> +      else {
> +        for (let key of Object.keys(object)) {
> +          yield key;

Instead of for/of + yield each item manually, you can just do:

  yield* Object.keys(object);

@@ +102,5 @@
>        }
>      }
> +
> +    query.actor = query.source ? query.source.actor : undefined;
> +    query.beginColumn = query.column ? query.column : undefined;

This can just be

  query.beginColumn = query.column;

@@ +103,5 @@
>      }
> +
> +    query.actor = query.source ? query.source.actor : undefined;
> +    query.beginColumn = query.column ? query.column : undefined;
> +    query.endColumn = query.column ? query.column + 1 : undefined;

Rather than modifying the in param `query`, let's just declare these as variables.

@@ +151,5 @@
>    setActor: function (location, actor) {
>      let { source, line, column } = location;
>  
> +    let beginColumn = column ? column : 0;
> +    let endColumn = column ? column + 1 : Infinity;

What if column is 0? Shouldn't these tests be `column != null`?

@@ +184,3 @@
>  
> +    let beginColumn = column ? column : 0;
> +    let endColumn = column ? column + 1 : Infinity;

column != null?

@@ +193,5 @@
> +          }
> +          delete this._actors[source.actor][line][beginColumn][endColumn];
> +          if (Object.keys(this._actors[source.actor][line][beginColumn]).length === 0) {
> +            delete this._actors[source.actor][line][beginColumn];
> +          }

I think this stuff can (and should?) be moved into the block above that is decrementing this._size.

Comment 34

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

Comment on attachment 8532127 [details] [diff] [review]
Step 10. Use source actors instead of source forms in locations

Review of attachment 8532127 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/script.js
@@ +2948,5 @@
>      }
>  
>      const { line, entryPoints } = result;
>      const actualLocation = line !== location.line
> +          ? { sourceActor: this, line }

funky indent

Comment 35

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/fx-team/rev/880899731497

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/880899731497

Comment 37

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/fx-team/rev/e3cc96211c46

Comment 38

[Eddy Bruel [:ejpbruel]]

Try run for step 10:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=799ba1f19719

Comment 39

[Eddy Bruel [:ejpbruel]]

Ugh, for some reason that try push contains an empty patch :-S

Second attempt:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d65a044c02a

Comment 40

[Eddy Bruel [:ejpbruel]]

Try looks green, pushing to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/f82edb30c66d

Comment 41

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f82edb30c66d