Closed
Bug 1191916
Opened 10 years ago
Closed 9 years ago
Can't reopen toolbox after manually pausing and then stepping in the debugger
Categories
(DevTools :: Debugger, defect, P1)
Tracking
(firefox39 affected, firefox40 affected, firefox41+ verified, firefox42+ verified, firefox43 verified)
VERIFIED
FIXED
Firefox 43
People
(Reporter: jfong, Assigned: jlong)
References
Details
Attachments
(1 file, 3 obsolete files)
9.65 KB,
patch
|
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
*************************
Comment 1•10 years ago
|
||
I can reproduce this
Comment 2•10 years ago
|
||
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
Comment 3•10 years ago
|
||
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?
status-firefox39:
--- → affected
status-firefox40:
--- → affected
status-firefox41:
--- → affected
Flags: qe-verify+
Flags: needinfo?(jlong)
Priority: -- → P1
Comment 4•9 years ago
|
||
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.
Assignee | ||
Comment 5•9 years ago
|
||
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)
Assignee | ||
Comment 6•9 years ago
|
||
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.
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8647760 -
Flags: review?(past)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jlong
Assignee | ||
Comment 8•9 years ago
|
||
Comment 9•9 years ago
|
||
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
Assignee | ||
Comment 10•9 years ago
|
||
(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.
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8647760 [details] [diff] [review]
1191916.patch
Also clearing r? until I write tests
Attachment #8647760 -
Flags: review?(past)
Comment 12•9 years ago
|
||
(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.
Assignee | ||
Comment 13•9 years ago
|
||
bgrins, you're right, I can still select on `disabled` since I do `element.disabled = true`. Thanks!
Assignee | ||
Comment 14•9 years ago
|
||
Added a test
Attachment #8647760 -
Attachment is obsolete: true
Attachment #8648109 -
Flags: review?(past)
Assignee | ||
Comment 15•9 years ago
|
||
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)
Assignee | ||
Comment 16•9 years ago
|
||
Comment 17•9 years ago
|
||
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)
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 18•9 years ago
|
||
Thanks bgrins! Fixed everything you mentioned.
Attachment #8648109 -
Attachment is obsolete: true
Attachment #8648109 -
Flags: review?(past)
Attachment #8648135 -
Flags: review?(bgrinstead)
Comment 19•9 years ago
|
||
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+
Assignee | ||
Comment 20•9 years ago
|
||
(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.
Assignee | ||
Comment 21•9 years ago
|
||
New try run since the review changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1741a7f41e00
Assignee | ||
Comment 22•9 years ago
|
||
whitespace change
Attachment #8648135 -
Attachment is obsolete: true
Assignee | ||
Comment 23•9 years ago
|
||
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
Comment 24•9 years ago
|
||
Keywords: checkin-needed
Comment 26•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
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)
tracking-firefox41:
--- → ?
tracking-firefox42:
--- → ?
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.
Reporter | ||
Comment 30•9 years ago
|
||
(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
Assignee | ||
Comment 32•9 years ago
|
||
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+
Assignee | ||
Comment 35•9 years ago
|
||
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+
Comment 37•9 years ago
|
||
Comment 38•9 years ago
|
||
Comment 39•9 years ago
|
||
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
Comment 40•9 years ago
|
||
(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)
Comment 41•9 years ago
|
||
Thanks, Brian!
I logged bug 1201008. Sorry, in comment 39 I meant web console, not browser console.
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•