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

Status: RESOLVED FIXED

Description

[Brandon Benvie [:benvie]]

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

Comment 1

[Brandon Benvie [:benvie]]

For step two, I meant "open a remote toolbox".

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Often a notification appears in WebIDE that the RDP connection has timed out (even though you can later select one of the available panels).

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: The toolbox should expect the previously selected tool to be unavailable

Simple fix for a bug that I always need to avoid hitting when I'm giving a talk.

Comment 5

[(Mostly unavailable in July) Brian Grinstead [:bgrins]]

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)) {}`

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

We discussed this on IRC and I realized what Brian suggested is different from what I initially thought and makes a lot of sense.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a6b952b85bc6

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d5dd7ebbeece

Comment 10

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/a6b952b85bc6
https://hg.mozilla.org/mozilla-central/rev/d5dd7ebbeece