Closed Bug 860035 Opened 11 years ago Closed 11 years ago

dbg-script-actors.js should handle breakpoints with columns

Categories

(DevTools :: Debugger, defect, P2)

Product:
DevTools
Component:
Debugger
Platform:
x86
macOS
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: fitzgen, Assigned: fitzgen)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 9 obsolete files)

Generate source notes for debugger statement
726 bytes, patch
till
: review+
Details | Diff | Splinter Review
v4.2
73.92 KB, patch
jimb
: review+
Details | Diff | Splinter Review 
Right now, we accept a column over the protocol, and jsdbg2 provides facilities for setting breakpoints in columns, but we throw away column information in TA__setBreakpoint.
QA Contact: nfitzgerald
This seems like a dupe of bug 676602 (the oldest open debugger bug!), no?
Assignee: nobody → nfitzgerald
QA Contact: nfitzgerald
(In reply to Panos Astithas [:past] from comment #1)
> This seems like a dupe of bug 676602 (the oldest open debugger bug!), no?

From that bug:

> We should provide a way for the debugger UI to accomplish that.

But this is just for the RDP, not for any kind of UI.

Still think this is a dup? Maybe that one should depend on this one?
I'm OK with doing the backend work here and having this bug block that one.
Attached patch wip (obsolete) — — Splinter Review 
Got the test I wrote for setting breakpoints in columns working, but regressed other tests and can't get breakpoints in minified js working.

Jim, I am getting |Debugger.Frame|s that claim to have a certain offset, but when I try to find that offset's column number via that frame's script and |getAllColumnOffsets| (as a shim till we have |getOffsetColumn|) I can't find the offset again. Any ideas why that might be? See |FrameActor.prototype._getOffsetColumn|.

Thanks!
Flags: needinfo?(jimb)
(In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> Created attachment 738896 [details] [diff] [review]
> wip
> 
> Got the test I wrote for setting breakpoints in columns working, but
> regressed other tests and can't get breakpoints in minified js working.
> 
> Jim, I am getting |Debugger.Frame|s that claim to have a certain offset, but
> when I try to find that offset's column number via that frame's script and
> |getAllColumnOffsets| (as a shim till we have |getOffsetColumn|) I can't
> find the offset again. Any ideas why that might be? See
> |FrameActor.prototype._getOffsetColumn|.
> 
> Thanks!

Oh, there's no doubt that frames might mention bytecode offsets that aren't mentioned *exactly* by the source info. The JavaScript shell has a 'dis' function that will show you bytecodes and their "source notes", giving the positions:

js> dis(function() {    return 3 + f() + 4; })
flags: LAMBDA
loc     op
-----   --
main:
00000:  int8 3
00002:  callgname "f"
00007:  undefined
00008:  notearg
00009:  call 0
00012:  add
00013:  int8 4
00015:  add
00016:  return
00017:  stop

Source notes:
 ofs line    pc  delta desc     args
---- ---- ----- ------ -------- ------
  0:    1     0 [   0] colspan 20
  2:    1    17 [  17] xdelta  
  3:    1    17 [   0] colspan 19

js> 

(I stuck some spaces in the function body so that the number "17" doesn't appear as both a bytecode offset and as a source column number, which it did in my first attempt...)

The source notes dump is a direct dump of the source notes array, which makes it a little hard to decipher. But I'm pretty sure this says:
bytecode offsets 0 < x <= 17 belong to line 1, column 20
bytecode offsets 17 < x belong to line 1 column 39 (20+19).

So what that really means is that bytecodes at 0-17 span columns 20-39, which is the entire 'return' statement.

The point is, though, that if you stop in f somewhere, and then walk up the stack, you'll see the anonymous function's frame with a bytecode offset of 9, pointing to the call instruction. (Or maybe 12. Not sure.) This is certainly a bytecode offset never mentioned in the source notes, and thus Debugger.Script's methods will never mention it.
Flags: needinfo?(jimb)
Comment on attachment 738896 [details] [diff] [review]
wip

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

Great stuff. This is a hairy problem.

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +264,5 @@
>     * @param function onPacket
>     *        Hook to modify the packet before it is sent. Feel free to return a
>     *        promise.
>     */
> +  _pauseAndRespond: function TA__pauseAndRespond(aFrame, aReason) {

(don't forget to update the docs)

@@ +527,5 @@
>            line: originalLine,
>            column: originalColumn } = aRequest.location;
>  
> +    // If we are given a column, we will try and break only at that location,
> +    // otherwise we will break anytime we get on that line.

This seems like the right behavior, but it's not what the protocol spec says; "Source Locations" says that omitted line and column numbers are equal to 1. We'll need an accompanying doc patch describing the actual behavior.

@@ +528,5 @@
>            column: originalColumn } = aRequest.location;
>  
> +    // If we are given a column, we will try and break only at that location,
> +    // otherwise we will break anytime we get on that line.
> +    let breakWholeLine = originalColumn == null;

originalColumn is actually 'undefined' if the property is omitted; this only works because undefined == null. It would be nicer to write '== undefined'.

@@ +540,1 @@
>      return locationPromise.then((aLocation) => {

Would it be good to just use the destructuring syntax in the => heads here?

@@ +543,2 @@
>            line < 0 ||
>            line == null) {

Might be nice to check the cheap conditions (line < 0, line == null) before the expensive condition (findScripts().length == 0).

@@ +600,5 @@
>     *
>     * @param object aLocation
>     *        The location of the breakpoint as specified in the protocol.
> +   * @param boolean aBreakWholeLine
> +   *        Whether we want to break on any offset on the location's line, or if

"on any offset on" isn't right; you mean, "at every entry point of". 

Wouldn't it make sense to drop aBreakWholeLine, and just use the presence of the 'column' property on aLocation to select the behavior? Then the caller could be simplified, too.

@@ +708,5 @@
>    },
>  
> +  _getOffsetMappingsForLine: function TA__getOffsetsForLine(aLine, aScript) {
> +    return aScript.getAllColumnOffsets()
> +      .filter((offsetMapping) => offsetMapping.lineNumber === aLine);

This could be:
     .filter(({lineNumber}) => lineNumber === aLine);

DESTRUCTURE ALL THE THINGS

@@ +711,5 @@
> +    return aScript.getAllColumnOffsets()
> +      .filter((offsetMapping) => offsetMapping.lineNumber === aLine);
> +  },
> +
> +  _findClosestOffsetMappings: function TA__findClosestOffsetMappings(aTargetColumn, aOffsetMappings) {

Would it make sense to just have one function that does the composition of _findClosestOffsetMappings with _getOffsetMappingsForLine? It seems like they'd always be used in conjunction, and that might make it easier for readers to see what sorts of values aTargetColumn and aOffsetMappings are. (They wouldn't be arguments any more.) The combined function would take aLocation as an argument, and do the whole-line/specific-column determination internally.

I'm feeling myopic in this code, though, so if that doesn't fit with other stuff, then don't worry about it.

@@ +716,5 @@
> +    let closestMappings = [];
> +    let closestDistance = Infinity;
> +
> +    for (let mapping of aOffsetMappings) {
> +      let currentDistance = Math.abs(aTargetColumn - mapping.columnNumber);

I kind of think we should be using "closest preceding", not "closest".

@@ +1996,5 @@
> +    dump("FITZGEN: aOffset = " + aOffset + "\n");
> +    let offsetMapping = aScript.getAllColumnOffsets()
> +      .filter((offsetMapping) => {
> +        dump("FITZGEN: mapping = " + JSON.stringify(offsetMapping) + "\n");
> +        return offsetMapping.offset === aOffset;

Yeah, you're going to get offsets in frames that aren't mentioned directly in the mapping. You need to treat the mapping as showing transitions: "at this offset, the source location changed to this". So you want the closest preceding offset, I guess.

@@ +2012,5 @@
> +      }
> +    }
> +
> +    if (aThrowIfMissing) {
> +      throw new TypeError("The specified offset is invalid.");

I take it this was getting thrown when you didn't expect?
(In reply to Jim Blandy :jimb from comment #6)
> @@ +716,5 @@
> > +    let closestMappings = [];
> > +    let closestDistance = Infinity;
> > +
> > +    for (let mapping of aOffsetMappings) {
> > +      let currentDistance = Math.abs(aTargetColumn - mapping.columnNumber);
> 
> I kind of think we should be using "closest preceding", not "closest".
> 

What do we want to do if there are only mappings after our target column? It shouldn't put a breakpoint on the next line if there are still mappings on this line, it should put it on the closest suceeding mapping then, right?

With this patch currently, we prefer (in order):

1. closest preceeding or succeeding mappings (whichever are closest)
2. mappings on the next line

Do you believe we should prefer (in order):

1. closest preceding mappings
2. closest succeeding mappings
3. mappings on the next line
Status: NEW → ASSIGNED
Attached patch wip 2 (obsolete) — — Splinter Review 
This patch would work perfectly and both tests would pass, except that the frame at depth 0 doesn't seem to ever have column information. Because of that, we can't properly source map back to the original location, and browser_dbg_source_map-03.js is failing.

Can someone give me some info on why this is? Bug? Expected behavior? Help!
Attachment #738896 - Attachment is obsolete: true
Flags: needinfo?(jimb)
Flags: needinfo?(ejpbruel)
Priority: -- → P2
Looks like this problem isn't specific to frame level 0, but rather to the debugger statement (which, of course, always occurs in frame level 0).

The problem is that we don't generate source notes for debugger statements like we do for other statements, because they are not fully fledged statements from the perspective of the emitter (they do not occur inside a PNK_SEMI node like other statements).

The solution is simply to explicitly generate source notes for the JSOP_DEBUGGER opcode.

Flagging Till for review because the patch is trivial, he knows his way around the frontend, and I don't want to unnecessarily put more work on Jason's plate.
Assignee: nfitzgerald → ejpbruel
Attachment #751843 - Flags: review?(tschneidereit)
Flags: needinfo?(ejpbruel)
Comment on attachment 751843 [details] [diff] [review]
Generate source notes for debugger statement

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

Yup, that makes total sense.
Attachment #751843 - Flags: review?(tschneidereit) → review+
Umm, maybe that stuff should go in its own bug, since this bug is more about having ThreadActors accept columns as well as lines for breakpoints, and I don't think my patch/review/etc should slow down your fix from landing, Eddy.
Flags: needinfo?(jimb)
(In reply to Nick Fitzgerald [:fitzgen] from comment #11)
> Umm, maybe that stuff should go in its own bug, since this bug is more about
> having ThreadActors accept columns as well as lines for breakpoints, and I
> don't think my patch/review/etc should slow down your fix from landing, Eddy.

We should be able to land this with [leave-open] in the whiteboard, right?
(In reply to Eddy Bruel [:ejpbruel] from comment #12)
> We should be able to land this with [leave-open] in the whiteboard, right?

Yeah, I suppose so.
Whiteboard: leave-open
I don't know if the whiteboard tag works without brackets, so ...
Whiteboard: leave-open → [leave open]
Attached patch v1 (obsolete) — — Splinter Review 
https://tbpl.mozilla.org/?tree=Try&rev=67691207b492

* Did not change to using closest preceding offset because Jim hasn't responded to my question, and because I'm not yet convinced this is actually what we want to do. What if the closest preceding is 100 columns away, but closest succeeding is only 1 column away? It seems that the correct thing to do is to set the breakpoint on the succeeding offset. I think just using the closest offset is the best approach.

* Changed up the way we set breakpoints a bit more. Can't set breakpoints on every script on the line anymore because every script is on the same line. Hopefully these changes are self evident when you read the code, if not I can add more comments.
Attachment #749103 - Attachment is obsolete: true
Attachment #752278 - Flags: review?(past)
Attachment #752278 - Flags: review?(jimb)
Comment on attachment 752278 [details] [diff] [review]
v1

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

I couldn't play with it, because Dave has bitrotted it. Needs rebasing.

(In reply to Nick Fitzgerald [:fitzgen] from comment #17)
> * Did not change to using closest preceding offset because Jim hasn't
> responded to my question, and because I'm not yet convinced this is actually
> what we want to do. What if the closest preceding is 100 columns away, but
> closest succeeding is only 1 column away? It seems that the correct thing to
> do is to set the breakpoint on the succeeding offset. I think just using the
> closest offset is the best approach.

I think we'll need to see how this works in practice first. I've never used a debugger frontend that allowed me to set breakpoints in columns before and I'm not sure how well offset locations map to user intuition. From an abstract POV I think your choice makes the most sense.

> * Changed up the way we set breakpoints a bit more. Can't set breakpoints on
> every script on the line anymore because every script is on the same line.
> Hopefully these changes are self evident when you read the code, if not I
> can add more comments.

Does that mean that until the frontend changes to accommodate column breakpoints we will have regressed bug 793214 (test_breakpoint-11.js)? I think the server should cope with clients that omit column numbers and still do the right thing in those cases.

::: browser/devtools/debugger/test/browser_dbg_source_maps-03.js
@@ +107,5 @@
> +  }, 100);
> +}
> +
> +registerCleanupFunction(function() {
> +  Services.prefs.setBoolPref("devtools.debugger.source-maps-enabled", false);

It would be more future-proof to revert to the previous value whatever that was. We may change the defaults in the future and this would impact the following tests.

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +521,3 @@
>      }
>  
>      // XXX: `originalColumn` is never used. See bug 827639.

Is this comment still valid?
Attachment #752278 - Flags: review?(past)
(In reply to Panos Astithas [:past] from comment #18)
> Comment on attachment 752278 [details] [diff] [review]
> v1
> 
> Review of attachment 752278 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I couldn't play with it, because Dave has bitrotted it. Needs rebasing.

Working on it now.

> (In reply to Nick Fitzgerald [:fitzgen] from comment #17)
> > * Did not change to using closest preceding offset because Jim hasn't
> > responded to my question, and because I'm not yet convinced this is actually
> > what we want to do. What if the closest preceding is 100 columns away, but
> > closest succeeding is only 1 column away? It seems that the correct thing to
> > do is to set the breakpoint on the succeeding offset. I think just using the
> > closest offset is the best approach.
> 
> I think we'll need to see how this works in practice first. I've never used
> a debugger frontend that allowed me to set breakpoints in columns before and
> I'm not sure how well offset locations map to user intuition. From an
> abstract POV I think your choice makes the most sense.

So this patch won't add any frontend ability to set breakpoints at specific columns, just protocol/server support which is needed when source mapping.

> > * Changed up the way we set breakpoints a bit more. Can't set breakpoints on
> > every script on the line anymore because every script is on the same line.
> > Hopefully these changes are self evident when you read the code, if not I
> > can add more comments.
> 
> Does that mean that until the frontend changes to accommodate column
> breakpoints we will have regressed bug 793214 (test_breakpoint-11.js)? I
> think the server should cope with clients that omit column numbers and still
> do the right thing in those cases.

Well the test didn't fail locally or in my try pushes, so I'd say no ;)

Actual answer: we have different behavior based on if you specify a column or not. If you don't specify a column, you should get the existing, correct behavior. If you do specify a column, then the whole premise of that bug is invalidated.

> ::: browser/devtools/debugger/test/browser_dbg_source_maps-03.js
> @@ +107,5 @@
> > +  }, 100);
> > +}
> > +
> > +registerCleanupFunction(function() {
> > +  Services.prefs.setBoolPref("devtools.debugger.source-maps-enabled", false);
> 
> It would be more future-proof to revert to the previous value whatever that
> was. We may change the defaults in the future and this would impact the
> following tests.

Good call!

> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +521,3 @@
> >      }
> >  
> >      // XXX: `originalColumn` is never used. See bug 827639.
> 
> Is this comment still valid?

I don't think so! Good catch!

Rebased and updated patch incoming...
Attached patch v2 (obsolete) — — Splinter Review 
* Rebased

* Removed old comment

* Set pref back to what it was, rather than hard coding to false

https://tbpl.mozilla.org/?tree=Try&rev=dd0a5ed6b911
Attachment #752278 - Attachment is obsolete: true
Attachment #752278 - Flags: review?(jimb)
Attachment #756034 - Flags: review?(past)
Attachment #756034 - Flags: review?(jimb)
Comment on attachment 756034 [details] [diff] [review]
v2

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

I only have one comment, but it looks fine to me otherwise.

::: toolkit/devtools/server/actors/script.js
@@ +2085,5 @@
> +      }
> +    }
> +
> +    if (!bestOffsetMapping) {
> +      throw new Error("Could not find a column for the offset.");

Wouldn't it be better to default to something (0 or 1?) instead of throwing here? Wouldn't throwing break the debugger frontend and infuriate the user?

I'm not sure I completely understand the conditions that would trigger this error, but I expect that Debugger.Script.prototype.getOffsetColumn would fix that, right?
Attachment #756034 - Flags: review?(past) → review+
Assignee: ejpbruel → nfitzgerald
Attached patch v2.1 (obsolete) — — Splinter Review 
Changed from a throwing an error, to returning a column 0 and simultaneously reporting the error so there is a paper trail.

New try push b/c of yesterday's infrastructure issues:

https://tbpl.mozilla.org/?tree=Try&rev=4277d5076277
Attachment #756034 - Attachment is obsolete: true
Attachment #756034 - Flags: review?(jimb)
Attachment #756793 - Flags: review?(jimb)
Comment on attachment 756793 [details] [diff] [review]
v2.1

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

Just a few comments so far... still reviewing...

::: browser/devtools/debugger/test/browser_dbg_source_maps-01.js
@@ +150,5 @@
>    }, 100);
>  }
>  
>  registerCleanupFunction(function() {
> +  Services.prefs.setBoolPref("devtools.debugger.source-maps-enabled", gSourceMapsPref);

Should we be using PushPrefEnv here instead?

https://developer.mozilla.org/en-US/docs/Mochitest

::: browser/devtools/debugger/test/browser_dbg_source_maps-03.js
@@ +35,5 @@
> +}
> +
> +function testMinJSAndSourceMaps() {
> +  gDebugger.addEventListener("Debugger:SourceShown", function _onSourceShown(aEvent) {
> +    gDebugger.removeEventListener("Debugger:SourceShown", _onSourceShown);

gDebugger is a DebuggerClient instance, right? I think that means it has an addOneTimeListener method. If so, this can become an => function.

@@ +89,5 @@
> +    closeDebuggerAndFinish();
> +  });
> +}
> +
> +function waitForCaretPos(number, callback)

This really belongs in head.js, but all those definitions in the other tests should be pulled out, too... follow-up bug?

::: toolkit/devtools/server/actors/script.js
@@ +272,5 @@
>        }
>        packet.why = aReason;
> +
> +      let { url, line, column } = packet.frame.where;
> +      this.sources.getOriginalLocation(url, line, column).then((aOrigPosition) => {

Would things read a little better if getOriginalLocation and getGeneratedLocation expected an object with 'url', 'line', and 'column' properties as an argument, instead of those three positional parameters? Hence:

this.sources.getOriginalLocation(packet.frame.where).then((aOrigPosition) => {
  packet.frame.where = aOrigPosition;
  ...

It looks like that would eliminate a lot of unpacking code...
(In reply to Jim Blandy :jimb from comment #23)his can become an => function.
> 
> @@ +89,5 @@
> > +    closeDebuggerAndFinish();
> > +  });
> > +}
> > +
> > +function waitForCaretPos(number, callback)
> 
> This really belongs in head.js, but all those definitions in the other tests
> should be pulled out, too... follow-up bug?
> 

There's bug 876277 which I hope I'll get to at some point.
Comment on attachment 756793 [details] [diff] [review]
v2.1

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

A few more comments:

::: toolkit/devtools/server/actors/script.js
@@ +599,4 @@
>  
>      // Get or create the breakpoint actor for the given location
>      let actor;
>      if (breakpoints[aLocation.line].actor) {

Don't we need to create separate breakpoint actors for breakpoints at different columns on the same line? (If that's true, we need a test that covers this case.)

@@ +604,5 @@
>      } else {
>        actor = breakpoints[aLocation.line].actor = new BreakpointActor(this, {
>          url: aLocation.url,
> +        line: aLocation.line,
> +        column: aLocation.column

Could we just use "aLocation" here instead of writing out a copy of it? Or do we need to make a copy for the "// WARNING: This overwrites aLocation.line" code below?

@@ +620,5 @@
>      }
>  
>     /**
>       * For each script, if the given line has at least one entry point, set
>       * breakpoint on the bytecode offet for each of them.

(While we're here, could you add the 'a' and missing 's' to this comment?)

@@ +627,1 @@
>      let found = false;

Can we eliminate 'found' altogether, and just use 'scriptsAndOffsetMappings.size > 0' as the condition?

@@ +627,2 @@
>      let found = false;
> +    let scriptsAndOffsetMappings = new Map();

Let's have a brief comment explaining what the keys and values of this map are.

@@ +630,4 @@
>      for (let script of scripts) {
> +      scriptsAndOffsetMappings = this._findClosestOffsetMappings(aLocation,
> +                                                                 script,
> +                                                                 scriptsAndOffsetMappings);

Let's just have this function's contract be that it mutates scriptsAndOffsetMappings; as long as we're actually updating the map destructively, it's best not to have interfaces that suggest pure patterns.

@@ +727,1 @@
>       * epically.

(Could we shore up this comment, too? "fail epically", I guess? Or is this like "I accidentally the whole breakpoint"?)

@@ +735,5 @@
> +  _findClosestOffsetMappings: function TA__findClosestOffsetMappings(aTargetLocation,
> +                                                                     aScript,
> +                                                                     aScriptsAndOffsetMappings) {
> +    let offsetMappings = aScript.getAllColumnOffsets()
> +      .filter(({ lineNumber }) => lineNumber === aTargetLocation.line);

Nice. (I wish Debugger had an API that would just tell you this...)
thanks for reviewing, Jim.

> Nice. (I wish Debugger had an API that would just tell you this...)

Is that something we can file a bug for? If we can move that kind of filtering into the Debugger, that might save us from having to do this elsewhere.
Attached patch v3 (obsolete) — — Splinter Review 
No try push as the tree is currently closed. Will update when I get a chance.

In addition to the changes associated with the previous round of review, I have formalized the breakpoint store so that its implementation doesn't leak all over the ThreadActor's code and because it got a bit more complicated now that there are two types of breakpoints: those that are for a specific line/column and those that specify only a line and should break on any column in that line.
Attachment #756793 - Attachment is obsolete: true
Attachment #756793 - Flags: review?(jimb)
Attachment #772411 - Flags: review?(jimb)
Comment on attachment 772411 [details] [diff] [review]
v3

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

Some intermediate comments; still reviewing.

I am so grateful for the comments in this patch. It just seems like an elementary courtesy to readers.

The comments about boolean conditions are just suggestions; if you disagree, no need to change.

::: toolkit/devtools/client/dbg-client.jsm
@@ +306,5 @@
> +          let msg = "Error executing callback passed to debugger client: "
> +            + e + "\n" + e.stack;
> +          dumpn(msg);
> +          Cu.reportError(msg);
> +        }

Won't these errors be caught by a makeInfallible? Can the issue here be addressed in a more systematic way?

::: toolkit/devtools/server/actors/script.js
@@ +8,5 @@
>  
>  /**
> + * BreakpointStore objects keep track of all breakpoints that get set so that we
> + * can reset them when the same script is introduced to the thread again (such
> + * as after a refresh).

Abstracting this out is excellent. Badly needed. Having this broken out this way gives me so much more confidence that I understand all the uses the data structure is being put to.

@@ +12,5 @@
> + * as after a refresh).
> + */
> +function BreakpointStore() {
> +  // url -> sparse array of { url, line[, actor] }
> +  this._wholeLineBreakpoints = Object.create(null);

These comments are helpful to me; thanks very much. But I incorrectly guessed how these maps worked from the comment. :( I guess I would say something like:

"If we have a whole-line breakpoint set at LINE in URL, then this._wholeLineBreakpoints[URL][LINE] is an object { url, line }, with an optional 'actor' property as well."

That way we cover both the keys and the values of the main map and all the intermediate maps.



(Also, to me 'sparse array' means something indexed by a non-contiguous set of integers. Arrays indexed by strings are almost always 'sparse'.)

@@ +31,5 @@
> +   *          - column (optional; omission implies that the breakpoint is for
> +   *            the whole line)
> +   *          - actor (optional)
> +   */
> +  addBreakpoint: function BS_addBreakpoint(aBreakpoint) {

Do we still need these 'BS_blah' names? Doesn't SpiderMonkey assign display names well enough? It ought to name this one BreakpointStore.prototype.addBreakpoint...

@@ +34,5 @@
> +   */
> +  addBreakpoint: function BS_addBreakpoint(aBreakpoint) {
> +    let { url, line, column } = aBreakpoint;
> +
> +    if (column != null) {

How do you feel about 'if (column)'?

@@ +62,5 @@
> +   *          - line
> +   *          - column (optional)
> +   */
> +  removeBreakpoint: function BS_removeBreakpoint({ url, line, column }) {
> +    if (column != null) {

'if (column)'?

@@ +92,5 @@
> +   */
> +  getBreakpoint: function BS_getBreakpoint(aLocation, aShouldThrow=true) {
> +    let { url, line, column } = aLocation;
> +    dbg_assert(url != null);
> +    dbg_assert(line != null);

(url), (line)?

@@ +177,5 @@
> +        ? this._breakpoints[aUrl].length
> +        : 0;
> +      const maxLine = Math.max(wholeLineLen, len);
> +
> +      for (let line = 0; line < maxLine; line++) {

It would be okay with me if you just did .keys on both of them, concatenated them, and sorted them. That would be super-legible.

@@ +198,5 @@
> +        yield aColumn;
> +      }
> +    } else {
> +      const len = this._breakpoints[aUrl][aLine].length;
> +      for (let column = 0; column < len; column++) {

If you use an empty array '[]' for the column-indexed maps, and then iterate over that using 'for (i in a)', then SpiderMonkey will always iterate over only the lines that are assigned, in increasing index order. (This is unspecified, but other browsers do it too, so SpiderMonkey isn't going to break it.) You won't need to spend time on the empty columns.

Perhaps there's a way to get this to work for the line numbers too, but since you need the union of two iterations, I dunno.

@@ +246,5 @@
>  /**
>   * The breakpoint store must be shared across instances of ThreadActor so that
>   * page reloads don't blow away all of our breakpoints.
>   */
> +ThreadActor.breakpointStore = new BreakpointStore();

This isn't right. The breakpoint store needs to be per-ThreadActor. Page reloads and navigation don't create new ThreadActors; the TabActor creates one when we attach to it.

It may be desirable for breakpoints to be saved between sessions, and shared between multiple simultaneous connections --- and a global breakpoint table could be part of that. But we can't just start behaving that way without a bunch of other protocol and client changes; without that accompanying work, making the table shared is a bug.

@@ +480,5 @@
>        }
>        packet.why = aReason;
> +
> +      let { url, line, column } = packet.frame.where;
> +      this.sources.getOriginalLocation(url, line, column).then(aOrigPosition => {

As I said in comment 23, I bet making getOriginalLocation and getGeneratedLocation take an object with 'url', 'line', and 'column' properties as an argument would clean up some of this unpacking code. That would be parallel to the BreakpointStore public methods, too.
(In reply to Jim Blandy :jimb from comment #29)
> ::: toolkit/devtools/client/dbg-client.jsm
> @@ +306,5 @@
> > +          let msg = "Error executing callback passed to debugger client: "
> > +            + e + "\n" + e.stack;
> > +          dumpn(msg);
> > +          Cu.reportError(msg);
> > +        }
> 
> Won't these errors be caught by a makeInfallible? Can the issue here be
> addressed in a more systematic way?

I think we should just make all of dbg-client.jsm use makeInfallible in a follow up. How does that sound?

> ::: toolkit/devtools/server/actors/script.js
> @@ +8,5 @@
> >  
> >  /**
> > + * BreakpointStore objects keep track of all breakpoints that get set so that we
> > + * can reset them when the same script is introduced to the thread again (such
> > + * as after a refresh).
> 
> Abstracting this out is excellent. Badly needed. Having this broken out this
> way gives me so much more confidence that I understand all the uses the data
> structure is being put to.

Yeah long overdue, IMO.

> @@ +12,5 @@
> > + * as after a refresh).
> > + */
> > +function BreakpointStore() {
> > +  // url -> sparse array of { url, line[, actor] }
> > +  this._wholeLineBreakpoints = Object.create(null);
> 
> These comments are helpful to me; thanks very much. But I incorrectly
> guessed how these maps worked from the comment. :( I guess I would say
> something like:
> 
> "If we have a whole-line breakpoint set at LINE in URL, then
> this._wholeLineBreakpoints[URL][LINE] is an object { url, line }, with an
> optional 'actor' property as well."
> 
> That way we cover both the keys and the values of the main map and all the
> intermediate maps.

Yeah I can do this, it's probably clearer.

> (Also, to me 'sparse array' means something indexed by a non-contiguous set
> of integers. Arrays indexed by strings are almost always 'sparse'.)

Which is exactly what the array indexed by LINE is ;) This misunderstanding speaks to the lack of utility of the comment above.

> 
> @@ +31,5 @@
> > +   *          - column (optional; omission implies that the breakpoint is for
> > +   *            the whole line)
> > +   *          - actor (optional)
> > +   */
> > +  addBreakpoint: function BS_addBreakpoint(aBreakpoint) {
> 
> Do we still need these 'BS_blah' names? Doesn't SpiderMonkey assign display
> names well enough? It ought to name this one
> BreakpointStore.prototype.addBreakpoint...

Didn't want to make the file heterogenous. Biggest code style no-no is inconsistency. We should just file a follow up to remove all of them.

Filed bug 895603.

> 
> @@ +34,5 @@
> > +   */
> > +  addBreakpoint: function BS_addBreakpoint(aBreakpoint) {
> > +    let { url, line, column } = aBreakpoint;
> > +
> > +    if (column != null) {
> 
> How do you feel about 'if (column)'?

Column can be 0, doesn't work. Using `== null` is nice because:

1) 0 == null is false (when column is 0)

2) null == null is true (when someone wants to be explicit that they don't have a column)

3) undefined == null is true (when someone just leaves out the column)

Same argument applies to the rest of the null checks.

> @@ +177,5 @@
> > +        ? this._breakpoints[aUrl].length
> > +        : 0;
> > +      const maxLine = Math.max(wholeLineLen, len);
> > +
> > +      for (let line = 0; line < maxLine; line++) {
> 
> It would be okay with me if you just did .keys on both of them, concatenated
> them, and sorted them. That would be super-legible.

But much slower :(

Compromise: should I add a comment at the beginning of the block explaining what is happening?

> @@ +198,5 @@
> > +        yield aColumn;
> > +      }
> > +    } else {
> > +      const len = this._breakpoints[aUrl][aLine].length;
> > +      for (let column = 0; column < len; column++) {
> 
> If you use an empty array '[]' for the column-indexed maps, and then iterate
> over that using 'for (i in a)', then SpiderMonkey will always iterate over
> only the lines that are assigned, in increasing index order. (This is
> unspecified, but other browsers do it too, so SpiderMonkey isn't going to
> break it.) You won't need to spend time on the empty columns.
> 
> Perhaps there's a way to get this to work for the line numbers too, but
> since you need the union of two iterations, I dunno.

They *are* initialized with empty arrays, see BreakpointStore.prototype.addBreakpoint. Will add this change.

> 
> @@ +246,5 @@
> >  /**
> >   * The breakpoint store must be shared across instances of ThreadActor so that
> >   * page reloads don't blow away all of our breakpoints.
> >   */
> > +ThreadActor.breakpointStore = new BreakpointStore();
> 
> This isn't right. The breakpoint store needs to be per-ThreadActor. Page
> reloads and navigation don't create new ThreadActors; the TabActor creates
> one when we attach to it.
> 
> It may be desirable for breakpoints to be saved between sessions, and shared
> between multiple simultaneous connections --- and a global breakpoint table
> could be part of that. But we can't just start behaving that way without a
> bunch of other protocol and client changes; without that accompanying work,
> making the table shared is a bug.

The table is already shared, this is only preserving existing behavior. We gave each thread actor their own breakpoint store at one point in time and things started breaking left and right.

https://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#48

https://bugzilla.mozilla.org/show_bug.cgi?id=777146

> 
> @@ +480,5 @@
> >        }
> >        packet.why = aReason;
> > +
> > +      let { url, line, column } = packet.frame.where;
> > +      this.sources.getOriginalLocation(url, line, column).then(aOrigPosition => {
> 
> As I said in comment 23, I bet making getOriginalLocation and
> getGeneratedLocation take an object with 'url', 'line', and 'column'
> properties as an argument would clean up some of this unpacking code. That
> would be parallel to the BreakpointStore public methods, too.

I started doing this and ended up with a bug that I couldn't track down (which is surprising since it seems like a simple change, maybe I was just sleepy that day). I meant to comment last time about pushing this to a follow up, but I forgot. Sorry.

Filed bug 895616
(In reply to Nick Fitzgerald [:fitzgen] from comment #30)
> (In reply to Jim Blandy :jimb from comment #29)
> > ::: toolkit/devtools/client/dbg-client.jsm
> > @@ +306,5 @@
> > > +          let msg = "Error executing callback passed to debugger client: "
> > > +            + e + "\n" + e.stack;
> > > +          dumpn(msg);
> > > +          Cu.reportError(msg);
> > > +        }
> > 
> > Won't these errors be caught by a makeInfallible? Can the issue here be
> > addressed in a more systematic way?
> 
> I think we should just make all of dbg-client.jsm use makeInfallible in a
> follow up. How does that sound?

Filed bug 895662
(In reply to Nick Fitzgerald [:fitzgen] from comment #30)
> > @@ +177,5 @@
> > > +        ? this._breakpoints[aUrl].length
> > > +        : 0;
> > > +      const maxLine = Math.max(wholeLineLen, len);
> > > +
> > > +      for (let line = 0; line < maxLine; line++) {
> > 
> > It would be okay with me if you just did .keys on both of them, concatenated
> > them, and sorted them. That would be super-legible.
> 
> But much slower :(
> 
> Compromise: should I add a comment at the beginning of the block explaining
> what is happening?

Nevermind, because of how sparse this tends to be, your suggestsion is actually going to be faster for real world use cases. Will update this.
(In reply to Nick Fitzgerald [:fitzgen] from comment #30)
> (In reply to Jim Blandy :jimb from comment #29)
> > ::: toolkit/devtools/client/dbg-client.jsm
> > @@ +306,5 @@
> > > +          let msg = "Error executing callback passed to debugger client: "
> > > +            + e + "\n" + e.stack;
> > > +          dumpn(msg);
> > > +          Cu.reportError(msg);
> > > +        }
> > 
> > Won't these errors be caught by a makeInfallible? Can the issue here be
> > addressed in a more systematic way?
> 
> I think we should just make all of dbg-client.jsm use makeInfallible in a
> follow up. How does that sound?

Oh, that's right, this is the client. I've done it (where I saw it) for the server. Yes, a follow-up bug is appropriate.

> Didn't want to make the file heterogenous. Biggest code style no-no is
> inconsistency. We should just file a follow up to remove all of them.
> 
> Filed bug 895603.

Good enough.

> > How do you feel about 'if (column)'?
> 
> Column can be 0, doesn't work. Using `== null` is nice because:

D'oh. Okay. Yes, I like the reasoning here.

> > @@ +177,5 @@
> > > +        ? this._breakpoints[aUrl].length
> > > +        : 0;
> > > +      const maxLine = Math.max(wholeLineLen, len);
> > > +
> > > +      for (let line = 0; line < maxLine; line++) {
> > 
> > It would be okay with me if you just did .keys on both of them, concatenated
> > them, and sorted them. That would be super-legible.
> 
> But much slower :(

Really? Scripts can be thousands of lines long, but will only have a handful of breakpoints set in them. Won't .keys give you only the line numbers that have breakpoints?


> > @@ +246,5 @@
> > >  /**
> > >   * The breakpoint store must be shared across instances of ThreadActor so that
> > >   * page reloads don't blow away all of our breakpoints.
> > >   */
> > > +ThreadActor.breakpointStore = new BreakpointStore();
> > 
> > This isn't right. The breakpoint store needs to be per-ThreadActor. Page
> > reloads and navigation don't create new ThreadActors; the TabActor creates
> > one when we attach to it.
> > 
> > It may be desirable for breakpoints to be saved between sessions, and shared
> > between multiple simultaneous connections --- and a global breakpoint table
> > could be part of that. But we can't just start behaving that way without a
> > bunch of other protocol and client changes; without that accompanying work,
> > making the table shared is a bug.
> 
> The table is already shared, this is only preserving existing behavior. We
> gave each thread actor their own breakpoint store at one point in time and
> things started breaking left and right.

I'm both embarrassed, and amazed. I'll talk to you and past about this on IRC.


> I started doing this and ended up with a bug that I couldn't track down
> (which is surprising since it seems like a simple change, maybe I was just
> sleepy that day). I meant to comment last time about pushing this to a
> follow up, but I forgot. Sorry.
> 
> Filed bug 895616

That's fine.
(In reply to Nick Fitzgerald [:fitzgen] from comment #32)
> Nevermind, because of how sparse this tends to be, your suggestsion is
> actually going to be faster for real world use cases. Will update this.

That's what I though, yeah.
(In reply to Nick Fitzgerald [:fitzgen] from comment #30)
> (In reply to Jim Blandy :jimb from comment #29)
> > This isn't right. The breakpoint store needs to be per-ThreadActor. Page
> > reloads and navigation don't create new ThreadActors; the TabActor creates
> > one when we attach to it.
> > 
> > It may be desirable for breakpoints to be saved between sessions, and shared
> > between multiple simultaneous connections --- and a global breakpoint table
> > could be part of that. But we can't just start behaving that way without a
> > bunch of other protocol and client changes; without that accompanying work,
> > making the table shared is a bug.
> 
> The table is already shared, this is only preserving existing behavior. We
> gave each thread actor their own breakpoint store at one point in time and
> things started breaking left and right.
> 
> https://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/
> actors/script.js#48
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=777146

I actually think this is no longer necessary. We started reusing the same ThreadActor across reloads to make sure breakpoints in inline <head> scripts work, so there is no longer a need to share the breakpoint store.

I would have quickly tested this assumption if my local test runs didn't fail with weird unrelated errors.
Attached patch v4 (obsolete) — — Splinter Review 
https://tbpl.mozilla.org/?tree=Try&rev=b889707ba489

* Rebased on fx-team

* Changed comments about the structures inside BreakpointStore objects

* Using Object.keys() -> concat -> sort inside _iterLines

* Use for (i in a) style iteration in _iterColumns
Attachment #772411 - Attachment is obsolete: true
Attachment #772411 - Flags: review?(jimb)
Attachment #779429 - Flags: review?(jimb)
Comment on attachment 772411 [details] [diff] [review]
v3

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

Some leftover comments from the obsolete version of the patch...

::: toolkit/devtools/server/actors/script.js
@@ +480,5 @@
>        }
>        packet.why = aReason;
> +
> +      let { url, line, column } = packet.frame.where;
> +      this.sources.getOriginalLocation(url, line, column).then(aOrigPosition => {

I wish we'd done this in the original source map patch. (Repeating IRC conversation for the record:) If there are any places where we're not presenting 'original' source locations, even when source maps are enabled, available, and retrieved, then we should file a bug for that. 

There's no reason some packets should send original locations and others generated locations; from the client's point of view, it should just be "the code" (ignoring the 'not retrieved yet' issue).

@@ +490,5 @@
> +            return {
> +              error: error.message + "\n" + error.stack
> +            };
> +          });
> +      });

Thanks for adding this error catch.

(For what it's worth, it helps me review faster if these sorts of things are broken out into separate patches where I can just say, "duh, sure" and not wonder whether it's related to the other stuff. They can be in the same bug.)

@@ +825,5 @@
>     * will be updated, so callers that iterate over the breakpoint cache should
>     * take that into account.
>     *
>     * @param object aLocation
>     *        The location of the breakpoint as specified in the protocol.

I think this comment became out-of-date when we landed source maps; isn't it the generated location corresponding to the original location specified in the protocol? (If so, could you fix the comment?)
Attachment #772411 - Attachment is obsolete: false
Attachment #772411 - Attachment is obsolete: true
Attached patch v4.1 (obsolete) — — Splinter Review 
Sorry for not splitting that out.

I can't think of any place where we are still sending non-source-mapped locations.

https://tbpl.mozilla.org/?tree=Try&rev=37ca85b20934

* Updates comment about locations

* Cleans up the error handling code for pauseAndRespond's onPacket to be more similar to the rest of our error packets.
Attachment #779429 - Attachment is obsolete: true
Attachment #779429 - Flags: review?(jimb)
Attachment #779502 - Flags: review?(jimb)
Waaah. I don't think that patch is what you meant to upload. :)
Attached patch v4.1 (forreal) (obsolete) — — Splinter Review 
Woops!

https://tbpl.mozilla.org/?tree=Try&rev=c97723acfc56
Attachment #779502 - Attachment is obsolete: true
Attachment #779502 - Flags: review?(jimb)
Attachment #779970 - Flags: review?(jimb)
Comment on attachment 779970 [details] [diff] [review]
v4.1 (forreal)

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

As a follow-up bug, would it make sense to give BreakpointActors all the necessary public properties, and then store them *directly* in the breakpoint store? That is, unify the { url, line, column, actor } thingy with BreakpointActor?

The URL manipulation things need to be broken out into their own patch. If you update this patch with those removed, I'll give you a speedy r+.

::: toolkit/devtools/server/actors/script.js
@@ +19,5 @@
> +  // is an object
> +  //
> +  //   { url, line[, actor] }
> +  //
> +  //  where the `actor` property is optional.

This comment makes me weep for joy.

In general, the class and method comments for BreakpointStore are a real courtesy to future developers.

@@ +47,5 @@
> +   *          - line
> +   *          - column (optional; omission implies that the breakpoint is for
> +   *            the whole line)
> +   *          - actor (optional)
> +   */

Should this explain that the store holds the object aBreakpoint, itself, not a copy? When reviewing below I needed to verify that _createAndStoreBreakpoint's argument was an object that was safe to steal and use as the store's entry; it seems like a property callers ought to know about.

@@ +117,5 @@
> +   * @param bool aShouldThrow
> +   *        Optional; defaults to true. Whether an error should be thrown when
> +   *        there is no breakpoint at the specified locaiton.
> +   */
> +  getBreakpoint: function BS_getBreakpoint(aLocation, aShouldThrow=true) {

If you think it would be an improvement, consider having a separate method, hasBreakpoint, which returns an entry if present, or null otherwise. Then getBreakpoint could always throw; it could be implemented simply in terms of hasBreakpoint; and the call sites that now pass false for aShouldThrow could become a little easier to read.

@@ +942,1 @@
>                if (url !== originalSource || line !== originalLine) {

Do we need to also check column against originalColumn here?

@@ +959,5 @@
>      });
>    },
>  
>    /**
>     * Create a breakpoint at the specified location and store it in the cache.

This really should explain that aLocation may be taken for use as a breakpoint store entry. Otherwise, callers might naively pass objects that are being used for something else (say, stuff directly from a packet, which is frozen).

@@ +1115,5 @@
>        actor: actor.actorID
>      };
>    },
>  
> +  _findClosestOffsetMappings: function TA__findClosestOffsetMappings(aTargetLocation,

This function really needs a solid description of what it's computing, and the rules it uses to do so. There's a lot of thinking going on here, and I don't completely follow it.

@@ +1134,5 @@
> +    let closestDistance = Infinity;
> +    if (aScriptsAndOffsetMappings.size) {
> +      for (let mappings of aScriptsAndOffsetMappings.values()) {
> +        closestDistance = Math.abs(aTargetLocation.column - mappings[0].columnNumber);
> +        break;

I'm confused. We break after the first iteration? Why a for-of loop, then? Isn't the 'if' around it good enough? But I think this loop meant to check all the column entries...

(If this code meant to do something else, add a test that catches this bug.)

@@ +3151,5 @@
>  
>    /**
> +   * Sets the source map's sourceRoot to be relative to the source map url.
> +   */
> +  _setSourceMapRoot: function TS__setSourceMapRoot(aSourceMap, aAbsSourceMapURL, aScriptURL) {

It looks like this, and a lot of the other changes in ThreadSources, having nothing to do with column support. Let's break these out into their own bug.
Attachment #779970 - Flags: review?(jimb)
I cleared review on the patch --- but unless you make changes to _findClosestOffsetMappings that you want a second pair of eyes on, go ahead and land this with the comments addressed. And post the updated patch so I can r+ it.
https://hg.mozilla.org/integration/fx-team/rev/00b996131f8c
Whiteboard: [leave open] → [leave open][fixed-in-fx-team]
Attached patch v4.2 — — Splinter Review 
THis is the patch that landed, updated with the changes suggested by Jim.
Attachment #779970 - Attachment is obsolete: true
Comment on attachment 780718 [details] [diff] [review]
v4.2

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

Great!
Attachment #780718 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/00b996131f8c
Whiteboard: [leave open][fixed-in-fx-team] → [leave open]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
No longer blocks: 878307
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: