XMPP implementation in JavaScript.

remoting/webapp/xmpp_connection.js

Index: remoting/webapp/xmpp_connection.js
diff --git a/remoting/webapp/xmpp_connection.js b/remoting/webapp/xmpp_connection.js
new file mode 100644
index 0000000000000000000000000000000000000000..d3f445b77cec149e6c0fedc221d0ac105caa2b10
--- /dev/null
+++ b/remoting/webapp/xmpp_connection.js
@@ -0,0 +1,275 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+'use strict';
+
+/** @suppress {duplicate} */
+var remoting = remoting || {};
+
+/**
+ * Object used to connect to XMPP server.

[Jamie, 2014/08/29 02:14:08]

I think it's implicit that it's an object. How about just "A connection to an
XMPP server"?

[Sergey Ulanov, 2014/08/29 23:40:29]

Done.

+ *
+ * TODO(sergeyu): Chrome provides two APIs for TCP sockets: chrome.socket and
+ * chrome.sockets.tcp . chrome.socket is deprecated but it's still used here
+ * because TLS support in chrome.sockets.tcp is currently broken, see
+ * crbug.com/403076 .
+ *
+ * @param {function(remoting.XmppConnection.State):void} onStateChangedCallback
+ *   Callback to call on state change.
+ * @param {function(Element):void} onIncomingStanzaCallback Callback to call to
+ *   handle incoming messages.
+ * @constructor
+ */
+remoting.XmppConnection =
+    function(onStateChangedCallback, onIncomingStanzaCallback) {
+  this.server_ = '';

[Jamie, 2014/08/29 02:14:08]

These should be declared as @private. I'm also a bit surprised they don't need
any type information. Has this been run through jscompile?

[Sergey Ulanov, 2014/08/29 23:40:28]

Yes. jscompiler can infer type from assignment here.

+  this.onStateChangedCallback_ = onStateChangedCallback;
+  this.onIncomingStanzaCallback_ = onIncomingStanzaCallback;
+  this.socketId_ = -1;
+  this.state_ = remoting.XmppConnection.State.NOT_CONNECTED;
+  this.readPending_ = false;
+  this.sendPending_ = false;
+  this.starttlsPending_ = false;

[Jamie, 2014/08/29 02:14:08]

s/tls/Tls/ for consistency with the method name.

[Sergey Ulanov, 2014/08/29 23:40:28]

Done.

+  /** @type {Array.<ArrayBuffer>} */
+  this.sendQueue_ = [];
+  this.jid_ = '';
+  this.error_ = remoting.Error.NONE;
+}

[Jamie, 2014/08/29 02:14:09]

Semi-colons after functions declared by assignment, please (emacs auto-indent
gets confused otherwise).

[Sergey Ulanov, 2014/08/29 23:40:28]

Done. Can emacs be integrated with clang-format for formatting? I think it can,
see https://code.google.com/p/chromium/wiki/ClangFormat . clang-format works
fine without semicolon here.

+
+/**
+ * @enum {number} XmppConnection states. Possible state transitions:
+ *    NOT_CONNECTED -> CONNECTING (connect() called).
+ *    CONNECTING -> HANDSHAKE (connected successfully).
+ *    HANDSHAKE -> CONNECTED (connected successfully).
+ *    CONNECTING -> FAILED (connection failed).
+ *    HANDSHAKE -> FAILED (authentication failed).
+ *    * -> CLOSED (dispose() called).
+ */
+remoting.XmppConnection.State = {
+  NOT_CONNECTED: 0,
+  CONNECTING: 1,
+  HANDSHAKE: 2,
+  CONNECTED: 3,
+  FAILED: 4,
+  CLOSED: 5
+};
+
+/**
+ * @param {string} server
+ * @param {string} username
+ * @param {string} authToken
+ */
+remoting.XmppConnection.prototype.connect =
+    function(server, username, authToken) {
+  if (this.state_ != remoting.XmppConnection.State.NOT_CONNECTED) {
+    console.error(
+        'remoting.XmppConnection.prototype.connect() can only be called once.');

[Jamie, 2014/08/29 02:14:08]

The log message will include the file name, so I think the class is redundant
(and if you prune everything before 'connect()' it will fit on one line).
Similarly for the other log messages.

Also, consider throwing an exception here (or using assert) otherwise the caller
has no indication that something went wrong. You could call onError, but that
doesn't feel like the right thing to do since it interferes with the previous
call to connect().

[Sergey Ulanov, 2014/08/29 23:40:29]

Replaced with assert()

+    return;
+  }
+
+  this.error_ = remoting.Error.NONE;
+  this.server_ = server;
+  /** @type {remoting.XmppLoginHandler} */
+  this.loginHandler_ =
+      new remoting.XmppLoginHandler(username, authToken,
+                                    this.sendInternal_.bind(this),
+                                    this.startTls_.bind(this),
+                                    this.onHandshakeDone_.bind(this),
+                                    this.onIncomingStanzaCallback_,
+                                    this.onError_.bind(this));
+  chrome.socket.create("tcp", {}, this.onSocketCreated_.bind(this));
+  this.setState_(remoting.XmppConnection.State.CONNECTING);
+}
+
+/** @param {string} message */
+remoting.XmppConnection.prototype.sendMessage = function(message) {
+  if (this.state_ != remoting.XmppConnection.State.CONNECTED) {
+    console.error(

[Jamie, 2014/08/29 02:14:08]

As above, the caller should have some indication that their request was ignored.
Or maybe just queue the message (ie, have sendMessage do the same thing as
sendInternal). That would solve the slightly awkward use of both sendInternal_
and doSend_, both of which scream "I'd like to call this method send(), but
that's already taken".

[Sergey Ulanov, 2014/08/29 23:40:29]

Done.

+        'remoting.XmppConnection.prototype.sendMessage() is called when not ' +
+        'connected.');
+    return;
+  }
+
+  this.sendInternal_(message);
+}
+
+/** @return {remoting.XmppConnection.State} Current state */
+remoting.XmppConnection.prototype.getState = function() {
+  return this.state_;
+}
+
+/** @return {remoting.Error} Error when in FAILED state. */
+remoting.XmppConnection.prototype.getError = function() {
+  return this.error_;
+}
+
+/** @return {string} Current JID when in CONNECTED state. */
+remoting.XmppConnection.prototype.getJid = function() {
+  return this.jid_;
+}
+
+remoting.XmppConnection.prototype.dispose = function() {

[Jamie, 2014/08/29 02:14:09]

This might be better named disconnect() for symmetry with connect(), and moved
adjacent to it. Alternatively, if you want to follow the base.Disposable
pattern, you should annotate this class with @implements {base.Disposable}.

[Sergey Ulanov, 2014/08/29 23:40:29]

Yes, the intent here is to follow the Disposable pattern. Added @implements.

+  this.closeSocket_();
+  this.setState_(remoting.XmppConnection.State.CLOSED);
+}
+
+/** @param {chrome.socket.CreateInfo} createInfo */

[Jamie, 2014/08/29 02:14:08]

@private for this and other methods ending in an underscore.

[Sergey Ulanov, 2014/08/29 23:40:28]

Done.

+remoting.XmppConnection.prototype.onSocketCreated_ = function(createInfo) {
+  // Check if connection was terminated.

[Jamie, 2014/08/29 02:14:08]

I think this can only happen if the caller calls dispose (disconnect if you
follow my naming advice). If that's the case, this might be clearer as "Check if
the connection attempt was cancelled", since "terminated" implies it might have
been closed by the remote end.

[Sergey Ulanov, 2014/08/29 23:40:28]

Replaced with "Check if connection was destroyed"

+  if (this.state_ != remoting.XmppConnection.State.CONNECTING) {
+    chrome.socket.destroy(createInfo.socketId);
+    return;
+  }
+
+  this.socketId_ = createInfo.socketId;
+
+  var hostnameAndPort = this.server_.split(':', 2);
+  var hostname = hostnameAndPort[0];
+  var port =
+      (hostnameAndPort.length == 2) ? parseInt(hostnameAndPort[1], 10) : 5222;
+  chrome.socket.connect(
+      this.socketId_, hostname, port, this.onSocketConnected_.bind(this));
+}
+
+/** @param {number} result */
+remoting.XmppConnection.prototype.onSocketConnected_ = function(result) {
+  // Check if connection was terminated.
+  if (this.state_ != remoting.XmppConnection.State.CONNECTING)
+    return;

[Jamie, 2014/08/29 02:14:09]

Braces for single-line ifs for consistency, please. Although don't you need to
destroy and unassign this.socket?

[Sergey Ulanov, 2014/08/29 23:40:29]

No. It should have been destroyed when dispose() was called.

+
+  if (result != 0) {
+    this.onError_(remoting.Error.NETWORK_FAILURE,
+                  'Failed to connect to ' + this.server_ + ': ' +  result);
+    return;
+  }
+
+  this.setState_(remoting.XmppConnection.State.HANDSHAKE);
+
+  this.tryRead_();
+  this.loginHandler_.start();
+}
+
+remoting.XmppConnection.prototype.tryRead_ = function() {
+  base.debug.assert(!this.readPending_);
+  if ((this.state_ != remoting.XmppConnection.State.HANDSHAKE &&
+       this.state_ != remoting.XmppConnection.State.CONNECTED) ||
+      this.starttlsPending_) {
+    return;
+  }

[Jamie, 2014/08/29 02:14:08]

Why an assert for readPending, but a conditional for the more complex case?
Since this is a private method, is it reasonable to assert that it should not be
called other than in HANDSHAKE or CONNECTED states, and never with
startTlsPending?

[Sergey Ulanov, 2014/08/29 23:40:29]

Done.

+
+  this.readPending_ = true;
+  chrome.socket.read(this.socketId_, this.onRead_.bind(this));
+}
+
+/** @param {chrome.socket.ReadInfo} readInfo */
+remoting.XmppConnection.prototype.onRead_ = function(readInfo) {
+  base.debug.assert(this.readPending_);
+  this.readPending_ = false;
+
+  if (readInfo.resultCode < 0) {
+    this.onError_(remoting.Error.NETWORK_FAILURE,
+                  'Failed to receive from XMPP socket: ' + readInfo.resultCode);
+    return;
+  }
+
+  this.loginHandler_.onDataReceived(readInfo.data);

[Jamie, 2014/08/29 02:14:09]

The name of this class implies that it just handles the initial login handshake,
but it appears that it also handles passing stanzas through to the calling code,
is that correct? If so, then I think the naming could be improved. Perhaps call
this class XmppConnector and the other something like XmppProtocol. Or the
XmppLoginHandler could be refactored to only handle the login part of the
protocol, and signal back to this class when the connection is authenticated and
ready to exchange client stanzas (which I believe it already does via
onHandshakeDone).

[Sergey Ulanov, 2014/08/29 23:40:29]

Refactored this code so that LoginHandler is used only during HANDSHAKE face and
then it passes the stream parser in the HandshakeDone callback to the
XmppConnection

+  this.tryRead_();
+}
+
+/** @param {string} text */
+remoting.XmppConnection.prototype.sendInternal_ = function(text) {
+  this.sendQueue_.push(base.encodeUtf8(text));
+  this.doSend_();
+}
+
+remoting.XmppConnection.prototype.doSend_ = function() {
+  if (this.sendPending_)
+    return;
+  if (this.sendQueue_.length == 0)
+    return;
+
+  var data = this.sendQueue_[0]
+  this.sendPending_ = true;
+  chrome.socket.write(this.socketId_, data, this.onWrite_.bind(this));
+}
+
+/** @param {chrome.socket.WriteInfo} writeInfo */
+remoting.XmppConnection.prototype.onWrite_ = function(writeInfo) {
+  base.debug.assert(this.sendPending_);
+  this.sendPending_ = false;
+
+  if (writeInfo.bytesWritten < 0) {
+    console.error('TCP write failed with error ' + writeInfo.bytesWritten);

[Jamie, 2014/08/29 02:14:08]

You should call onError here so that this.error is set correctly.

[Sergey Ulanov, 2014/08/29 23:40:29]

Done.

+    this.setState_(remoting.XmppConnection.State.FAILED);
+    return;
+  }
+
+  base.debug.assert(this.sendQueue_.length > 0);
+
+  var data = this.sendQueue_[0]
+  base.debug.assert(writeInfo.bytesWritten <= data.byteLength);

[Jamie, 2014/08/29 02:14:09]

What is this assert guarding against? You're already guarding against the API
returning a value that's too large because you use >= below, and if it returns
some other bogus value, you can't detect it anyway. I would be inclined to
remove the assert.

[Sergey Ulanov, 2014/08/29 23:40:29]

Its to detect bugs that cause head of sendQueue_ to be changed while write() is
pending. I'm not trying to detect bugs in the API implementation.

+  if (writeInfo.bytesWritten >= data.byteLength) {
+    this.sendQueue_.shift();
+  } else {
+    this.sendQueue_[0] = data.slice(data.byteLength - writeInfo.bytesWritten);
+  }
+
+  this.doSend_();
+}
+
+remoting.XmppConnection.prototype.startTls_ = function() {
+  base.debug.assert(!this.readPending_);
+  base.debug.assert(!this.starttlsPending_);
+
+  this.starttlsPending_ = true;
+  chrome.socket.secure(
+      this.socketId_, {}, this.onTlsStarted_.bind(this));
+}
+
+/** @param {number} resultCode */
+remoting.XmppConnection.prototype.onTlsStarted_ = function(resultCode) {
+  base.debug.assert(this.starttlsPending_);
+  this.starttlsPending_ = false;
+
+  if (resultCode < 0) {
+    this.onError_(remoting.Error.NETWORK_FAILURE,
+                  'Failed to start TLS: ' + resultCode);
+    return;
+  }
+
+  this.tryRead_();
+  this.loginHandler_.onTlsStarted();
+}
+
+/** @param {string} jid */
+remoting.XmppConnection.prototype.onHandshakeDone_ = function(jid) {
+  this.jid_ = jid;
+  this.setState_(remoting.XmppConnection.State.CONNECTED);
+}
+
+/**
+ * @param {remoting.Error} error
+ * @param {string} text
+ */
+remoting.XmppConnection.prototype.onError_ = function(error, text) {
+  console.error(text);
+  this.error_ = error;
+  this.closeSocket_();
+  this.setState_(remoting.XmppConnection.State.FAILED);
+}
+
+remoting.XmppConnection.prototype.closeSocket_ = function() {
+  if (this.socketId_ != -1) {
+    chrome.socket.destroy(this.socketId_);
+    this.socketId_ = -1;
+  }
+}
+
+/** @param {remoting.XmppConnection.State} newState */
+remoting.XmppConnection.prototype.setState_ = function(newState) {
+  if (this.state_ != newState) {
+    this.state_ = newState;
+    this.onStateChangedCallback_(this.state_);
+  }
+}