Merge pull request #713 from kanaka/properdisconnections

Better error handling

app/styles/base.css

@@ -172,6 +172,43 @@ input[type=button]:active, select:active {
   pointer-events: auto;
 }
 
+/* ----------------------------------------
+ * Fallback error
+ * ----------------------------------------
+ */
+
+#noVNC_fallback_error {
+  position: fixed;
+  z-index: 3;
+  left: 50%;
+  transform: translate(-50%, -50px);
+  transition: 0.5s ease-in-out;
+
+  visibility: hidden;
+  opacity: 0;
+
+  top: 60px;
+  padding: 15px;
+  width: auto;
+
+  text-align: center;
+  font-weight: bold;
+  word-wrap: break-word;
+  color: #fff;
+
+  border-radius: 10px;
+  box-shadow: 6px 6px 0px rgba(0, 0, 0, 0.5);
+  background: rgba(200,55,55,0.8);
+}
+#noVNC_fallback_error.noVNC_open {
+  transform: translate(-50%, 0);
+  visibility: visible;
+  opacity: 1;
+}
+#noVNC_fallback_errormsg {
+  font-weight: normal;
+}
+
 /* ----------------------------------------
  * Control Bar
  * ----------------------------------------

app/ui.js

@@ -25,6 +25,21 @@ var UI;
 (function () {
     "use strict";
 
+    // Fallback for all uncought errors
+    window.addEventListener('error', function(msg, url, line) {
+        try {
+            document.getElementById('noVNC_fallback_error')
+                .classList.add("noVNC_open");
+            document.getElementById('noVNC_fallback_errormsg').innerHTML =
+                url + ' (' + line + ') <br><br>' + msg;
+        } catch (exc) {
+            document.write("noVNC encountered an error.");
+        }
+        // Don't return true since this would prevent the error
+        // from being printed to the browser console.
+        return false;
+    });
+
     /* [begin skip-as-module] */
     // Load supporting scripts
     WebUtil.load_scripts(

core/rfb.js

@@ -213,7 +213,7 @@
             this._rfb_init_state = 'ProtocolVersion';
             Util.Debug("Starting VNC handshake");
         } else {
-            this._fail("Got unexpected WebSocket connection");
+            this._fail("Unexpected server connection");
         }
     }.bind(this));
     this._sock.on('close', function (e) {
@@ -231,13 +231,20 @@
                 this._updateConnectionState('disconnected');
                 break;
             case 'connecting':
-                this._fail('Failed to connect to server' + msg);
+                this._fail('Failed to connect to server', msg);
+                break;
+            case 'connected':
+                // Handle disconnects that were initiated server-side
+                this._updateConnectionState('disconnecting');
+                this._updateConnectionState('disconnected');
                 break;
             case 'disconnected':
-                Util.Error("Received onclose while disconnected" + msg);
+                this._fail("Unexpected server disconnect",
+                           "Already disconnected: " + msg);
                 break;
             default:
-                this._fail("Server disconnected" + msg);
+                this._fail("Unexpected server disconnect",
+                           "Not in any state yet: " + msg);
                 break;
         }
         this._sock.off('close');
@@ -343,7 +350,7 @@
         requestDesktopSize: function (width, height) {
             if (this._rfb_connection_state !== 'connected' ||
                 this._view_only) {
-                return;
+                return false;
             }
 
             if (this._supportsSetDesktopSize) {
@@ -361,6 +368,7 @@
 
         _connect: function () {
             Util.Debug(">> RFB.connect");
+            this._init_vars();
 
             var uri;
             if (typeof UsingSocketIO !== 'undefined') {
@@ -372,11 +380,28 @@
             uri += '://' + this._rfb_host + ':' + this._rfb_port + '/' + this._rfb_path;
             Util.Info("connecting to " + uri);
 
+            try {
+                // WebSocket.onopen transitions to the RFB init states
                 this._sock.open(uri, this._wsProtocols);
+            } catch (e) {
+                if (e.name === 'SyntaxError') {
+                    this._fail("Invalid host or port value given", e);
+                } else {
+                    this._fail("Error while connecting", e);
+                }
+            }
 
             Util.Debug("<< RFB.connect");
         },
 
+        _disconnect: function () {
+            Util.Debug(">> RFB.disconnect");
+            this._cleanup();
+            this._sock.close();
+            this._print_stats();
+            Util.Debug("<< RFB.disconnect");
+        },
+
         _init_vars: function () {
             // reset state
             this._FBU.rects        = 0;
@@ -450,19 +475,7 @@
                 return;
             }
 
-            this._rfb_connection_state = state;
+            // Ensure proper transitions before doing anything
-
-            var smsg = "New state '" + state + "', was '" + oldstate + "'.";
-            Util.Debug(smsg);
-
-            if (this._disconnTimer && state !== 'disconnecting') {
-                Util.Debug("Clearing disconnect timer");
-                clearTimeout(this._disconnTimer);
-                this._disconnTimer = null;
-                this._sock.off('close');  // make sure we don't get a double event
-            }
-
-            this._onUpdateState(this, state, oldstate);
             switch (state) {
                 case 'connected':
                     if (oldstate !== 'connecting') {
@@ -478,7 +491,50 @@
                                    "previous connection state: " + oldstate);
                         return;
                     }
+                    break;
 
+                case 'connecting':
+                    if (oldstate !== '') {
+                        Util.Error("Bad transition to connecting state, " +
+                                   "previous connection state: " + oldstate);
+                        return;
+                    }
+                    break;
+
+                case 'disconnecting':
+                    if (oldstate !== 'connected' && oldstate !== 'connecting') {
+                        Util.Error("Bad transition to disconnecting state, " +
+                                   "previous connection state: " + oldstate);
+                        return;
+                    }
+                    break;
+
+                default:
+                    Util.Error("Unknown connection state: " + state);
+                    return;
+            }
+
+            // State change actions
+
+            this._rfb_connection_state = state;
+            this._onUpdateState(this, state, oldstate);
+
+            var smsg = "New state '" + state + "', was '" + oldstate + "'.";
+            Util.Debug(smsg);
+
+            if (this._disconnTimer && state !== 'disconnecting') {
+                Util.Debug("Clearing disconnect timer");
+                clearTimeout(this._disconnTimer);
+                this._disconnTimer = null;
+
+                // make sure we don't get a double event
+                this._sock.off('close');
+            }
+
+            switch (state) {
+                case 'disconnected':
+                    // Call onDisconnected callback after onUpdateState since
+                    // we don't know if the UI only displays the latest message
                     if (this._rfb_disconnect_reason !== "") {
                         this._onDisconnected(this, this._rfb_disconnect_reason);
                     } else {
@@ -488,47 +544,50 @@
                     break;
 
                 case 'connecting':
-                    this._init_vars();
-
-                    // WebSocket.onopen transitions to the RFB init states
                     this._connect();
                     break;
 
                 case 'disconnecting':
-                    this._cleanup();
+                    this._disconnect();
-                    this._sock.close(); // transitions to 'disconnected'
 
                     this._disconnTimer = setTimeout(function () {
                         this._rfb_disconnect_reason = "Disconnect timeout";
                         this._updateConnectionState('disconnected');
                     }.bind(this), this._disconnectTimeout * 1000);
-
-                    this._print_stats();
                     break;
-
-                default:
-                    Util.Error("Unknown connection state: " + state);
-                    return;
             }
         },
 
-        _fail: function (msg) {
+        /* Print errors and disconnect
+         *
+         * The optional parameter 'details' is used for information that
+         * should be logged but not sent to the user interface.
+         */
+        _fail: function (msg, details) {
+            var fullmsg = msg;
+            if (typeof details !== 'undefined') {
+                fullmsg = msg + "(" + details + ")";
+            }
             switch (this._rfb_connection_state) {
                 case 'disconnecting':
-                    Util.Error("Error while disconnecting: " + msg);
+                    Util.Error("Failed when disconnecting: " + fullmsg);
                     break;
                 case 'connected':
-                    Util.Error("Error while connected: " + msg);
+                    Util.Error("Failed while connected: " + fullmsg);
                     break;
                 case 'connecting':
-                    Util.Error("Error while connecting: " + msg);
+                    Util.Error("Failed when connecting: " + fullmsg);
                     break;
                 default:
-                    Util.Error("RFB error: " + msg);
+                    Util.Error("RFB failure: " + fullmsg);
                     break;
             }
-            this._rfb_disconnect_reason = msg;
+            this._rfb_disconnect_reason = msg; //This is sent to the UI
+
+            // Transition to disconnected without waiting for socket to close
             this._updateConnectionState('disconnecting');
+            this._updateConnectionState('disconnected');
+
             return false;
         },
 
@@ -669,7 +728,8 @@
 
         _negotiate_protocol_version: function () {
             if (this._sock.rQlen() < 12) {
-                return this._fail("Incomplete protocol version");
+                return this._fail("Error while negotiating with server",
+                                  "Incomplete protocol version");
             }
 
             var sversion = this._sock.rQshiftStr(12).substr(4, 7);
@@ -694,7 +754,8 @@
                     this._rfb_version = 3.8;
                     break;
                 default:
-                    return this._fail("Invalid server version " + sversion);
+                    return this._fail("Unsupported server",
+                                      "Invalid server version: " + sversion);
             }
 
             if (is_repeater) {
@@ -727,7 +788,8 @@
                 if (num_types === 0) {
                     var strlen = this._sock.rQshift32();
                     var reason = this._sock.rQshiftStr(strlen);
-                    return this._fail("Security failure: " + reason);
+                    return this._fail("Error while negotiating with server",
+                                      "Security failure: " + reason);
                 }
 
                 this._rfb_auth_scheme = 0;
@@ -749,7 +811,8 @@
                 }
 
                 if (this._rfb_auth_scheme === 0) {
-                    return this._fail("Unsupported security types: " + types);
+                    return this._fail("Unsupported server",
+                                      "Unsupported security types: " + types);
                 }
 
                 this._sock.send([this._rfb_auth_scheme]);
@@ -819,12 +882,16 @@
             if (serverSupportedTunnelTypes[0]) {
                 if (serverSupportedTunnelTypes[0].vendor != clientSupportedTunnelTypes[0].vendor ||
                     serverSupportedTunnelTypes[0].signature != clientSupportedTunnelTypes[0].signature) {
-                    return this._fail("Client's tunnel type had the incorrect vendor or signature");
+                    return this._fail("Unsupported server",
+                                      "Client's tunnel type had the incorrect " +
+                                      "vendor or signature");
                 }
                 this._sock.send([0, 0, 0, 0]);  // use NOTUNNEL
                 return false; // wait until we receive the sub auth count to continue
             } else {
-                return this._fail("Server wanted tunnels, but doesn't support the notunnel type");
+                return this._fail("Unsupported server",
+                                  "Server wanted tunnels, but doesn't support " +
+                                  "the notunnel type");
             }
         },
 
@@ -877,12 +944,15 @@
                             this._rfb_auth_scheme = 2;
                             return this._init_msg();
                         default:
-                            return this._fail("Unsupported tiny auth scheme: " + authType);
+                            return this._fail("Unsupported server",
+                                              "Unsupported tiny auth scheme: " +
+                                              authType);
                     }
                 }
             }
 
-            return this._fail("No supported sub-auth types!");
+            return this._fail("Unsupported server",
+                              "No supported sub-auth types!");
         },
 
         _negotiate_authentication: function () {
@@ -891,7 +961,7 @@
                     if (this._sock.rQwait("auth reason", 4)) { return false; }
                     var strlen = this._sock.rQshift32();
                     var reason = this._sock.rQshiftStr(strlen);
-                    return this._fail("Auth failure: " + reason);
+                    return this._fail("Authentication failure", reason);
 
                 case 1:  // no auth
                     if (this._rfb_version >= 3.8) {
@@ -911,7 +981,9 @@
                     return this._negotiate_tight_auth();
 
                 default:
-                    return this._fail("Unsupported auth scheme: " + this._rfb_auth_scheme);
+                    return this._fail("Unsupported server",
+                                      "Unsupported auth scheme: " +
+                                      this._rfb_auth_scheme);
             }
         },
 
@@ -927,15 +999,16 @@
                         var length = this._sock.rQshift32();
                         if (this._sock.rQwait("SecurityResult reason", length, 8)) { return false; }
                         var reason = this._sock.rQshiftStr(length);
-                        return this._fail(reason);
+                        return this._fail("Authentication failure", reason);
                     } else {
                         return this._fail("Authentication failure");
                     }
                     return false;
                 case 2:
-                    return this._fail("Too many auth attempts");
+                    return this._fail("Too many authentication attempts");
                 default:
-                    return this._fail("Unknown SecurityResult");
+                    return this._fail("Unsupported server",
+                                      "Unknown SecurityResult");
             }
         },
 
@@ -1083,7 +1156,7 @@
                     return this._negotiate_server_init();
 
                 default:
-                    return this._fail("Unknown init state: " +
+                    return this._fail("Internal error", "Unknown init state: " +
                                       this._rfb_init_state);
             }
         },
@@ -1148,7 +1221,8 @@
              */
 
             if (!(flags & (1<<31))) {
-                return this._fail("Unexpected fence response");
+                return this._fail("Internal error",
+                                  "Unexpected fence response");
             }
 
             // Filter out unsupported flags
@@ -1180,7 +1254,8 @@
                     this._onXvpInit(this._rfb_xvp_ver);
                     break;
                 default:
-                    this._fail("Disconnected: illegal server XVP message " + xvp_msg);
+                    this._fail("Unexpected server message",
+                               "Illegal server XVP message " + xvp_msg);
                     break;
             }
 
@@ -1239,7 +1314,7 @@
                     return this._handle_xvp_msg();
 
                 default:
-                    this._fail("Disconnected: illegal server message type " + msg_type);
+                    this._fail("Unexpected server message", "Type:" + msg_type);
                     Util.Debug("sock.rQslice(0, 30): " + this._sock.rQslice(0, 30));
                     return true;
             }
@@ -1300,7 +1375,8 @@
                          'encodingName': this._encNames[this._FBU.encoding]});
 
                     if (!this._encNames[this._FBU.encoding]) {
-                        this._fail("Disconnected: unsupported encoding " +
+                        this._fail("Unexpected server message",
+                                   "Unsupported encoding " +
                                    this._FBU.encoding);
                         return false;
                     }
@@ -1807,7 +1883,8 @@
                 if (this._sock.rQwait("HEXTILE subencoding", this._FBU.bytes)) { return false; }
                 var subencoding = rQ[rQi];  // Peek
                 if (subencoding > 30) {  // Raw
-                    this._fail("Disconnected: illegal hextile subencoding " + subencoding);
+                    this._fail("Unexpected server message",
+                               "Illegal hextile subencoding: " + subencoding);
                     return false;
                 }
 
@@ -1939,7 +2016,9 @@
 
         display_tight: function (isTightPNG) {
             if (this._fb_depth === 1) {
-                this._fail("Tight protocol handler only implements true color mode");
+                this._fail("Internal error",
+                           "Tight protocol handler only implements " +
+                           "true color mode");
             }
 
             this._FBU.bytes = 1;  // compression-control byte
@@ -2169,10 +2248,13 @@
             else if (ctl === 0x0A)  cmode = "png";
             else if (ctl & 0x04)    cmode = "filter";
             else if (ctl < 0x04)    cmode = "copy";
-            else return this._fail("Illegal tight compression received, ctl: " + ctl);
+            else return this._fail("Unexpected server message",
+                                   "Illegal tight compression received, " +
+                                   "ctl: " + ctl);
 
             if (isTightPNG && (cmode === "filter" || cmode === "copy")) {
-                return this._fail("filter/copy received in tightPNG mode");
+                return this._fail("Unexpected server message",
+                                  "filter/copy received in tightPNG mode");
             }
 
             switch (cmode) {
@@ -2233,7 +2315,9 @@
                     } else {
                         // Filter 0, Copy could be valid here, but servers don't send it as an explicit filter
                         // Filter 2, Gradient is valid but not use if jpeg is enabled
-                        this._fail("Unsupported tight subencoding received, filter: " + filterId);
+                        this._fail("Unexpected server message",
+                                   "Unsupported tight subencoding received, " +
+                                   "filter: " + filterId);
                     }
                     break;
                 case "copy":

tests/test.rfb.js

@@ -330,7 +330,7 @@ describe('Remote Frame Buffer Protocol Client', function() {
             it('should clear the disconnect timer if the state is not "disconnecting"', function () {
                 var spy = sinon.spy();
                 client._disconnTimer = setTimeout(spy, 50);
-                client._updateConnectionState('connected');
+                client._updateConnectionState('connecting');
                 this.clock.tick(51);
                 expect(spy).to.not.have.been.called;
                 expect(client._disconnTimer).to.be.null;
@@ -338,27 +338,37 @@ describe('Remote Frame Buffer Protocol Client', function() {
 
             it('should call the updateState callback', function () {
                 client.set_onUpdateState(sinon.spy());
-                client._updateConnectionState('a specific state');
+                client._updateConnectionState('connecting');
                 var spy = client.get_onUpdateState();
                 expect(spy).to.have.been.calledOnce;
-                expect(spy.args[0][1]).to.equal('a specific state');
+                expect(spy.args[0][1]).to.equal('connecting');
             });
 
             it('should set the rfb_connection_state', function () {
-                client._updateConnectionState('a specific state');
+                client._rfb_connection_state = 'disconnecting';
-                expect(client._rfb_connection_state).to.equal('a specific state');
+                client._updateConnectionState('disconnected');
+                expect(client._rfb_connection_state).to.equal('disconnected');
             });
 
             it('should not change the state when we are disconnected', function () {
                 client._rfb_connection_state = 'disconnected';
-                client._updateConnectionState('a specific state');
+                client._updateConnectionState('connecting');
-                expect(client._rfb_connection_state).to.not.equal('a specific state');
+                expect(client._rfb_connection_state).to.not.equal('connecting');
             });
 
             it('should ignore state changes to the same state', function () {
                 client.set_onUpdateState(sinon.spy());
-                client._rfb_connection_state = 'a specific state';
+                client._rfb_connection_state = 'connecting';
-                client._updateConnectionState('a specific state');
+                client._updateConnectionState('connecting');
+                var spy = client.get_onUpdateState();
+                expect(spy).to.not.have.been.called;
+            });
+
+            it('should ignore illegal state changes', function () {
+                client.set_onUpdateState(sinon.spy());
+                client._rfb_connection_state = 'connected';
+                client._updateConnectionState('disconnected');
+                expect(client._rfb_connection_state).to.not.equal('disconnected');
                 var spy = client.get_onUpdateState();
                 expect(spy).to.not.have.been.called;
             });
@@ -391,11 +401,19 @@ describe('Remote Frame Buffer Protocol Client', function() {
             });
 
             it('should set disconnect_reason', function () {
+                client._rfb_connection_state = 'connected';
                 client._fail('a reason');
                 expect(client._rfb_disconnect_reason).to.equal('a reason');
             });
 
+            it('should not include details in disconnect_reason', function () {
+                client._rfb_connection_state = 'connected';
+                client._fail('a reason', 'details');
+                expect(client._rfb_disconnect_reason).to.equal('a reason');
+            });
+
             it('should result in disconnect callback with message when reason given', function () {
+                client._rfb_connection_state = 'connected';
                 client.set_onDisconnected(sinon.spy());
                 client._fail('a reason');
                 var spy = client.get_onDisconnected();
@@ -542,7 +560,7 @@ describe('Remote Frame Buffer Protocol Client', function() {
             it('should call the updateState callback before the disconnect callback', function () {
                 client.set_onDisconnected(sinon.spy());
                 client.set_onUpdateState(sinon.spy());
-                client._rfb_connection_state = 'other state';
+                client._rfb_connection_state = 'disconnecting';
                 client._updateConnectionState('disconnected');
                 var updateStateSpy = client.get_onUpdateState();
                 var disconnectSpy = client.get_onDisconnected();
@@ -717,7 +735,8 @@ describe('Remote Frame Buffer Protocol Client', function() {
                 client._sock._websocket._receive_data(failure_data);
 
                 expect(client._fail).to.have.been.calledOnce;
-                expect(client._fail).to.have.been.calledWith('Security failure: whoops');
+                expect(client._fail).to.have.been.calledWith(
+                    'Error while negotiating with server','Security failure: whoops');
             });
 
             it('should transition to the Authentication state and continue on successful negotiation', function () {
@@ -756,7 +775,8 @@ describe('Remote Frame Buffer Protocol Client', function() {
 
                 sinon.spy(client, '_fail');
                 client._sock._websocket._receive_data(new Uint8Array(data));
-                expect(client._fail).to.have.been.calledWith('Auth failure: Whoopsies');
+                expect(client._fail).to.have.been.calledWith(
+                    'Authentication failure', 'Whoopsies');
             });
 
             it('should transition straight to SecurityResult on "no auth" (1) for versions >= 3.8', function () {
@@ -988,7 +1008,8 @@ describe('Remote Frame Buffer Protocol Client', function() {
                 sinon.spy(client, '_fail');
                 var failure_data = [0, 0, 0, 1, 0, 0, 0, 6, 119, 104, 111, 111, 112, 115];
                 client._sock._websocket._receive_data(new Uint8Array(failure_data));
-                expect(client._fail).to.have.been.calledWith('whoops');
+                expect(client._fail).to.have.been.calledWith(
+                    'Authentication failure', 'whoops');
             });
 
             it('should fail on an error code of 1 with a standard message for version < 3.8', function () {
@@ -2112,10 +2133,18 @@ describe('Remote Frame Buffer Protocol Client', function() {
                 expect(client._rfb_connection_state).to.equal('disconnected');
             });
 
-            it('should transition to failed if we get a close event from any non-"disconnection" state', function () {
+            it('should fail if we get a close event while connecting', function () {
                 sinon.spy(client, "_fail");
                 client.connect('host', 8675);
-                client._rfb_connection_state = 'connected';
+                client._rfb_connection_state = 'connecting';
+                client._sock._websocket.close();
+                expect(client._fail).to.have.been.calledOnce;
+            });
+
+            it('should fail if we get a close event while disconnected', function () {
+                sinon.spy(client, "_fail");
+                client.connect('host', 8675);
+                client._rfb_connection_state = 'disconnected';
                 client._sock._websocket.close();
                 expect(client._fail).to.have.been.calledOnce;
             });

vnc.html

@@ -65,6 +65,12 @@
 </head>
 
 <body>
+
+    <div id="noVNC_fallback_error">
+        <div>noVNC encountered an error:</div>
+        <div id="noVNC_fallback_errormsg"></div>
+    </div>
+
     <!-- noVNC Control Bar -->
     <div id="noVNC_control_bar_anchor" class="noVNC_vcenter">