Can't reopen toolbox after manually pausing and then stepping in the debugger

VERIFIED FIXED in Firefox 41

Status

defect
P1
normal
VERIFIED FIXED
4 years ago
Last year

People

(Reporter: jfong, Assigned: jlong)

Tracking

42 Branch
Firefox 43
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox39 affected, firefox40 affected, firefox41+ verified, firefox42+ verified, firefox43 verified)

Details

Attachments

(1 attachment, 3 obsolete attachments)

STR:

1. Open browser, load any page that has JS
2. Go to a JS source file and prettify it
3. Set a breakpoint somewhere
4. Click on pause, then step over
5. Close the toolbox
6. Can't reopen the toolbox

--

Error in local build during #4:

onPacket threw an exception: Error: Server did not specify an actor, dropping packet: {"error":"unknownError","message":"error occurred while processing 'resume: TypeError: generatedSourceActor is null\nStack: TabSources.prototype.getOriginalLocation@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/utils/TabSources.js:571:9\nThreadActor.prototype._handleResumeLimit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:903:12\nThreadActor.prototype.onResume@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:1009:28\nDSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1593:15\nChildDebuggerTransport.prototype.receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:742:5\nEventLoop.prototype.enter@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:339:5\nThreadActor.prototype._pushThreadPause@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:534:5\nThreadActor.prototype.onInterrupt@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:1370:7\nDSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1593:15\nChildDebuggerTransport.prototype.receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:742:5\nLine: 571, column: 9"}
Stack: DebuggerClient.prototype.onPacket@resource://gre/modules/devtools/dbg-client.jsm:969:1
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:569:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14

---

Error in local build during attempt to reopen the toolbox in #6:

*************************
A coding exception was thrown in a Promise resolution callback.
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Full message: TypeError: this.doc is undefined
Full stack: Toolbox.prototype.selectTool@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:1233:9
DevTools.prototype.showToolbox/hostPromise<@resource:///modules/devtools/gDevTools.jsm:386:18
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:801:7
this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:740:39
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:740:7
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:764:7
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:447:5
DevTools.prototype.showToolbox@resource:///modules/devtools/gDevTools.jsm:385:23
gDevToolsBrowser.selectToolCommand@resource:///modules/devtools/gDevTools.jsm:670:7
oncommand@chrome://browser/content/browser.xul:1:1

*************************
Even easier STR:

Open: data:text/html,hi
Open Debugger
Press 'Pause'
Press 'Step Over'
Close toolbox

Expected:
I can reopen the toolbox

Actual:
I can't reopen the toolbox
Summary: Can't reopen toolbox after interacting with the debugger → Can't reopen toolbox after manually pausing and then stepping in the debugger
This seems pretty bad since it's easy to trigger (see Comment 2) and you can no longer open the toolbox for that tab after it happens.  James, any idea what's going on here?
Flags: qe-verify+
Flags: needinfo?(jlong)
Priority: -- → P1
Depending on the fix ( and timing ), let's see how far up the chain we can uplift. I would like to aim for Beta 41 if possible.
Sorry this dropped off my radar! I will look tomorrow.

(I'm so eager to clean up the debugger! hopefully will land some big stability fixes this quarter & next)
Flags: needinfo?(jlong)
This should be an easy fix, I'll have a patch tomorrow. Should be very easy to uplift.

The problem is that the debugger isn't paused at any particular location, and it's sending an RDP request assuming it has a location to step from. The backend assumes this location exists and it throws an error. I guess throwing an error in that particular play kills the whole toolbox, which isn't good. I'll see if there's a way to make errors in TabSources not do that, but for now I'll fix those buttons.

I'm going to just disable those buttons if you're not paused at a location, which makes the most UX sense.
Posted patch 1191916.patch (obsolete) — Splinter Review
Attachment #8647760 - Flags: review?(past)
Assignee: nobody → jlong
Comment on attachment 8647760 [details] [diff] [review]
1191916.patch

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

::: browser/devtools/debugger/views/toolbar-view.js
@@ +144,5 @@
> +    ];
> +    for(let button of buttons) {
> +      button.disabled = !enabled;
> +      if(enabled) {
> +        button.classList.add('btn-enabled');

Suggestion: You could use setAttribute("disabled", "true") / removeAttribute("disabled") which would also prevent hover states due to UA styles when it's disabled.

Then in CSS you could do :not([disabled]) instead of .btn-enable for additional styling
(In reply to Brian Grinstead [:bgrins] from comment #9)
> Comment on attachment 8647760 [details] [diff] [review]
> 1191916.patch
> 
> Review of attachment 8647760 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/debugger/views/toolbar-view.js
> @@ +144,5 @@
> > +    ];
> > +    for(let button of buttons) {
> > +      button.disabled = !enabled;
> > +      if(enabled) {
> > +        button.classList.add('btn-enabled');
> 
> Suggestion: You could use setAttribute("disabled", "true") /
> removeAttribute("disabled") which would also prevent hover states due to UA
> styles when it's disabled.
> 
> Then in CSS you could do :not([disabled]) instead of .btn-enable for
> additional styling

I'll try that again, but unfortunately that not seem to actually disable the button? Maybe I was doing something wrong. I'll try it again.
Comment on attachment 8647760 [details] [diff] [review]
1191916.patch

Also clearing r? until I write tests
Attachment #8647760 - Flags: review?(past)
(In reply to James Long (:jlongster) from comment #10)
> (In reply to Brian Grinstead [:bgrins] from comment #9)
> > Comment on attachment 8647760 [details] [diff] [review]
> > 1191916.patch
> > 
> > Review of attachment 8647760 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/devtools/debugger/views/toolbar-view.js
> > @@ +144,5 @@
> > > +    ];
> > > +    for(let button of buttons) {
> > > +      button.disabled = !enabled;
> > > +      if(enabled) {
> > > +        button.classList.add('btn-enabled');
> > 
> > Suggestion: You could use setAttribute("disabled", "true") /
> > removeAttribute("disabled") which would also prevent hover states due to UA
> > styles when it's disabled.
> > 
> > Then in CSS you could do :not([disabled]) instead of .btn-enable for
> > additional styling
> 
> I'll try that again, but unfortunately that not seem to actually disable the
> button? Maybe I was doing something wrong. I'll try it again.

Yeah I think you'll still need to handle checking .disabled in handlers.  Also, I just noticed you were already doing button.disabled = !enabled and that already should set / remove the attribute so you may just be able to remove class setting code altogether.
bgrins, you're right, I can still select on `disabled` since I do `element.disabled = true`. Thanks!
Posted patch 1191916.patch (obsolete) — Splinter Review
Added a test
Attachment #8647760 - Attachment is obsolete: true
Attachment #8648109 - Flags: review?(past)
Comment on attachment 8648109 [details] [diff] [review]
1191916.patch

Pretty small patch, adding bgrins as another reviewer so whoever gets to it first can review. Brian, this touches enough CSS and testing stuff that it would be useful for you to take a look.
Attachment #8648109 - Flags: review?(bgrinstead)
Comment on attachment 8648109 [details] [diff] [review]
1191916.patch

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

LGTM, I'd like to take one more look before landing so please flag me on the next version

::: browser/devtools/debugger/test/browser_dbg_pause-no-step.js
@@ +6,5 @@
> + */
> +
> +const TAB_URL = EXAMPLE_URL + "doc_pause-exceptions.html";
> +
> +let gTab, gPanel, gDebugger;

I guess none of the debugger tests have been converted to add_task yet?

@@ +21,5 @@
> +    gStepInButton = gDebugger.document.getElementById("step-in");
> +    gStepOutButton = gDebugger.document.getElementById("step-out");
> +    gResumeKey = gDebugger.document.getElementById("resumeKey");
> +    gFrames = gDebugger.DebuggerView.StackFrames;
> +    gSources = gDebugger.DebuggerView.Sources;

gTab and gSources aren't used in this test - I'm guessing they were copied in as boilerplate but they could be removed.  Not sure of the style for the debugger tests, so maybe it's intentional but I'd remove them personally.

@@ +45,5 @@
> +    // Nothing should happen here because the button is disabled. If
> +    // this runs any code, there will be errors and the test will fail.
> +    EventUtils.sendMouseEvent({ type: "mousedown" }, gStepOverButton, gDebugger);
> +
> +    is(gDebugger.gThreadClient.paused, true,

Could just use ok(gDebugger.gThreadClient.paused) / ok(!gDebugger.gThreadClient.paused) instead of is to get everything onto one line.  But it doesn't really matter to me if you change that

::: browser/devtools/debugger/views/toolbar-view.js
@@ +110,5 @@
>  
>    /**
>     * Sets the resume button state based on the debugger active thread.
>     *
>     * @param string aState

Params need updating in comment

@@ +120,5 @@
>        this._resumeButton.setAttribute("checked", "true");
>        this._resumeButton.setAttribute("tooltiptext", this._resumeTooltip);
> +
> +      // Only enable the stepping buttons if we are paused at a
> +      // specific location. In the future, hopefully we'll always be

Replace "in the future, hopefully" with "After Bug 789430",

@@ +123,5 @@
> +      // Only enable the stepping buttons if we are paused at a
> +      // specific location. In the future, hopefully we'll always be
> +      // paused at a location, but currently you can pause the entire
> +      // engine at any point without knowing the location.
> +      if(hasLocation) {

Nit: whitespace - if ()

@@ +141,5 @@
> +      this._stepOutButton,
> +      this._stepInButton,
> +      this._stepOverButton
> +    ];
> +    for(let button of buttons) {

Nit: whitespace  for ()

::: browser/themes/shared/devtools/debugger.css
@@ +575,5 @@
>      -moz-image-region: rect(0px,64px,32px,32px);
>    }
>  }
>  
> +#debugger-controls toolbarbutton {

Nice, these are more clear than the previous selectors
Attachment #8648109 - Flags: review?(bgrinstead)
Status: NEW → ASSIGNED
Posted patch 1191916.patch (obsolete) — Splinter Review
Thanks bgrins! Fixed everything you mentioned.
Attachment #8648109 - Attachment is obsolete: true
Attachment #8648109 - Flags: review?(past)
Attachment #8648135 - Flags: review?(bgrinstead)
Comment on attachment 8648135 [details] [diff] [review]
1191916.patch

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

Nice, this should also fix the hover / active states for stepping buttons on a non-paused debugger

::: browser/devtools/debugger/test/browser_dbg_pause-no-step.js
@@ +64,5 @@
> +
> +  BrowserTestUtils.synthesizeMouseAtCenter('button', {}, gBrowser.selectedBrowser);
> +}
> +
> +

Nit: extra empty line
Attachment #8648135 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #19)
> Comment on attachment 8648135 [details] [diff] [review]
> 1191916.patch
> 
> Review of attachment 8648135 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice, this should also fix the hover / active states for stepping buttons on
> a non-paused debugger
> 

Yep, it does! It feels a lot better now. Didn't know we weren't actually disabling the buttons.
Posted patch 1191916.patchSplinter Review
whitespace change
Attachment #8648135 - Attachment is obsolete: true
This should not affect xpcshell tests at all. It only tweaks the frontend, which does not run any xpcshell tests. It looks like running xpcshell tests on windows is just plain broken, so those oranges don't apply here.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7d66a11d81a4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
[Tracking Requested - why for this release]: Tracking so we don't forget to uplift this (assuming it's reasonable to do so)
Jen, can you please confirm that this issue is fixed for you on the latest Nightly build? Thanks.
Flags: needinfo?(jfong)
James, please request patch uplift to Aurora and Beta whenever possible.
Flags: needinfo?(jlong)
(In reply to Ritu Kothari (:ritu) from comment #28)
> Jen, can you please confirm that this issue is fixed for you on the latest
> Nightly build? Thanks.

confirmed!
Flags: needinfo?(jfong)
(In reply to Jen Fong-Adwent [:ednapiranha] from comment #30)
> (In reply to Ritu Kothari (:ritu) from comment #28)
> > Jen, can you please confirm that this issue is fixed for you on the latest
> > Nightly build? Thanks.
> 
> confirmed!

Thanks. Setting status to "Verified".
Status: RESOLVED → VERIFIED
Comment on attachment 8648335 [details] [diff] [review]
1191916.patch

Approval Request Comment
[Feature/regressing bug #]: NA
[User impact if declined]: only affects devtools debugger usage, but there's an easy way to put it in a broken state and devtools can't even open anymore
[Describe test coverage new/current, TreeHerder]: added another test, landed on m-c a while ago
[Risks and why]: not much rish
[String/UUID change made/needed]:
Flags: needinfo?(jlong)
Attachment #8648335 - Flags: approval-mozilla-aurora?
James, please request uplift to Beta whenever you get a chance. I noticed that you have only requested uplift to Aurora.
Flags: needinfo?(jlong)
Comment on attachment 8648335 [details] [diff] [review]
1191916.patch

The fix seems non risky and has been verified on Nightly. Approved for uplift to Aurora.
Attachment #8648335 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8648335 [details] [diff] [review]
1191916.patch

Approval Request Comment
[Feature/regressing bug #]: NA
[User impact if declined]: only affects devtools debugger usage, but there's an easy way to put it in a broken state and devtools can't even open anymore
[Describe test coverage new/current, TreeHerder]: includes new test, landed on m-c for a while
[Risks and why]: not much risk
[String/UUID change made/needed]:
Flags: needinfo?(jlong)
Attachment #8648335 - Flags: approval-mozilla-beta?
Comment on attachment 8648335 [details] [diff] [review]
1191916.patch

Safe to uplift to Beta41.
Attachment #8648335 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Environments: Win 7 64-bit and Mac OS X 10.9.5

Verified using Firefox 41 beta 6 and latest Dev Edition 42.0a2 that the "Step Over/In/Out" buttons are disabled and the Debugger can be closed and re-opened.

I noticed another issue while using the url from comment 2: Opening the debugger on this page and adding a Breakpoint leads to inability to close the browser console. It reproduces back to Firefox 38.0.5 so it's not a regression from this fix.
Brian, is this something that needs to be fill? Thank you
Flags: needinfo?(bgrinstead)
(In reply to Petruta Rasa [QA] [:petruta] from comment #39)
> I noticed another issue while using the url from comment 2: Opening the
> debugger on this page and adding a Breakpoint leads to inability to close
> the browser console. It reproduces back to Firefox 38.0.5 so it's not a
> regression from this fix.
> Brian, is this something that needs to be fill? Thank you

Interesting problem - yes you could please file a new bug for that?
Flags: needinfo?(bgrinstead)
Thanks, Brian!

I logged bug 1201008. Sorry, in comment 39 I meant web console, not browser console.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.