Closed Bug 1447316 Opened 3 years ago Closed 3 years ago

The debugger should use pausePoints to decide where to stop

Categories

(DevTools :: Debugger, enhancement, P2)

Product:
DevTools
Component:
Debugger
Type:
enhancement

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: jlast, Assigned: jlast)

Details

Attachments

(1 file)

Bug 1447316 - The debugger should use pausePoints to decide where to stop.
59 bytes, text/x-review-board-request
jimb
: review+
Details 
Today, the debugger's stepping behavior is broken in two significant ways: multiple statements in the same line, statements that span multiple lines.

In the long run, we can improve the engine's stepping behavior. In the short term,
we can address these two issues by having a dictionary of valid pause and step locations.
Could we get a comment explaining the format of the pausepoint objects?
Assignee: nobody → jlaster
Comment on attachment 8960616 [details]
Bug 1447316 - The debugger should use pausePoints to decide where to stop.

https://reviewboard.mozilla.org/r/229372/#review235224

::: devtools/server/actors/thread.js:508
(Diff revision 3)
>        // above cases is true:
>        //
>        // 2.1. We are in a source mapped region, but inside a null mapping
>        //      (doesn't correlate to any region of original source)
>        // 2.2. The source we are in is black boxed.
> +      // 2.3. We are in the same location as before

I don't think this comment is quite right. If we have changed frames, then that can mean that we've entered a recursive call in code like this:

    function f(n) { return n <= 1 ? 0 : n * f(n-1); }

In that case, even though the line has not changed, we should pause.

I think the way to update this comment is to change 1.3 to think in terms other than 'line'.

::: devtools/server/actors/thread.js:519
(Diff revision 3)
>        }
>  
> -      // Cases 1.1, 1.2 and 1.3
> -      if (this !== startFrame
> -          || startLocation.originalUrl !== newLocation.originalUrl
> -          || startLocation.originalLine !== newLocation.originalLine) {
> +      // Case 2.3
> +      if (
> +        startLocation.originalLine === newLocation.originalLine
> +        && startLocation.originalColumn === newLocation.originalColumn

By moving the check against `startFrame` later, this change breaks the case I mentioned earlier, of calls to the the same line.

::: devtools/server/actors/thread.js:1916
(Diff revision 3)
>    }
>    return entryPoints;
>  }
>  
> +function findPausePointForLocation(pausePoints, location) {
> +  return pausePoints.find(pausePoint =>

I'm uncomfortable using a linear search here. Could we use a double map or a binary search instead?

Also, I'm surprised that looking for an exact match works; I expected to see some sort of ranged search. Does this mean that if the pause point provided by the client doesn't exactly match the position at which Debugger reports the stop, that the pause point will be skipped altogether?
Attachment #8960616 - Flags: review?(jimb) → review-
> I don't think this comment is quite right.

good call

> I'm uncomfortable using a linear search here.

I'm also concerned about the search and compactness of the data structure. I was planning on coming back to it, but i'd be happy to drop in a binary search.

> Also, I'm surprised that looking for an exact match works;

This was also a concern of mine, but having played with the algorithm I feel better about it. The primary area where I could see issues are with control flow (e.g. IF). Does it make sense to follow up here and explore relaxing the criteria?
> I don't think this comment is quite right.

So, I looked into this and i think checking that we're in the same location is correct:

1. what if it is recursive and we have a different frame and the same location?

In this case, we'll be in 1.1 and we'll have already paused.

2. what about extending 1.3 to check more than the line

1.3 is specifically for the case that we do *not* have pause points, in that case we need to use the old heuristic. When we do have pause points, we can be stricter to insist that we are not at the same place, which would happen at a multi-line call expression which has to byte instructions that map to the same location.

I've moved the location check down to the pause point section so that it is clearer.
Comment on attachment 8960616 [details]
Bug 1447316 - The debugger should use pausePoints to decide where to stop.

https://reviewboard.mozilla.org/r/229372/#review235638


Code analysis found 38 defects in this patch (only the first 30 are reported here):
 - 38 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/server/actors/source.js:121
(Diff revision 4)
> +
> +  return 0;
> +}
> +
> +function sortPausePoints(pausePoints) {
> +  return pausePoints.sort((a,b) => compareLocations(a.location, b.location))

Error: A space is required after ','. [eslint: comma-spacing]

::: devtools/server/actors/source.js:121
(Diff revision 4)
> +
> +  return 0;
> +}
> +
> +function sortPausePoints(pausePoints) {
> +  return pausePoints.sort((a,b) => compareLocations(a.location, b.location))

Error: Missing semicolon. [eslint: semi]

::: devtools/server/actors/thread.js:1918
(Diff revision 4)
>    return entryPoints;
>  }
>  
> +function findPausePointForLocation(pausePoints, location) {
> +  const foundIndex = binarySearch(pausePoints, pausePoint => {
> +    const { line, column } = pausePoint.location

Error: Missing semicolon. [eslint: semi]

::: devtools/server/actors/thread.js:1933
(Diff revision 4)
> +    if (line > location.originalLine) {
> +      return 1;
> +    }
> +
> +    return column < location.originalColumn ? -1 : 1;
> +  })

Error: Missing semicolon. [eslint: semi]

::: devtools/shared/binary-search.js:18
(Diff revision 4)
> + * @param low Indices here and lower do not contain the needle.
> + * @param high Indices here and higher do not contain the needle.
> + * @param haystack The non-empty array being searched.
> + * @param compare Function which takes two elements and returns -1, 0, or 1.
> + */
> +function recursiveSearch(low, high, haystack, compare) {

Error: Block must not be padded by blank lines. [eslint: padded-blocks]

::: devtools/shared/binary-search.js:21
(Diff revision 4)
> + * @param compare Function which takes two elements and returns -1, 0, or 1.
> + */
> +function recursiveSearch(low, high, haystack, compare) {
> +
> +  if (low >= high) {
> +    return -1

Error: Missing semicolon. [eslint: semi]

::: devtools/shared/binary-search.js:24
(Diff revision 4)
> +
> +  if (low >= high) {
> +    return -1
> +  }
> +
> +  var mid = Math.floor((high - low) / 2) + low;

Error: Unexpected var, use let or const instead. [eslint: mozilla/var-only-at-top-level]

::: devtools/shared/binary-search.js:25
(Diff revision 4)
> +  if (low >= high) {
> +    return -1
> +  }
> +
> +  var mid = Math.floor((high - low) / 2) + low;
> +  var cmp = compare(haystack[mid], true);

Error: Unexpected var, use let or const instead. [eslint: mozilla/var-only-at-top-level]

::: devtools/shared/binary-search.js:30
(Diff revision 4)
> +  var cmp = compare(haystack[mid], true);
> +
> +  if (cmp === 0) {
> +    // Found the element we are looking for.
> +    return mid;
> +  }

Error: Closing curly brace does not appear on the same line as the subsequent block. [eslint: brace-style]

::: devtools/shared/binary-search.js:40
(Diff revision 4)
> +      // The element is in the upper half.
> +      return recursiveSearch(mid, high, haystack, compare);
> +    }
> +
> +    return -1;
> +  }

Error: Closing curly brace does not appear on the same line as the subsequent block. [eslint: brace-style]

::: devtools/shared/binary-search.js:41
(Diff revision 4)
> +      return recursiveSearch(mid, high, haystack, compare);
> +    }
> +
> +    return -1;
> +  }
> +  else {

Error: Unnecessary 'else' after 'return'. [eslint: no-else-return]

::: devtools/shared/binary-search.js:51
(Diff revision 4)
> +    }
> +
> +    return low < 0 ? -1 : low;
> +  }
> +}
> +

Error: More than 1 blank line not allowed. [eslint: no-multiple-empty-lines]

::: devtools/shared/binary-search.js:58
(Diff revision 4)
> +/**
> + * This is an implementation of binary search which will try and find the item
> + * based on a predicate function, which determines whether the item is a match.
> + *
> + * @param haystack The array that is being searched.
> + * @param compare A function which returns -1, 0, or 1 depending on whether 

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: devtools/shared/binary-search.js:59
(Diff revision 4)
> + * This is an implementation of binary search which will try and find the item
> + * based on a predicate function, which determines whether the item is a match.
> + *
> + * @param haystack The array that is being searched.
> + * @param compare A function which returns -1, 0, or 1 depending on whether 
> + *        the element is less than, equal to, or greater than the item we're searching for.

Error: Line 59 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/shared/binary-search.js:66
(Diff revision 4)
> +function binarySearch(haystack, compare) {
> +  if (haystack.length === 0) {
> +    return -1;
> +  }
> +
> +  var index = recursiveSearch(-1, haystack.length, haystack, compare)

Error: Unexpected var, use let or const instead. [eslint: mozilla/var-only-at-top-level]

::: devtools/shared/binary-search.js:66
(Diff revision 4)
> +function binarySearch(haystack, compare) {
> +  if (haystack.length === 0) {
> +    return -1;
> +  }
> +
> +  var index = recursiveSearch(-1, haystack.length, haystack, compare)

Error: Missing semicolon. [eslint: semi]

::: devtools/shared/binary-search.js:74
(Diff revision 4)
> +    return -1;
> +  }
> +
> +  return index;
> +
> +}

Error: Block must not be padded by blank lines. [eslint: padded-blocks]

::: devtools/shared/tests/unit/test_binarySearch.js:14
(Diff revision 4)
> +  function comparator(a, b) {
> +    if (a == b) {
> +      return 0;
> +    }
> +
> +    return a < b ? -1 : 1

Error: Missing semicolon. [eslint: semi]

::: devtools/shared/tests/unit/test_binarySearch.js:17
(Diff revision 4)
> +    }
> +
> +    return a < b ? -1 : 1
> +  }
> +
> +  const array = [1,2,3,4,5,6]

Error: A space is required after ','. [eslint: comma-spacing]

::: devtools/shared/tests/unit/test_binarySearch.js:17
(Diff revision 4)
> +    }
> +
> +    return a < b ? -1 : 1
> +  }
> +
> +  const array = [1,2,3,4,5,6]

Error: A space is required after ','. [eslint: comma-spacing]

::: devtools/shared/tests/unit/test_binarySearch.js:17
(Diff revision 4)
> +    }
> +
> +    return a < b ? -1 : 1
> +  }
> +
> +  const array = [1,2,3,4,5,6]

Error: A space is required after ','. [eslint: comma-spacing]

::: devtools/shared/tests/unit/test_binarySearch.js:17
(Diff revision 4)
> +    }
> +
> +    return a < b ? -1 : 1
> +  }
> +
> +  const array = [1,2,3,4,5,6]

Error: A space is required after ','. [eslint: comma-spacing]

::: devtools/shared/tests/unit/test_binarySearch.js:17
(Diff revision 4)
> +    }
> +
> +    return a < b ? -1 : 1
> +  }
> +
> +  const array = [1,2,3,4,5,6]

Error: A space is required after ','. [eslint: comma-spacing]

::: devtools/shared/tests/unit/test_binarySearch.js:17
(Diff revision 4)
> +    }
> +
> +    return a < b ? -1 : 1
> +  }
> +
> +  const array = [1,2,3,4,5,6]

Error: Missing semicolon. [eslint: semi]

::: devtools/shared/tests/unit/test_binarySearch.js:19
(Diff revision 4)
> +    return a < b ? -1 : 1
> +  }
> +
> +  const array = [1,2,3,4,5,6]
> +
> +  const items = [[0,-1], [1,0], [6,5], [7,-1], ["t", -1]]

Error: A space is required after ','. [eslint: comma-spacing]

::: devtools/shared/tests/unit/test_binarySearch.js:19
(Diff revision 4)
> +    return a < b ? -1 : 1
> +  }
> +
> +  const array = [1,2,3,4,5,6]
> +
> +  const items = [[0,-1], [1,0], [6,5], [7,-1], ["t", -1]]

Error: A space is required after ','. [eslint: comma-spacing]

::: devtools/shared/tests/unit/test_binarySearch.js:19
(Diff revision 4)
> +    return a < b ? -1 : 1
> +  }
> +
> +  const array = [1,2,3,4,5,6]
> +
> +  const items = [[0,-1], [1,0], [6,5], [7,-1], ["t", -1]]

Error: A space is required after ','. [eslint: comma-spacing]

::: devtools/shared/tests/unit/test_binarySearch.js:19
(Diff revision 4)
> +    return a < b ? -1 : 1
> +  }
> +
> +  const array = [1,2,3,4,5,6]
> +
> +  const items = [[0,-1], [1,0], [6,5], [7,-1], ["t", -1]]

Error: A space is required after ','. [eslint: comma-spacing]

::: devtools/shared/tests/unit/test_binarySearch.js:19
(Diff revision 4)
> +    return a < b ? -1 : 1
> +  }
> +
> +  const array = [1,2,3,4,5,6]
> +
> +  const items = [[0,-1], [1,0], [6,5], [7,-1], ["t", -1]]

Error: Missing semicolon. [eslint: semi]

::: devtools/shared/tests/unit/test_binarySearch.js:22
(Diff revision 4)
> +  const array = [1,2,3,4,5,6]
> +
> +  const items = [[0,-1], [1,0], [6,5], [7,-1], ["t", -1]]
> +  for (let item of items) {
> +    const [index, expected] = item;
> +    const actual = binarySearch(array,  newValue => comparator(index, newValue))

Error: Multiple spaces found before 'newvalue'. [eslint: no-multi-spaces]

::: devtools/shared/tests/unit/test_binarySearch.js:22
(Diff revision 4)
> +  const array = [1,2,3,4,5,6]
> +
> +  const items = [[0,-1], [1,0], [6,5], [7,-1], ["t", -1]]
> +  for (let item of items) {
> +    const [index, expected] = item;
> +    const actual = binarySearch(array,  newValue => comparator(index, newValue))

Error: Missing semicolon. [eslint: semi]

::: devtools/shared/tests/unit/test_binarySearch.js:26
(Diff revision 4)
> +    const [index, expected] = item;
> +    const actual = binarySearch(array,  newValue => comparator(index, newValue))
> +    equal(expected, actual);
> +  }
> +
> +  const objs = [{i: 1},{i: 2},{i: 3},{i: 4},{i: 5},{i: 6}]

Error: A space is required after ','. [eslint: comma-spacing]

::: devtools/shared/tests/unit/test_binarySearch.js:26
(Diff revision 4)
> +    const [index, expected] = item;
> +    const actual = binarySearch(array,  newValue => comparator(index, newValue))
> +    equal(expected, actual);
> +  }
> +
> +  const objs = [{i: 1},{i: 2},{i: 3},{i: 4},{i: 5},{i: 6}]

Error: A space is required after ','. [eslint: comma-spacing]

::: devtools/shared/tests/unit/test_binarySearch.js:26
(Diff revision 4)
> +    const [index, expected] = item;
> +    const actual = binarySearch(array,  newValue => comparator(index, newValue))
> +    equal(expected, actual);
> +  }
> +
> +  const objs = [{i: 1},{i: 2},{i: 3},{i: 4},{i: 5},{i: 6}]

Error: A space is required after ','. [eslint: comma-spacing]

::: devtools/shared/tests/unit/test_binarySearch.js:26
(Diff revision 4)
> +    const [index, expected] = item;
> +    const actual = binarySearch(array,  newValue => comparator(index, newValue))
> +    equal(expected, actual);
> +  }
> +
> +  const objs = [{i: 1},{i: 2},{i: 3},{i: 4},{i: 5},{i: 6}]

Error: A space is required after ','. [eslint: comma-spacing]

::: devtools/shared/tests/unit/test_binarySearch.js:26
(Diff revision 4)
> +    const [index, expected] = item;
> +    const actual = binarySearch(array,  newValue => comparator(index, newValue))
> +    equal(expected, actual);
> +  }
> +
> +  const objs = [{i: 1},{i: 2},{i: 3},{i: 4},{i: 5},{i: 6}]

Error: A space is required after ','. [eslint: comma-spacing]

::: devtools/shared/tests/unit/test_binarySearch.js:26
(Diff revision 4)
> +    const [index, expected] = item;
> +    const actual = binarySearch(array,  newValue => comparator(index, newValue))
> +    equal(expected, actual);
> +  }
> +
> +  const objs = [{i: 1},{i: 2},{i: 3},{i: 4},{i: 5},{i: 6}]

Error: Missing semicolon. [eslint: semi]

::: devtools/shared/tests/unit/test_binarySearch.js:27
(Diff revision 4)
> +    const actual = binarySearch(array,  newValue => comparator(index, newValue))
> +    equal(expected, actual);
> +  }
> +
> +  const objs = [{i: 1},{i: 2},{i: 3},{i: 4},{i: 5},{i: 6}]
> +  const objIndex = binarySearch(objs, newValue => comparator(4, newValue.i))

Error: Missing semicolon. [eslint: semi]
Priority: -- → P2
Here is a patch with the binary search for historical records: https://gist.github.com/jasonLaster/4f07c8a4d1e82473e4cc5cf675ab7689
Comment on attachment 8960616 [details]
Bug 1447316 - The debugger should use pausePoints to decide where to stop.

https://reviewboard.mozilla.org/r/229372/#review235848

::: devtools/server/actors/thread.js:508
(Diff revision 5)
>        // above cases is true:
>        //
>        // 2.1. We are in a source mapped region, but inside a null mapping
>        //      (doesn't correlate to any region of original source)
>        // 2.2. The source we are in is black boxed.
> +      // 2.3. We are in the same location as before

This comment still doesn't match the actual behavior. This is the list of reasons to continue "even if one of the above cases is true", but we (correctly) do pause if we change frames, even if "we are in the same location as before".

::: devtools/server/actors/thread.js:543
(Diff revision 5)
> +
> +      // When pause points are specified for the source,
> +      // we should pause when we are at a stepOver pause point
> +      const pausePoint = findPausePointForLocation(pausePoints, newLocation);
> +      if (pausePoint && pausePoint.types.stepOver) {
>          return pauseAndRespond(this);

At first I thought this test was backwards, since `stepOver` suggests to me that the location should be "stepped over". How about calling this "step"?

(We can leave this to be changed in a later patch.)
Attachment #8960616 - Flags: review?(jimb) → review+
Pushed by jlaster@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44fe122012fe
The debugger should use pausePoints to decide where to stop. r=jimb
https://hg.mozilla.org/mozilla-central/rev/44fe122012fe
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.