1131643 - Implement a Location object.

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Eddy Bruel [:ejpbruel]]

Attachment: patch

At the moment, we are being rather vague about what a Location object is. Sometimes they contain a sourceActor, at other times a Debugger.Source instance. Sometimes they contain a url property and sometimes they don't, etc.

This patch implements a Location object that offers several advantages over using plain objects for locations:
- Make sure we never store source actors directly, but only their id
- Helper functions for obtaining the source actor from a location object, as well
  as other derived properties such as the source form or url
- Helper functions for basic operations such as comparing locations and converting
  a Location object to a JSON object for use in response packets.
- By always freezing Location objects we guarantee that we'll never run into some
  of the shared state bugs we had with locations in the past.

Comment 1

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

Idea: can we split Location into `CodeLocation`(ie a location in the code spidermonkey is actually executing) and `SourceLocation` (ie a location in the original source) and then anything that gets sent across the RDP has to be a `SourceLocation`?

Would be nice to have a "type system" (lol js) to ensure that we are doing the correct thing.

Comment 2

[James Long (:jlongster)]

(In reply to Nick Fitzgerald [:fitzgen] from comment #1)
> Idea: can we split Location into `CodeLocation`(ie a location in the code
> spidermonkey is actually executing) and `SourceLocation` (ie a location in
> the original source) and then anything that gets sent across the RDP has to
> be a `SourceLocation`?
> 
> Would be nice to have a "type system" (lol js) to ensure that we are doing
> the correct thing.

Not sure I follow. Would `CodeLocation` correlate to the generated location if the source is sourcemapped?

Comment 3

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

Yes, the code SpiderMonkey is actually executing.

The worst thing I've ever named is "original" and "generated" (at least, I think it was me that named that...).

I find it much more straightforward to use "source" and "(object) code"

Comment 4

[James Long (:jlongster)]

That's not a bad idea. It would certainly make it more clear in the code which is which. Just make the current `Location` a base class and then inherit it in these two types? Then we could also add several asserts throughout the code to make sure they are passing the right type of location...

Comment 5

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

We wouldn't need to manually add asserts if their property sets were not the same:

    CodeLocation { codeFile, codeLine, codeColumn }
    SourceLocation { sourceFile, sourceLine, sourceColumn }

It just wouldn't work if you tried the wrong thing, which is good and catches bugs early.

Comment 6

[Eddy Bruel [:ejpbruel]]

I like your idea Nick. I was thinking about something similar myself.

First, I agree that code vs source is clearer than original vs generated, but I don't want to start using those terms interchangeably. That would be even more confusing. If you think we should change those names we should do it all in one go in a separate bug. Agreed?

Second, if we use two different constructors, the only thing that that would be good for is that we could assert whether were using the correct kind of location using instanceof right? I'm not a big fan of using type assertions in JavaScript like that, but I do appreciate what you're trying to do here. Since the two different locations are otherwise completely the same in terms of what properties they have, how about this instead: define two derived properties on location, called isOriginalLocation and isGeneratedLocation, so we can assert on those directly. We should be able to tell if a location is an original or a generated location by checking if its source actor has an original source or not.

Comment 7

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

(In reply to Eddy Bruel [:ejpbruel] from comment #6)
> If you think we should change those names we should do
> it all in one go in a separate bug. Agreed?

Yep.

> Second, if we use two different constructors, the only thing that that would
> be good for is that we could assert whether were using the correct kind of
> location using instanceof right? I'm not a big fan of using type assertions
> in JavaScript like that, but I do appreciate what you're trying to do here.

No, this wouldn't require us lazy and frequently-making-mistakes-and-forgetting-things programmers to manually write assertions or use `instanceof`. It would be _incredible_ if we could write code that doesn't require us to check things all the time, like how SpiderMonkey will only accept rooted params via Handle<T>, etc. If we can get safety/correctness without requiring us faulty programmers to constantly think about it and double check, that is a huge win.

Consider how the following requires no manual assertions at each place where it is _used_ (only the one place where it is defined):

  OriginalLocation {
    originalLine,
    originalColumn,
    originalSource,

    get generatedLine() { throw new Error() },
    get generatedColumn() { throw new Error() },
    get generatedSource() { throw new Error() }
  }

We could also just make Location have both generated and original locations in the same object so that you can't just say `location.line` but choose either `location.generatedLine` or `location.originalLine`, which is an improvement to what we have now (although not as good as the above snippet, IMO).

> Since the two different locations are otherwise completely the same in terms
> of what properties they have, how about this instead: define two derived
> properties on location, called isOriginalLocation and isGeneratedLocation,
> so we can assert on those directly. We should be able to tell if a location
> is an original or a generated location by checking if its source actor has
> an original source or not.

The downside is that this still allows the wrong type of location to be used and you have to remember to `assert(isXLocation)` at every place that uses these location objects, rather than just the one time in the definition.

Comment 8

[Eddy Bruel [:ejpbruel]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #7)
> (In reply to Eddy Bruel [:ejpbruel] from comment #6)
> > If you think we should change those names we should do
> > it all in one go in a separate bug. Agreed?
> 
> Yep.
> 
> > Second, if we use two different constructors, the only thing that that would
> > be good for is that we could assert whether were using the correct kind of
> > location using instanceof right? I'm not a big fan of using type assertions
> > in JavaScript like that, but I do appreciate what you're trying to do here.
> 
> No, this wouldn't require us lazy and
> frequently-making-mistakes-and-forgetting-things programmers to manually
> write assertions or use `instanceof`. It would be _incredible_ if we could
> write code that doesn't require us to check things all the time, like how
> SpiderMonkey will only accept rooted params via Handle<T>, etc. If we can
> get safety/correctness without requiring us faulty programmers to constantly
> think about it and double check, that is a huge win.
> 
> Consider how the following requires no manual assertions at each place where
> it is _used_ (only the one place where it is defined):
> 
>   OriginalLocation {
>     originalLine,
>     originalColumn,
>     originalSource,
> 
>     get generatedLine() { throw new Error() },
>     get generatedColumn() { throw new Error() },
>     get generatedSource() { throw new Error() }
>   }
> 
> We could also just make Location have both generated and original locations
> in the same object so that you can't just say `location.line` but choose
> either `location.generatedLine` or `location.originalLine`, which is an
> improvement to what we have now (although not as good as the above snippet,
> IMO).
> 
> > Since the two different locations are otherwise completely the same in terms
> > of what properties they have, how about this instead: define two derived
> > properties on location, called isOriginalLocation and isGeneratedLocation,
> > so we can assert on those directly. We should be able to tell if a location
> > is an original or a generated location by checking if its source actor has
> > an original source or not.
> 
> The downside is that this still allows the wrong type of location to be used
> and you have to remember to `assert(isXLocation)` at every place that uses
> these location objects, rather than just the one time in the definition.

Jup, I love that idea. I'll file a new patch tomorrow that does it like this.

Comment 9

[Eddy Bruel [:ejpbruel]]

Attachment: patch

Here's a new patch that incorporates fitzgen's idea. This makes the code a lot clearer in my opinion. Let me know what you think!

Comment 10

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

Comment on attachment 8563414 [details] [diff] [review]
patch

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

::: toolkit/devtools/server/actors/common.js
@@ +377,5 @@
> +  },
> +
> +  get generatedColumn() {
> +    return this._column;
> +  }

So you decided not to add getters that throw assertion errors for originalX on GeneratedLocation and generatedX on OriginalLocation? Without those, accessing a bad property will result in `undefined` and propogate through the code until somewhere further down the line it results in an error.

Comment 11

[Eddy Bruel [:ejpbruel]]

Attachment: patch

Added those getters that Nick wanted because nobody wants a sad Nick.

Comment 12

[Eddy Bruel [:ejpbruel]]

Try run for this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdd0e050557b

Comment 13

[Eddy Bruel [:ejpbruel]]

James, could you review this patch please?

Comment 14

[James Long (:jlongster)]

Comment on attachment 8564160 [details] [diff] [review]
patch

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

r+ with a few nits, if you fix them and explain why some of the code was removed

::: toolkit/devtools/server/actors/common.js
@@ +339,5 @@
> +    return this._column;
> +  },
> +
> +  get generatedSourceActor() {
> +    throw new Error();

can you put error messages in all of these errors? just something like "generated actor not available from an original location"

@@ +384,5 @@
> +    throw new Error();
> +  },
> +
> +  get originalUrl() {
> +    throw new Error();

ditto

::: toolkit/devtools/server/actors/script.js
@@ +2747,5 @@
>      // Debugger.Script -> array of offset mappings
>      const scriptsAndOffsetMappings = new Map();
>  
>      for (let script of scripts) {
> +      this._findClosestOffsetMappings(generatedLocation, script, scriptsAndOffsetMappings);

nit: long line, can you put the arguments on separate lines? The style I like when function calls get too long is:

this._findClosestOffsetMappings(generatedLocation,
                                script,
                                scriptsAndOffsetMappings);

@@ +2940,5 @@
>        }
>      }
>  
>      return Promise.resolve().then(() => {
> +      if (actualGeneratedLocation.generatedSourceActor.source) {

So verbose, but I like it :) The verbosity helps this code a lot.

@@ +3320,5 @@
> +      return {
> +        source: originalLocation.originalSourceActor.form(),
> +        line: originalLocation.originalLine,
> +        column: originalLocation.originalColumn
> +      };

Would it be confusing to add a `form` on `OriginalLocation` so that you could just call that here? Are should `form` really only ever be on actors?

@@ +4554,4 @@
>        form.where = {
> +        source: generatedLocation.generatedSourceActor.form(),
> +        line: generatedLocation.generatedLine,
> +        column: generatedLocation.generatedColumn

Another place we could use `form` on the location objects, unless you think those should really only be for actors

@@ -5603,5 @@
> -          }),
> -          url: sourceUrl,
> -          line: sourceLine,
> -          column: sourceCol,
> -          name: sourceName

Why did this have the `name` property? You dropped this in your new code. I remember having to do this for some reason, are you aware of it and are sure it's ok to not use any more?

(I actually don't even see where sourceName is declared...)

::: toolkit/devtools/server/actors/tracer.js
@@ -353,5 @@
>        }
>  
>        if (this._requestsForTraceType.name) {
> -        if (sourceMappedLocation && sourceMappedLocation.name) {
> -          packet.name = sourceMappedLocation.name;

Oh, here is where it uses name. Why do you remove this code?

Comment 15

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

> Another place we could use `form` on the location objects, unless you think those should really only be for actors

A "form" is an RDP-ism: the representation of the actor over the protocol. We shouldn't conflate our terms.

Comment 16

[Eddy Bruel [:ejpbruel]]

(In reply to James Long (:jlongster) from comment #14)
> 
> @@ +3320,5 @@
> > +      return {
> > +        source: originalLocation.originalSourceActor.form(),
> > +        line: originalLocation.originalLine,
> > +        column: originalLocation.originalColumn
> > +      };
> 
> Would it be confusing to add a `form` on `OriginalLocation` so that you
> could just call that here? Are should `form` really only ever be on actors?
> 

I tried adding a toJSON method to the Location objects. The problem with that is that the breakpoint-actor-map-test.js doesn't use proper location objects (it uses dummy objects instead of proper actors, so we can't pass an actor ID to the location constructor), and I haven't figured out yet how to work around that cleanly. I might fix this in a followup bug.

> 
> @@ -5603,5 @@
> > -          }),
> > -          url: sourceUrl,
> > -          line: sourceLine,
> > -          column: sourceCol,
> > -          name: sourceName
> 
> Why did this have the `name` property? You dropped this in your new code. I
> remember having to do this for some reason, are you aware of it and are sure
> it's ok to not use any more?
> 

I removed this because I wanted to make the Location objects more homogeneous, and it didn't seem to be actually used (removing the code didn't cause any tests to fail). However, now that I've thought about it some more, I can see the point of adding a name property to OriginalLocation, so I've readded the code.

I'm still not completely happy with this though, since a name technically isn't part of a location and is only ever used for display purposes in the tracer. I might come up with a cleaner way to do this in the future, but for now, let's just leave it as is.

Comment 17

[Eddy Bruel [:ejpbruel]]

Try push looks green. Pushing to fx-team with comments by jlong addressed. Have not done another try push since I've only added some strings, and re-added code that was there before. Made sure tests were passing locally before pushing though:
https://hg.mozilla.org/integration/fx-team/rev/c02c4fcfeb78

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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