811282 - Remote Console fails to close completely in the event of a connection error

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Rob Campbell [:rc] (:robcee)]

STR:

1. Open a remote web console with no available connection target
2. Close the Web Console.

Expected:

Remote Web Console closes completely.

Actual:

A sliver of toolbar remains in the originating tab, unable to be removed.

Comment 1

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

We should probably do what the debugger does in this case, which is to not display the tool UI until the connection is established. Although with the imminent landing of the devtools window work, this may only be necessary for a brief period of time.

Comment 2

[Rob Campbell [:rc] (:robcee)]

well, the brief period of time is a 6 week window. If that's easy to implement, I'd suggest doing it.

Comment 3

[Mihai Sucan [:msucan]]

Attachment: proposed patch

This makes the Web Console display a "connection timeout" message if nothing happens during the configured timeout delay. I also added code that makes sure the web console closes safely even if the connection is dropped.

I tested with B2G Desktop: things work well - I can drop the connection any time I want, and the web console still closes properly. If you try to use the console input after a timeout you get errors - as expected, anything that tries to use the console client throws.

I expect the devtools-window work will make things better in this regard.

Looking forward to your review. Thanks!

Comment 4

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 682523 [details] [diff] [review]
proposed patch

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

Nice changes. Only real concern is the nullification of that argument.

::: browser/devtools/webconsole/webconsole.js
@@ +417,5 @@
> +  _resetConnectionTimeout: function WCF__resetConnectionTimeout()
> +  {
> +    let timer = this._connectTimer;
> +    if (timer) {
> +      let timeout = timer.delay;

you'd rather reuse this here than check the pref? I'm ok with that.

@@ +4384,5 @@
> +        timer = null;
> +      }
> +      if (aOnDisconnect) {
> +        aOnDisconnect();
> +        aOnDisconnect = null;

yuo're nulling out an argument here. I don't think that's necessary (or a thing we want to start).

Comment 5

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #4)
> Comment on attachment 682523 [details] [diff] [review]
> proposed patch
> 
> Review of attachment 682523 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice changes. Only real concern is the nullification of that argument.
> 
> ::: browser/devtools/webconsole/webconsole.js
> @@ +417,5 @@
> > +  _resetConnectionTimeout: function WCF__resetConnectionTimeout()
> > +  {
> > +    let timer = this._connectTimer;
> > +    if (timer) {
> > +      let timeout = timer.delay;
> 
> you'd rather reuse this here than check the pref? I'm ok with that.

Yep.

> @@ +4384,5 @@
> > +        timer = null;
> > +      }
> > +      if (aOnDisconnect) {
> > +        aOnDisconnect();
> > +        aOnDisconnect = null;
> 
> yuo're nulling out an argument here. I don't think that's necessary (or a
> thing we want to start).

There are 3 ways onDisconnect() can execute. Nulling-out the argument is there to prevent further execution of the callback. Not elegant, indeed. More elegant would be to have a separate function for each case, but then again this is a temporary solution - the proper fix would be for the debugger client to call the onClose callback we provide, even when things fail - it should give the callback an error object of some sorts.

Thank you for the review!

Comment 6

[Mihai Sucan [:msucan]]

Landed:

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

Comment 7

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/mozilla-central/rev/a701300a17ab

Comment 8

[Mihai Sucan [:msucan]]

Attachment: patch for aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 768096
User impact if declined: poor user experience when the server fails to respond, when a remote connection starts. This bug also fixes the web console closing when the server connection is broken.
Testing completed (on m-c, etc.): landed in m-c for some time already.
Risk to taking this patch (and alternatives if risky): minimal.
String or UUID changes made by this patch: none.

Thank you!

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 687141 [details] [diff] [review]
patch for aurora

Approving in support of our new remote debugger feature.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eceb6e65d038

Comment 11

[Mihai Sucan [:msucan]]

Alex: thank you for approval.

Planned to land this into aurora today. I see Ryan was quicker - thank you!