Closed Bug 943356 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 30

People

(Reporter: vporof, Assigned: b4bomsy)

Details

(Whiteboard: [good first bug][lang=js][mentor=fitzgen])

Attachments

(1 file, 2 obsolete files)

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.
Thank you for filing this, it has been very annoying when doing demos lately.
Would like to work on this.
Go for it! \m/ (^_^) \m/
Assignee: nobody → b4bomsy
setting to assigned.
Status: NEW → ASSIGNED
Priority: -- → P3
(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.
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.
> 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
Nick, can you please mentor this?
Whiteboard: [good first bug][lang=js][mentor=fitzgen]
(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.
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;
> }

(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.
Attached patch 943356fix.patch (obsolete) — Splinter Review
Added a test as well. is this ok??
Attachment #8378832 - Flags: feedback?(nfitzgerald)
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.
Attachment #8378832 - Flags: feedback?(nfitzgerald) → feedback+
Attached patch 943356fix.patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=110777e26938
Attachment #8378832 - Attachment is obsolete: true
Attachment #8380419 - Flags: review?(nfitzgerald)
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.
Attachment #8380419 - Flags: review?(nfitzgerald) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/c97d84ca91ca
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js][mentor=fitzgen] → [good first bug][lang=js][mentor=fitzgen][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c97d84ca91ca
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][mentor=fitzgen][fixed-in-fx-team] → [good first bug][lang=js][mentor=fitzgen]
Target Milestone: --- → Firefox 30
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.