Error when last selected tool is not available for current toolbox target

RESOLVED FIXED in Firefox 41

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: bbenvie, Assigned: past)

Tracking

Trunk
Firefox 41

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment)

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
For step two, I meant "open a remote toolbox".
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
Often a notification appears in WebIDE that the RDP connection has timed out (even though you can later select one of the available panels).
Simple fix for a bug that I always need to avoid hitting when I'm giving a talk.
Attachment #8604015 - Flags: review?(bgrinstead)
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+
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.
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: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.