Closed
Bug 915448
Opened 9 years ago
Closed 7 years ago
Error when last selected tool is not available for current toolbox target
Categories
(DevTools :: Framework, defect)
DevTools
Framework
Tracking
(firefox41 fixed)
RESOLVED
FIXED
Firefox 41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: bbenvie, Assigned: past)
Details
Attachments
(1 file)
4.07 KB,
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
If you have a different set of tools in remote vs. local toolboxes, and the last selected tool isn't available when opening a new toolbox of the opposite target type (remote/local), an error is thrown. STR: * create a new panel that is only remote (returns `target.isRemote` from isTargetSupported) * open a remote panel and select that panel * open a new local toolbox Because the last selected panel type is not compatible with the current toolbox's target, an error is thrown: TypeError: tab is null @ resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:544
Reporter | ||
Comment 1•9 years ago
|
||
For step two, I meant "open a remote toolbox".
Assignee | ||
Comment 2•7 years ago
|
||
Easier STR: 1. Open the netmonitor on a page and then close the toolbox 2. Open WebIDE and connect to Chrome with Valence 3. Try to debug a page on Chrome. ************************* A coding exception was thrown and uncaught in a Task. Full message: TypeError: tab is null Full stack: Toolbox.prototype.selectTool@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:1131:5 Toolbox.prototype.open/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:373:13 TaskImpl_run@resource://gre/modules/Task.jsm:314:40 Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:867:23 this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:7 this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:37 Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:5 this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:709:7 this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:671:7 Toolbox.prototype.open/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:330:36
Assignee: nobody → past
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•7 years ago
|
||
Often a notification appears in WebIDE that the RDP connection has timed out (even though you can later select one of the available panels).
Assignee | ||
Comment 4•7 years ago
|
||
Simple fix for a bug that I always need to avoid hitting when I'm giving a talk.
Assignee | ||
Updated•7 years ago
|
Attachment #8604015 -
Flags: review?(bgrinstead)
Comment 5•7 years ago
|
||
Comment on attachment 8604015 [details] [diff] [review] The toolbox should expect the previously selected tool to be unavailable Review of attachment 8604015 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, and I think we can also remove some code from the constructor. r=me with that change ::: browser/devtools/framework/toolbox.js @@ +371,5 @@ > this._pingTelemetry(); > > + // This check needs to happen after the target is remoted, otherwise we > + // could have done it in the toolbox constructor (bug 1072764). > + let toolDef = gDevTools.getToolDefinition(this._defaultToolId); There is very similar code in the constructor (that's checking only if the toolDef exists and setting it to webconsole if not): https://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#136-138. Looks like that condition can be removed and handled here with `if (!toolDef || !toolDef.isTargetSupported(this._target)) {}`
Attachment #8604015 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Comment 6•7 years ago
|
||
That's what I tried and unfortunately it doesn't work without bug 1072764. This is why I put in the comment so we can remember to move it to the constructor when it's feasible to do so.
Assignee | ||
Comment 7•7 years ago
|
||
We discussed this on IRC and I realized what Brian suggested is different from what I initially thought and makes a lot of sense.
https://hg.mozilla.org/mozilla-central/rev/a6b952b85bc6 https://hg.mozilla.org/mozilla-central/rev/d5dd7ebbeece
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Updated•4 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•