Ensure proper connection state transitions

Makes the state machine more rubust and clear.

core/rfb.js

@@ -239,7 +239,7 @@
                 this._updateConnectionState('disconnected');
                 break;
             case 'disconnected':
-                Util.Error("Received onclose while disconnected" + msg);
+                this._fail("Received onclose while disconnected" + msg);
                 break;
             default:
                 this._fail("Unexpected server disconnect" + msg);
@@ -473,19 +473,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') {
@@ -501,7 +489,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 {
@@ -522,10 +553,6 @@
                         this._updateConnectionState('disconnected');
                     }.bind(this), this._disconnectTimeout * 1000);
                     break;
-
-                default:
-                    Util.Error("Unknown connection state: " + state);
-                    return;
             }
         },
 

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,13 @@ 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 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 +554,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();
@@ -2120,6 +2132,14 @@ describe('Remote Frame Buffer Protocol Client', function() {
                 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;
+            });
+
             it('should unregister close event handler', function () {
                 sinon.spy(client._sock, 'off');
                 client.connect('host', 8675);