Closed Bug 1131643 Opened 7 years ago Closed 7 years ago

Implement a Location object.

Categories

(DevTools :: Debugger, defect)

x86
macOS
defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: ejpbruel, Assigned: ejpbruel)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8562132 - Flags: review?(jlong)
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.
(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?
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"
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...
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.
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.
(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.
(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.
Attached patch patch (obsolete) — Splinter Review
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!
Attachment #8562132 - Attachment is obsolete: true
Attachment #8562132 - Flags: review?(jlong)
Attachment #8563414 - Flags: review?(jlong)
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.
Attached patch patchSplinter Review
Added those getters that Nick wanted because nobody wants a sad Nick.
Attachment #8563414 - Attachment is obsolete: true
Attachment #8563414 - Flags: review?(jlong)
Attachment #8564160 - Flags: review?(jlong)
James, could you review this patch please?
Flags: needinfo?(jlong)
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?
Attachment #8564160 - Flags: review?(jlong) → review+
> 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.
(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.
Flags: needinfo?(jlong)
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
https://hg.mozilla.org/mozilla-central/rev/c02c4fcfeb78
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Target Milestone: Firefox 38 → Firefox 39
Depends on: 1327974
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.