943356 - Prettifying a source while paused shouldn't switch away from it

Status: VERIFIED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Victor Porof [:vporof][:vp]]

STR:

1. Go to http://htmlpad.org/debugger/
2. Open debugger
3. Click me
4. Select jquery from the sources list
5. Click the {} icon

The pretty printing worked, but the debugger/ source got automatically selected. This is because of the debugger's paused state. 

I vaguely remember this used to work properly, so it might be a recent regression. Anyway, we need more tests.

Comment 1

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

Thank you for filing this, it has been very annoying when doing demos lately.

Comment 2

[Hubert B Manilla]

Would like to work on this.

Comment 3

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

Go for it! \m/ (^_^) \m/

Comment 4

[Rob Campbell [:rc] (:robcee)]

setting to assigned.

Comment 5

[Hubert B Manilla]

(In reply to Victor Porof [:vporof][:vp] from comment #0)
> STR:
> 
> 1. Go to http://htmlpad.org/debugger/
> 2. Open debugger
> 3. Click me
> 4. Select jquery from the sources list
> 5. Click the {} icon
> 
> The pretty printing worked, but the debugger/ source got automatically
> selected. This is because of the debugger's paused state. 
> 
> I vaguely remember this used to work properly, so it might be a recent
> regression. Anyway, we need more tests.

interestingly, the chrome devtools has the same behaviour, when testing this.

Comment 6

[Hubert B Manilla]

Looking into this, i noticed that if the active thread is paused, and we try to select and prettyPrint a different source, the _onPrettyPrintChange event is triggered and a fillFrames is called on the active thread(1), which in turn triggers a framescleared event(2) which causes the debugger to reset the pause and flip the source which contains the frame.


(In reply to Hubert B Manilla from comment #5)
> (In reply to Victor Porof [:vporof][:vp] from comment #0)
> > STR:
> > 
> > 1. Go to http://htmlpad.org/debugger/
> > 2. Open debugger
> > 3. Click me
> > 4. Select jquery from the sources list
> > 5. Click the {} icon
> > 
> > The pretty printing worked, but the debugger/ source got automatically
> > selected. This is because of the debugger's paused state. 
> > 
> > I vaguely remember this used to work properly, so it might be a recent
> > regression. Anyway, we need more tests.
> 
> interestingly, the chrome devtools has the same behaviour, when testing this.

Also i noticed that  blackboxing the jquery source after step 4 causes the breakpoint on the breakpoint to disappear, i'm thinking this might also be a bug.

Comment 7

[Hubert B Manilla]

> Looking into this, i noticed that if the active thread is paused, and we try
> to select and prettyPrint a different source, the _onPrettyPrintChange event
> is triggered and a fillFrames is called on the active thread(1), which in
> turn triggers a framesadded event(2) which causes the debugger to reset
> the pause and flip the source which contains the frame.
> 
Sorry framesadded not framescleared.

1) http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#794
2) http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#655

Comment 8

[Victor Porof [:vporof][:vp]]

Nick, can you please mentor this?

Comment 9

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

(In reply to Hubert B Manilla from comment #6)
> Also i noticed that  blackboxing the jquery source after step 4 causes the
> breakpoint on the breakpoint to disappear, i'm thinking this might also be a
> bug.

We disable and hide all breakpoints when a source is blackboxed, so this is expected.

Comment 10

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

This is kind of gross because we are bouncing back and forth between the frontend methods through thread events and so there is no easy way to just add a "don't auto switch sources" flag :(

I think the simplest approach would be to add an optional callback parameter to the client's |fillFrames| method and then simply ensure that the source being prettified is selected in a callback passed to |fillFrames| when called from |_onPrettyPrintChange|. Make sense? So |_onPrettyPrintChange| would look something like this:

> _onPrettyPrintChange: function() {
>   const source = DebuggerView.Sources.selectedValue;
>   if (this.activeThread.state == "paused") {
>     this.activeThread.fillFrames(CALL_STACK_PAGE_SIZE,
>                                  () => DebuggerView.Sources.selectedValue = source);
>   }
> },

Would probably warrent a quick comment describing why it is necessary.

Also, make sure you call the |fillFrames| callback when there is an error from the RDP after you call |getFrames|. We don't even check for an error now, which is a minor bug in itself. Add something like this:

> if (aResponse.error) {
>   aCallback(aResponse);
>   return;
> }

Comment 11

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

warrant*

Comment 12

[Hubert B Manilla]

(In reply to Nick Fitzgerald [:fitzgen] [Ðoge:D6jomNp59N9TVfgc538HU3RswwmwZwcrd7] from comment #10)
> This is kind of gross because we are bouncing back and forth between the
> frontend methods through thread events and so there is no easy way to just
> add a "don't auto switch sources" flag :(
> 
> I think the simplest approach would be to add an optional callback parameter
> to the client's |fillFrames| method and then simply ensure that the source
> being prettified is selected in a callback passed to |fillFrames| when
> called from |_onPrettyPrintChange|. Make sense? So |_onPrettyPrintChange|
> would look something like this:
> 
> > _onPrettyPrintChange: function() {
> >   const source = DebuggerView.Sources.selectedValue;
> >   if (this.activeThread.state == "paused") {
> >     this.activeThread.fillFrames(CALL_STACK_PAGE_SIZE,
> >                                  () => DebuggerView.Sources.selectedValue = source);
> >   }
> > },
> 
> Would probably warrent a quick comment describing why it is necessary.
> 
> Also, make sure you call the |fillFrames| callback when there is an error
> from the RDP after you call |getFrames|. We don't even check for an error
> now, which is a minor bug in itself. Add something like this:
> 
> > if (aResponse.error) {
> >   aCallback(aResponse);
> >   return;
> > }

Thanks Nick, Quite clear.

Comment 13

[Hubert B Manilla]

Attachment: 943356fix.patch

Added a test as well. is this ok??

Comment 14

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

Comment on attachment 8378832 [details] [diff] [review]
943356fix.patch

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

Looking good! The approach is correct, I just have a few nitpicks. Fix it up and flag me again for review :)

::: browser/devtools/debugger/debugger-controller.js
@@ +795,3 @@
>      if (this.activeThread.state == "paused") {
> +      this.activeThread.fillFrames(CALL_STACK_PAGE_SIZE, 
> +                                   () => DebuggerView.Sources.selectedValue = source);

Style nits:

* Space between "//" and "Makes" in that comment, please.

* Trailing space after "CALL_STACK_PAGE_SIZE" (set up your editor to highlight trailing whitespace for you; you'll save a ton of time)

* Wrap lines at 80 chars; comment and the fillFrames parameters.

I think this would be better:

this.activeThread.fillFrames(
  CALL_STACK_PAGE_SIZE,
  () => DebuggerView.Sources.selectedValue = source
);

::: browser/devtools/debugger/test/browser_dbg_pretty-print-on-paused.js
@@ +22,5 @@
> +    gEditor = gDebugger.DebuggerView.editor;
> +    gSources = gDebugger.DebuggerView.Sources;
> +    gPrefs = gDebugger.Prefs;
> +    gOptions = gDebugger.DebuggerView.Options;
> +    gView = gDebugger.DebuggerView;

Nit: only define gSomething if you're using it in the test.

@@ +25,5 @@
> +    gOptions = gDebugger.DebuggerView.Options;
> +    gView = gDebugger.DebuggerView;
> +
> +    addBreakpoint();
> +    

Nit: trailing white space.

@@ +42,5 @@
> +        testProgressBarShown();
> +        return finished;
> +      })
> +      .then(testSecondSourceIsStillSelected)
> +      .then(() => closeDebuggerAndFinish(gPanel))

I think you need to call |resumeDebuggerThenCloseAndFinish| instead of |closeDebuggerAndFinish| because you're still paused at the end of the test, right?

@@ +52,5 @@
> +  });
> +}
> +function addBreakpoint() {
> +  return promise.resolve(null)
> +    .then(() => gPanel.addBreakpoint({ url: gSources.values[0], line: 6 })) 

Nit: trailing whitespace and no semicolon.

@@ +62,5 @@
> +}
> +
> +function testSecondSourceIsSelected(){
> +  ok(gSources.containsValue(EXAMPLE_URL + gSecondSourceLabel),
> +    "The second source should be selected.");  

Nit: trailing white space.

@@ +76,5 @@
> +}
> +
> +function testSecondSourceIsStillSelected(){
> +  ok(gSources.containsValue(EXAMPLE_URL + gSecondSourceLabel),
> +    "The second source should still be selected.")  

Nit: trailing white space and no semicolon.

::: toolkit/devtools/client/dbg-client.jsm
@@ +1591,5 @@
>      let numFrames = this._frameCache.length;
>  
>      this.getFrames(numFrames, aTotal - numFrames, (aResponse) => {
> +      if (aResponse.error) {
> +        if (typeof aCallback === "function") {

We could remove this extra type check by having fillFrames' parameters declared like so:

  fillFrames: function (aTotal, aCallback=noop) {
    ...
  }

and defining noop at the bottom of the file:

  function noop() {}

@@ +1608,2 @@
>  
> +      if (typeof aCallback === "function") {

Ditto for this type check.

Comment 15

[Hubert B Manilla]

Attachment: 943356fix.patch

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

Comment 16

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

Comment on attachment 8380419 [details] [diff] [review]
943356fix.patch

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

Looks good!

Fix up the comment below and add the "checkin-needed" keyword when you re-attach the new patch :)

::: browser/devtools/debugger/test/browser_dbg_pretty-print-on-paused.js
@@ +49,5 @@
> +}
> +
> +function addBreakpoint() {
> +  return promise.resolve(null)
> +    .then(() => gPanel.addBreakpoint({ url: gSources.values[0], line: 6 }));

You can just 

  return gPanel.addBreakpoint(...);

or even just inline that where you call addBreakpoint() now.

Comment 17

[Hubert B Manilla]

Attachment: Prettifying a source while paused will not switch away from it.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c97d84ca91ca