Closed Bug 1013219 Opened 7 years ago Closed 5 years ago

Stepping on the last line in a frame might be confusing

Categories

(DevTools :: Debugger, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

(firefox48 fixed, relnote-firefox 48+)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed
relnote-firefox --- 48+

People

(Reporter: Honza, Assigned: tromey)

References

(Depends on 1 open bug)

Details

(Whiteboard: [firebug-p2][polish-backlog])

Attachments

(2 files, 27 obsolete files)

37.15 KB, patch
tromey
: review+
Details | Diff | Splinter Review
22.10 KB, patch
tromey
: review+
Details | Diff | Splinter Review
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
Whiteboard: [firebug-p2]
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
This (stepping to last line of a loop) is true for all kinds of for and while loops since very long ..
Summary: [jsdbg2] Stepping on the last line in a frame might be confusing → Stepping on the last line in a frame might be confusing
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.
Sorry, I forgot to mention that my line numbers differ because I extracted
the function into a standalone .js file.
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.
Assignee: nobody → ttromey
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).
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.
(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.
> > 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.
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.
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.
Duplicate of this bug: 1093738
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.
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.
Attachment #8535811 - Attachment is obsolete: true
I forgot to note - I also added a test case to cover the bug I
mentioned in comment #13.
Attachment #8536713 - Flags: review?(jimb)
Duplicate of this bug: 1052738
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.
Attachment #8536713 - Flags: review?(jimb)
(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.
The new version that doesn't mark the retrval as artificial, but
instead just gives it a sensible line number.
Attachment #8536713 - Attachment is obsolete: true
Attachment #8542721 - Flags: review?(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.
Attachment #8542721 - Flags: review?(jimb) → review+
Blocks: 1052738
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
Flags: needinfo?(ttromey)
(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.
Flags: needinfo?(ttromey)
The tests at least depend on the other patch.
Rebased.
Attachment #8542721 - Attachment is obsolete: true
Depends on: 1003554
Rebased and updated for changes to the parser.
Attachment #8562381 - Attachment is obsolete: true
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.)
Attachment #8588042 - Flags: review+
Duplicate of this bug: 1155966
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.
This is updated to include Nick's test case.
Comment on attachment 8594875 [details] [diff] [review]
set the line number of the terminating retrval

Requesting an additional review for the new test case.
Attachment #8594875 - Flags: review?(ejpbruel)
Attachment #8588042 - Attachment is obsolete: true
Comment on attachment 8594875 [details] [diff] [review]
set the line number of the terminating retrval

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

Test LGTM!
Attachment #8594875 - Flags: review?(ejpbruel) → review+
Whiteboard: [firebug-p2] → [firebug-p2][polish-backlog]
Rebased.
Attachment #8594875 - Attachment is obsolete: true
Attachment #8668026 - Flags: review+
Update to fix unit tests in devtools/server
Attachment #8668026 - Attachment is obsolete: true
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.
Attachment #8669974 - Flags: review?(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.
Attachment #8669974 - Flags: review?(ejpbruel) → review+
(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).
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 "}".
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 "}".
Rebased and updated per review.
Attachment #8669974 - Attachment is obsolete: true
Attachment #8677549 - Flags: review+
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?
Attachment #8677552 - Flags: review?(nfitzgerald)
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.
Attachment #8677552 - Flags: review?(nfitzgerald)
Attachment #8677552 - Flags: review?(efaustbmo)
Attachment #8677552 - Flags: review+
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 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.
Attachment #8678965 - Flags: feedback?(efaustbmo)
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.
Attachment #8678965 - Flags: feedback?(efaustbmo) → review+
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.
Attachment #8677552 - Flags: review?(efaustbmo) → review+
Attachment #8677552 - Attachment is obsolete: true
Attachment #8678965 - Attachment is obsolete: true
Attachment #8679053 - Flags: review+
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.
Rebased; plus a fix for the PCToLineNumber bug.
Attachment #8677549 - Attachment is obsolete: true
Attachment #8679053 - Attachment is obsolete: true
Rebased.
Comment on attachment 8701235 [details] [diff] [review]
set line number of return instruction

This patch didn't change, so carrying over the r+
Attachment #8701235 - Flags: review+
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.
Attachment #8701234 - Flags: review?(nfitzgerald)
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!
Attachment #8701234 - Flags: review?(nfitzgerald) → review+
Address review comment.
Attachment #8701234 - Attachment is obsolete: true
Attachment #8702307 - Flags: review+
(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.
Depends on: 1235636
Blocks: 1172572
Rebased; plus moved the PCToLineNumber bits out to a separate bug.
Attachment #8701235 - Attachment is obsolete: true
Attachment #8702307 - Attachment is obsolete: true
Rebased.
Attachment #8704202 - Flags: review+
Attachment #8704204 - Flags: review+
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.
Attachment #8704203 - Flags: review?(ejpbruel)
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.
Attachment #8704205 - Flags: review?(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.
Attachment #8704203 - Flags: review?(ejpbruel) → review+
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.
Attachment #8704205 - Flags: review?(ejpbruel) → review+
Squashed test fixup patch into this one; rebased.
Attachment #8704202 - Attachment is obsolete: true
Attachment #8704203 - Attachment is obsolete: true
Attachment #8704204 - Attachment is obsolete: true
Attachment #8704205 - Attachment is obsolete: true
Squashed test fixup patch into this one; rebased.
Attachment #8705172 - Flags: review+
Attachment #8705173 - Flags: review+
Rebased again, this time fixing some conflicts.
Attachment #8705172 - Attachment is obsolete: true
Attachment #8705173 - Attachment is obsolete: true
Rebased again.
Attachment #8705233 - Flags: review+
Attachment #8705234 - Flags: review+
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
Flags: needinfo?(ttromey)
Looking at it.
Flags: needinfo?(ttromey)
Rebased.
Rebased.
Attachment #8706422 - Flags: review+
Attachment #8705233 - Attachment is obsolete: true
Attachment #8706423 - Flags: review+
Attachment #8705234 - Attachment is obsolete: true
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.
Sorry.
I don't understand why my try runs don't show this timeout, but I will figure it out.
Flags: needinfo?(ttromey)
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.
Fix head_dbg.js.
Attachment #8706422 - Attachment is obsolete: true
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.
Attachment #8708018 - Flags: review?(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));
Attachment #8708018 - Flags: review?(ejpbruel) → review+
Updated per review.
Attachment #8708018 - Attachment is obsolete: true
Attachment #8711793 - Flags: review+
Depends on: 1242827
https://hg.mozilla.org/mozilla-central/rev/a877772255f6
https://hg.mozilla.org/mozilla-central/rev/a97c7103effe
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
This was backed out, so it can't be fixed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Duplicate of this bug: 1249910
Duplicate of this bug: 1255631
Today's attempt to reproduce the failure:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d6bb110f8b0
Another attempt; rebased and with more platforms.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c9a31face99
Rebased.
Attachment #8706423 - Attachment is obsolete: true
Attachment #8711793 - Attachment is obsolete: true
Rebased.
Attachment #8735534 - Flags: review+
Attachment #8735535 - Flags: review+
I can't seem to reproduce the intermittent, despite many tries.
So, I'm re-requesting checkin of these patches.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5777c32d2b39
https://hg.mozilla.org/mozilla-central/rev/1b45c030f024
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
This is now only fixed in 48 right?
(In reply to Tom Schuster [:evilpie] from comment #106)
> This is now only fixed in 48 right?

Yes, that's my understanding as well.
Target Milestone: Firefox 47 → Firefox 48
Yes, the milestone got set in comment #93, but then not reset when this was backed out.
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
relnote-firefox: --- → ?
I believe the release note should rather be put into a 'fixed' section.

Sebastian
Flags: needinfo?(rkothari)
(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!
Flags: needinfo?(rkothari)
Duplicate of this bug: 1266964
Product: Firefox → DevTools
Blocks: 1552532
No longer blocks: 1552532
You need to log in before you can comment on or make changes to this bug.