1013219 - Stepping on the last line in a frame might be confusing

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Stepping in the debugger can be sometimes confusing for the user since the current exe line is at non expected location.

Might be hard to fix, but still it would be nice enhancement.

STR:
1) Load https://getfirebug.com/tests/manual/issues/5802/issue5802.html
2) Open the debugger, create breakpoint at line 10
3) Reload the page to hit the breakpoint
4) Step over once
5) The debugger jumps at line 12 (the last executable line in the frame), which is confusing since it looks like the debugger jumped into the loop.

Honza

Comment 1

[Sebastian Zartner [:sebo]]

FWIW the debuggers in the other browsers halt at line 15 in this case, i.e. the line with the closing curly brace, which is somewhat clearer.

Sebastian

Comment 2

[Girish Sharma [:Optimizer]]

This (stepping to last line of a loop) is true for all kinds of for and while loops since very long ..

Comment 3

[Tom Tromey :tromey]

The bug here is that when the front end emits the "retrval" instruction
to terminate the bytecode, it doesn't set a sensible line number.

When I "dis(test)", I see that the instruction is

00062:  retrval

And the line table says

Source notes:
 ofs line    pc  delta desc     args
---- ---- ----- ------ -------- ------
...
 33:    4    49 [   0] colspan 21
 35:    4    62 [  13] xdelta  
 36:    4    62 [   0] newline 
 37:    5    62 [   0] colspan 18


... but line 5 is the middle of the loop.

I think what would be best is if the retrval were given a location from
the brace that closes the function.  This information is dropped somewhere
in the parser right now.

Comment 4

[Tom Tromey :tromey]

Sorry, I forgot to mention that my line numbers differ because I extracted
the function into a standalone .js file.

Comment 5

[Tom Tromey :tromey]

I have a patch that fixes this by more correctly setting the location
of the retrval.  However, it regresses the test case for bug #936143 --
because now (I believe) there is no way for a function's source notes
to end with newline notes.

Comment 6

[Tom Tromey :tromey]

One oddity about this approach is that, if you are stepping through a function,
you will see an extra stop on the closing brace of the function.  I'm not sure
if this is preferable; the other option would be to mark the retrval instruction
as artificial (following bug 1003554).

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

We already pause once when stepping out of the function to give the user an opportunity to inspect the return value. If the extra stop you mention is in addition to that, then I can't think of any use for it.

Comment 8

[Tom Tromey :tromey]

(In reply to Panos Astithas [:past] from comment #7)
> We already pause once when stepping out of the function to give the user an
> opportunity to inspect the return value. If the extra stop you mention is in
> addition to that, then I can't think of any use for it.

Yeah.  I was thinking about it a bit, though, and one possible rationale is
that if we give the retrval a location, then the user can set a breakpoint on it.
On the other hand if we mark it artificial then that instruction will not appear
as an entry point.

The best approach might be to give the retrval a line number and then change the debugger UI
so that it doesn't do the extra stop when stepping out.

Comment 9

[Tom Tromey :tromey]

> > We already pause once when stepping out of the function to give the user an
> > opportunity to inspect the return value. If the extra stop you mention is in
> > addition to that, then I can't think of any use for it.

Well, ok, it is more complicated than I was thinking.
The return value is available via onPop but not via onStep.
I think I will make the instruction artificial and fix up the various
tests not to assume that the end of the function is an entry point.

Comment 10

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

The one use case I want to maintain is having "step out" from the middle of some function then stop at the last line of that function or the corresponding return or throw or wherever we exit the frame. That's super useful.

Comment 11

[Tom Tromey :tromey]

Attachment: mark the terminating retrval as SRC_ARTIFICIAL

This patch marks the retrval as artificial.  It requires the patch
from bug 1003554.

You'll note there is some fallout in the existing tests.

In the new approach, because the implicit retrval instruction is
artificial, it is not considered an entry point for the purposes of
breakpoint setting.

It's worth noting that these tests are already a bit weird, in that if
you closely examine the reported columns, the final column often
corresponds to something not very useful.

Comment 13

[Tom Tromey :tromey]

Re-testing this using the actual debugger revealed that I regressed the
patch from my original fix.

Now the retrval is artificial, so we don't get a stop there.
But we do see an onPop stop.
So far, so good -- however, onPop appears to stop at the same bad location,
presumably because it does not have any better idea what the last line
of the function might be.

I'm not sure of the best way to fix this.  Maybe we can use the script's
lineCount attribute to inform the onPop display?  That is, treat it as a
display issue only?

Another idea is to change the retrval emission code to emit both an ordinary
line note (pointing at the last token of the function body, like the "}")
and an artificial note.  Due to the way artificial notes are implemented this
may do the right thing.

It all seems a bit rickety though.

Comment 14

[Tom Tromey :tromey]

Attachment: mark the terminating retrval as SRC_ARTIFICIAL

This version of the patch adds a line number note for the artificial
retrval instruction, and then also marks the instruction as
artificial.

The line number is needed so that onPop handlers will work correctly.
What happens is that onPop is notified at the final bytecode, which
now resolves to the location of the final token of the function body.
A minor change to the parser was needed to ensure that the function
body's locations were correct; and this in turn required a minor hack
to the Script-startLine test case.

Marking the instruction as artificial makes it so the final brace
isn't considered an entry point for the purposes of breakpoint
setting.

Comment 15

[Tom Tromey :tromey]

I forgot to note - I also added a test case to cover the bug I
mentioned in comment #13.

Comment 17

[Tom Tromey :tromey]

I was thinking about this recently and came up with another odd case.
If you have a completely empty function, it will appear not to have
any entry points.  The debugger claims you can set breakpoints on
both lines of this function:

function empty() {
}

... but neither of them will be hit.

This is part of the problem in bug 1051632.

The simplest fix here is to not mark the retrval as artificial.
Another possibility would be to only do this special case if the function is
otherwise free of user source lines.  Still thinking about it, but meanwhile
I'll remove the review request here.

Comment 18

[Tom Tromey :tromey]

(In reply to Tom Tromey :tromey from comment #17)

> The simplest fix here is to not mark the retrval as artificial.

I am planning to do this.  Providing this spot as an entry
point for stepping and breakpoint setting will fix the issue
with empty functions.  The downside is that, in some
functions, the user may see an extra stop when stepping -- one
stop due to the retrval line note, and a second due to the onPop
handler.  See bug 923975 for discussion of this.

Comment 19

[Tom Tromey :tromey]

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

Comment 20

[Tom Tromey :tromey]

Attachment: set the line number of the terminating retrval

The new version that doesn't mark the retrval as artificial, but
instead just gives it a sensible line number.

Comment 21

[Jim Blandy :jimb]

Comment on attachment 8542721 [details] [diff] [review]
set the line number of the terminating retrval

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

I am so happy the solution here didn't entail marking anything as artificial.

Comment 22

[Sebastian Zartner [:sebo]]

As the review was positive can we expect this to land soon?
Note that the Firebug Working Group got several reports for that like for example https://groups.google.com/d/topic/firebug/5Wom2FRKbZY/discussion.

Sebastian

Comment 23

[Tom Tromey :tromey]

(In reply to Sebastian Zartner [:sebo] from comment #22)
> As the review was positive can we expect this to land soon?
> Note that the Firebug Working Group got several reports for that like for
> example https://groups.google.com/d/topic/firebug/5Wom2FRKbZY/discussion.

I was waiting for the patch to bug 1003554, as it solves more problems,
and as the patch in this bug hasn't been tested in isolation.
However as it appears that 1003554 will take a while, I will send this
through "try" and, if it works, push it in.

Comment 24

[Tom Tromey :tromey]

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

Comment 25

[Tom Tromey :tromey]

The tests at least depend on the other patch.

Comment 26

[Tom Tromey :tromey]

Attachment: set the line number of the terminating retrval

Rebased.

Comment 27

[Tom Tromey :tromey]

Attachment: set the line number of the terminating retrval

Rebased and updated for changes to the parser.

Comment 28

[Tom Tromey :tromey]

Comment on attachment 8588042 [details] [diff] [review]
set the line number of the terminating retrval

Carrying over the r+ here as the patch has not substantially changed.
(There were a few deletions as the relevant changes were in a different patch.)

Comment 30

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

Note that https://bugzilla.mozilla.org/show_bug.cgi?id=1155966#c9 has a test for the debugger's stepping that we should also land with this fix.

Comment 31

[Tom Tromey :tromey]

Attachment: set the line number of the terminating retrval

This is updated to include Nick's test case.

Comment 32

[Tom Tromey :tromey]

Comment on attachment 8594875 [details] [diff] [review]
set the line number of the terminating retrval

Requesting an additional review for the new test case.

Comment 33

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8594875 [details] [diff] [review]
set the line number of the terminating retrval

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

Test LGTM!

Comment 34

[Tom Tromey :tromey]

Attachment: set the line number of the terminating retrval

Rebased.

Comment 35

[Tom Tromey :tromey]

Attachment: set the line number of the terminating retrval

Update to fix unit tests in devtools/server

Comment 36

[Tom Tromey :tromey]

Comment on attachment 8669974 [details] [diff] [review]
set the line number of the terminating retrval

This needed some additional test updates in devtools/server.

For setBreakpoint-on-column-with-no-offsets-at-end-of-script.js
(and the -line- version), I updated the tests to continue to show
the sliding behavior.

For test_breakpoint-13 and test_breakpoint-14, I had to account
for the extra stop introduced by this patch.  I rewrote these
tests a bit to avoid excessive indentation.

The remaining updates are also about accounting for extra stops or
changed stop locations.

Note this version includes some changes to Frame-onStep-11 and -12.
These just bring the test up to the approved form from the other bug;
but split this way so that the earlier test doesn't show any test
failures.

Comment 37

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8669974 [details] [diff] [review]
set the line number of the terminating retrval

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

r+ for the xpcshell test changes, with comments addressed.

::: devtools/server/tests/unit/setBreakpoint-on-column-with-no-offsets-at-end-of-script.js
@@ -1,4 @@
>  "use strict";
>  
>  function f() {
> -  function g() { var a = 1; var b = 2; } g();

Nit: instead of removing var b = 2; please put the return statement after it.

::: devtools/server/tests/unit/setBreakpoint-on-line-with-no-offsets-at-end-of-script.js
@@ -7,1 @@
>  

Nit: Instead of replacing line 6, please put the return statement on the previously empty line 7 (I left it empty to suggest that there is no offset there).

::: devtools/server/tests/unit/test_breakpoint-13.js
@@ +90,3 @@
>  
> +      let currentTest = 0;
> +      let onPause = function(aEvent, aPacket) {

This is definitely an improvement over the deeply indented code, but you could have written this even cleaner using Task.js, along these lines:

Task.spawn(function* () {
  for (let callback of testCallbacks) {
    yield waitForPause(gThreadClient)
    callback();
  }
});

Anyway, I won't r- over this, so I leave it up to you if you want to do anything with this comment or not.

::: devtools/server/tests/unit/test_breakpoint-14.js
@@ +88,4 @@
>  
> +      let currentTest = 0;
> +      let onPause = function(aEvent, aPacket) {
> +        testCallbacks[currentTest](aPacket);

Similar to what i said about test_breakpoint-13.js

::: devtools/server/tests/unit/test_get-executable-lines.js
@@ -37,5 @@
>      gThreadClient.getSources(function ({error, sources}) {
>        do_check_true(!error);
>        let source = gThreadClient.source(sources[0]);
>        source.getExecutableLines(function(lines){
> -        do_check_true(arrays_equal([2, 3, 5, 6, 7, 8, 12, 14, 16], lines));

I understand why you had to add line 10 here, but why was line 16 already included? How is that different?

::: devtools/server/tests/unit/test_stepping-07.js
@@ +1,1 @@
> +/* Any copyright is dedicated to the Public Domain.

I already reviewed this file during a previous review. It doesn't look like the file changed since then, so I'm carrying over the r+ for that file.

Comment 38

[Tom Tromey :tromey]

(In reply to Eddy Bruel [:ejpbruel] from comment #37)

> ::: devtools/server/tests/unit/test_get-executable-lines.js
> @@ -37,5 @@
> >      gThreadClient.getSources(function ({error, sources}) {
> >        do_check_true(!error);
> >        let source = gThreadClient.source(sources[0]);
> >        source.getExecutableLines(function(lines){
> > -        do_check_true(arrays_equal([2, 3, 5, 6, 7, 8, 12, 14, 16], lines));
> 
> I understand why you had to add line 10 here, but why was line 16 already
> included? How is that different?

The difference has to do with which parse node is considered the end
of the statement.

For the retrval at line 16 we hit this code:

https://dxr.mozilla.org/mozilla-central/source/js/src/frontend/BytecodeEmitter.cpp#8048

The end location of the outermost tree of the script is on line 16.
So, we see a line note to that effect.

However, before this patch, for the function in this patch, the end location
is taken from the "debugger;" statement, and so no new line note is emitted (as
one was already emitted for the debugger statement itself).

Comment 39

[Tom Tromey :tromey]

I realized today that this does the wrong thing for
a function like

function f(a) {
  return a+1;
}

Here, the JSOP_RETURN gets the location of the "return" statement, but I think
for consistency it would be better if it got the location of the "}".

Comment 40

[Tom Tromey :tromey]

I wrote a second patch to set the location of the "return" instruction
to be the location of the "}" that ends the function.  This yields
fairly reasonable stepping behavior when encountering a return.
E.g:

function f() {
  mumble;
  return;
}

Here, if you are stopped at "mumble" and then step, you'll end up
on the "return".  Then a second step will take you to the "}".
Then a third step will stay on the "}" but will show the return value.

Note that I think this is ok but not fantastic -- there is an extra stop in there;
but another approach would involve more hair in the actor if you wanted
to be able to set a breakpoint on the "}".  So, despite the oddity,
I'm currently planning to press on with this.


One other thing I noticed is that this work means that the tests
test_setBreakpoint-on-line-with-no-offsets-at-end-of-script.js
and test_setBreakpoint-on-column-with-no-offsets-at-end-of-script.js
become impossible -- there is no way to have a line or column without
offsets in the middle of a function, because the final instruction
will always end up with the location of the "}".

Comment 41

[Tom Tromey :tromey]

Attachment: set the line number of the terminating retrval

Rebased and updated per review.

Comment 42

[Tom Tromey :tromey]

Attachment: set line number of return instruction

Comment 43

[Tom Tromey :tromey]

Comment on attachment 8677552 [details] [diff] [review]
set line number of return instruction

This changes JSOP_RETURN emission to set the location of the "return"
instruction to the location of the "}" that ends the function.
This makes stepping with an explicit return more consistent with
stepping when there is no return.

See comment #40 for some other notes.

I think the implementation here is pretty ugly, basically spaghetti,
but I couldn't think of anything better :(

I tried making the relevant pn_pos be a parameter of the
BytecodeEmitter constructor, which would have been a good solution.
But this ran across the constructor call in
BytecodeCompiler::createEmitter, which, as far as I can tell,
doesn't have access to the relevant node (the node being created
later, in BytecodeCompiler::compileScript).

Maybe I could overload the BytecodeEmitter constructor and also
keep a flag holding the validity of the new field.  Then the use
of the field could also assert the validity.

What do you think?  Or can you think of something else?  Or is this
ok even though it is ugly?

Comment 44

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

Comment on attachment 8677552 [details] [diff] [review]
set line number of return instruction

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

It seems reasonable to me, but the frontend isn't really my area of expertise. So regarding this stuff[0], I'd like to ask :efaust's opinion.

[0]:

> I tried making the relevant pn_pos be a parameter of the
> BytecodeEmitter constructor, which would have been a good solution.
> But this ran across the constructor call in
> BytecodeCompiler::createEmitter, which, as far as I can tell,
> doesn't have access to the relevant node (the node being created
> later, in BytecodeCompiler::compileScript).
> 
> Maybe I could overload the BytecodeEmitter constructor and also
> keep a flag holding the validity of the new field.  Then the use
> of the field could also assert the validity.

Comment 45

[Tom Tromey :tromey]

Attachment: set function end position on BytecodeEmitter

Here's an addition to the second patch that adds an assert and a
setter.  FWIW the assert helped catch a couple of spots I missed the
first time through.

Comment 46

[Tom Tromey :tromey]

Comment on attachment 8678965 [details] [diff] [review]
set function end position on BytecodeEmitter

Let me know what you think of this approach.
This has to be applied on top of the second patch -- it's an addition,
not a complete replacement.  (I can squash them for you if you want.)

I tend to think it's a bit cleaner than the earlier patch.

Comment 47

[Eric Faust [:efaust]]

Comment on attachment 8678965 [details] [diff] [review]
set function end position on BytecodeEmitter

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

r=me for these changes on top of the previous patch, to land as one.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +145,5 @@
>      insideEval(insideEval),
>      insideNonGlobalEval(insideNonGlobalEval),
>      insideModule(false),
> +    emitterMode(emitterMode),
> +    functionBodyEndPosSet(false)

It's not worth the ugly to make this DEBUG only, but we could in theory. Too bad DebugOnly<bool> still takes up the same space.

Comment 48

[Eric Faust [:efaust]]

Comment on attachment 8677552 [details] [diff] [review]
set line number of return instruction

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

r=me with the second patch, as discussed.

Comment 49

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddaaabd9bcf5

Comment 50

[Tom Tromey :tromey]

Attachment: set line number of return instruction

Comment 51

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=21e0b2270cb5

Comment 52

[Tom Tromey :tromey]

Finally getting back to this.  The oranges turn out to be a latent bug in PCToLineNumber.
It conditionally updates the line number, but unconditionally resets the column number -- oops.
I've fixed this in my patch but I'm still debugging a test hang.

Comment 53

[Tom Tromey :tromey]

Attachment: set the line number of the terminating retrval

Rebased; plus a fix for the PCToLineNumber bug.

Comment 54

[Tom Tromey :tromey]

Attachment: set line number of return instruction

Rebased.

Comment 55

[Tom Tromey :tromey]

Comment on attachment 8701235 [details] [diff] [review]
set line number of return instruction

This patch didn't change, so carrying over the r+

Comment 56

[Tom Tromey :tromey]

Comment on attachment 8701234 [details] [diff] [review]
set the line number of the terminating retrval

Nick, could you review the change to PCToLineNumber?
I rewrote it a bit as I found the old code overly confusing.
Now it is closer to the way BytecodeRangeWithPosition works.
The basic issue is that the code here conditionally updates
the line number, but unconditionally sets the column to 0.
I think this is incorrect.

Comment 57

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

Comment on attachment 8701234 [details] [diff] [review]
set the line number of the terminating retrval

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

Wow, that was a lot of test clean up for what ended up being such a small change. Thanks for slogging through on this one! :)

::: js/src/jit-test/tests/debug/Frame-onPop-23.js
@@ +12,5 @@
> +       "        for(var b=0; b<0; b++) {\n" +  // +2
> +       "            c = 2;\n" +                // +3
> +       "        }\n" +                         // +4
> +       "    }\n" +                             // +5
> +       "}\n");                                 // +6

FYI, `-strings let you have newlines in the middle, so it makes writing snippets like this easier to read.

::: js/src/jsscript.cpp
@@ +3232,5 @@
> +            column = 0;
> +        } else if (type == SRC_NEWLINE) {
> +            lineno++;
> +            column = 0;
> +        } else if (type == SRC_COLSPAN) {

This is so much better without that nested `if (offset <= target)` check which we do anyways for the break. Nice cleanup!

Comment 58

[Tom Tromey :tromey]

Attachment: set the line number of the terminating retrval

Address review comment.

Comment 59

[Tom Tromey :tromey]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #57)

> Wow, that was a lot of test clean up for what ended up being such a small
> change. Thanks for slogging through on this one! :)

Most of those were due to the pre-existing retval/return location change.

The try runs are looking pretty orange still (sigh) so I am considering breaking
out this fix into a separate bug+patch.

Comment 60

[Tom Tromey :tromey]

Attachment: set the line number of the terminating retrval

Rebased; plus moved the PCToLineNumber bits out to a separate bug.

Comment 61

[Tom Tromey :tromey]

Attachment: small test fixes for the retrval patch

Comment 62

[Tom Tromey :tromey]

Attachment: set line number of return instruction

Rebased.

Comment 63

[Tom Tromey :tromey]

Attachment: small test fixes for the return patch

Comment 64

[Tom Tromey :tromey]

Comment on attachment 8704203 [details] [diff] [review]
small test fixes for the retrval patch

This cleans up the test results for the retrval patch.
It is just adjusting some tests to account for the stop-on-"}"
behavior.

I plan to squash this into the retrval patch before committing;
it is just separate to make obvious what has changed.

Comment 65

[Tom Tromey :tromey]

Comment on attachment 8704205 [details] [diff] [review]
small test fixes for the return patch

This cleans up the test results for the return patch.
It is just adjusting some tests to account for the stop-on-"}"
behavior.

I plan to squash this into the return patch before committing;
it is just separate to make obvious what has changed.

Comment 66

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0aef23f6429

Comment 67

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8704203 [details] [diff] [review]
small test fixes for the retrval patch

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

Looks good to me.

Comment 68

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8704205 [details] [diff] [review]
small test fixes for the return patch

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

This one looks good too.

Comment 69

[Tom Tromey :tromey]

Attachment: set the line number of the terminating retrval

Squashed test fixup patch into this one; rebased.

Comment 70

[Tom Tromey :tromey]

Attachment: set line number of return instruction

Squashed test fixup patch into this one; rebased.

Comment 71

[Tom Tromey :tromey]

Attachment: set the line number of the terminating retrval

Rebased again, this time fixing some conflicts.

Comment 72

[Tom Tromey :tromey]

Attachment: set line number of return instruction

Rebased again.

Comment 73

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=81639ce05d2b

Comment 74

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7e311f480d8f
https://hg.mozilla.org/integration/fx-team/rev/6d73df273b78

Comment 75

[Pulsebot]

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

Comment 76

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for XPCshell bustage:

TEST-UNEXPECTED-TIMEOUT | devtools/server/tests/unit/test_stepping-07.js | Test timed out

(Not from this push: TEST-UNEXPECTED-TIMEOUT | devtools/server/tests/unit/test_sourcemaps-03.js | Test timed out)

Failing push: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=6d73df273b78
Log of failing job: https://treeherder.mozilla.org/logviewer.html#?job_id=6514879&repo=fx-team

Comment 77

[Tom Tromey :tromey]

Looking at it.

Comment 78

[Tom Tromey :tromey]

Attachment: set the line number of the terminating retrval

Rebased.

Comment 79

[Tom Tromey :tromey]

Attachment: set line number of return instruction

Rebased.

Comment 80

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ff28b46e756

Comment 81

[Tom Tromey :tromey]

I think the timeouts were caused by some other patch.
Rebasing and re-doing the try run does not show the timeouts.
I'm going to put this in again.

Comment 82

[Pulsebot]

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

Comment 83

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/f71701b82ab0 for xpcshell bustage:

https://treeherder.mozilla.org/logviewer.html#?job_id=6586982&repo=fx-team

Comment 84

[Tom Tromey :tromey]

Sorry.
I don't understand why my try runs don't show this timeout, but I will figure it out.

Comment 85

[Tom Tromey :tromey]

The bug is that head_dbg.js:stepOver is incorrect, causing the new test to fail.
I still don't know why this ever worked.

Comment 86

[Tom Tromey :tromey]

Attachment: set the line number of the terminating retrval

Fix head_dbg.js.

Comment 87

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=55956ece390f

Comment 88

[Tom Tromey :tromey]

Comment on attachment 8708018 [details] [diff] [review]
set the line number of the terminating retrval

Sorry, one more review.  I'm sure we all hope this is the last one.

The only change is the fix in devtools/server/tests/unit/head_dbg.js.
This is needed to avoid a timeout in the new test -- stepOver had to
be adapted to the new approach, per bug 1227978.

Comment 89

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8708018 [details] [diff] [review]
set the line number of the terminating retrval

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

LGTM

::: devtools/server/tests/unit/head_dbg.js
@@ +738,5 @@
> +function stepOver(client, threadClient) {
> +  dumpn("Stepping over.");
> +  const paused = waitForPause(client);
> +  return threadClient.stepOver()
> +    .then(() => paused);

The temporary variable here had me confused for a few seconds. I would just write this code like this:

threadClient.stepOver()
            .then(() => waitForPause(client));

Comment 90

[Tom Tromey :tromey]

Attachment: set the line number of the terminating retrval

Updated per review.

Comment 91

[Pulsebot]

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

Comment 92

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This apparently caused bug 1242827. Backed out in https://hg.mozilla.org/integration/fx-team/rev/a476661db4fb

Comment 93

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a877772255f6
https://hg.mozilla.org/mozilla-central/rev/a97c7103effe

Comment 94

[Tom Tromey :tromey]

This was backed out, so it can't be fixed.

Comment 97

[Tom Tromey :tromey]

Today's attempt to reproduce the failure:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d6bb110f8b0

Comment 98

[Tom Tromey :tromey]

Another attempt; rebased and with more platforms.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c9a31face99

Comment 99

[Tom Tromey :tromey]

One more rebase and try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f99b1a1d10da

Comment 100

[Tom Tromey :tromey]

Attachment: set the line number of the terminating retrval

Rebased.

Comment 101

[Tom Tromey :tromey]

Attachment: set line number of return instruction

Rebased.

Comment 102

[Tom Tromey :tromey]

Fuller try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d3a725ccd63

Comment 103

[Tom Tromey :tromey]

I can't seem to reproduce the intermittent, despite many tries.
So, I'm re-requesting checkin of these patches.

Comment 104

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5777c32d2b39
https://hg.mozilla.org/integration/fx-team/rev/1b45c030f024

Comment 105

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5777c32d2b39
https://hg.mozilla.org/mozilla-central/rev/1b45c030f024

Comment 106

[Tom S [:evilpie]]

This is now only fixed in 48 right?

Comment 107

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Tom Schuster [:evilpie] from comment #106)
> This is now only fixed in 48 right?

Yes, that's my understanding as well.

Comment 108

[Tom Tromey :tromey]

Yes, the milestone got set in comment #93, but then not reset when this was backed out.

Comment 109

[Sebastian Zartner [:sebo]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Important when debugging JavaScript
[Suggested wording]: Improved step debugging on last line of functions (a bit poor, maybe someone can find a better phrasing)
[Links (documentation, blog post, etc)]: None yet

Sebastian

Comment 110

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx48 Aurora release notes

Comment 111

[Sebastian Zartner [:sebo]]

I believe the release note should rather be put into a 'fixed' section.

Sebastian

Comment 112

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #107)
> (In reply to Tom Schuster [:evilpie] from comment #106)
> > This is now only fixed in 48 right?
> 
> Yes, that's my understanding as well.

Done!