Add associated interfaces & bindings.

mojo/public/js/router.js

 // 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.
 
 define("mojo/public/js/router", [
   "mojo/public/js/connector",
   "mojo/public/js/core",
   "mojo/public/js/interface_types",
   "mojo/public/js/lib/interface_endpoint_handle",
   "mojo/public/js/lib/pipe_control_message_handler",
@@ skipping 56 matching lines @@
     this.connector_.setIncomingReceiver({
         accept: this.accept.bind(this),
     });
     this.connector_.setErrorHandler({
         onError: this.onPipeConnectionError.bind(this),
     });
 
     this.setInterfaceIdNamespaceBit_ = setInterfaceIdNamespaceBit;
     this.controlMessageHandler_ = new PipeControlMessageHandler(this);
     this.controlMessageProxy_ = new PipeControlMessageProxy(this.connector_);
-    this.nextInterfaceIdValue = 1;
+    this.nextInterfaceIdValue_ = 1;
     this.encounteredError_ = false;
     this.endpoints_ = new Map();
   }
 
+  Router.prototype.associateInterface = function(handleToSend) {
+    if (!handleToSend.pendingAssociation()) {
+      return types.kInvalidInterfaceId;
+    }
+
+    var id = 0;
+    do {
+      if (this.nextInterfaceIdValue_ >= types.kInterfaceIdNamespaceMask) {
+        this.nextInterfaceIdValue_ = 1;
+      }
+      id = this.nextInterfaceIdValue_++;
+      if (this.setInterfaceIdNamespaceBit_) {
+        id += types.kInterfaceIdNamespaceMask;
+      }
+    } while (this.endpoints_.has(id));
+
+    var endpoint = new InterfaceEndpoint(this, id);
+    this.endpoints_.set(id, endpoint);
+    if (this.encounteredError_) {
+      this.updateEndpointStateMayRemove(endpoint,
+          EndpointStateUpdateType.PEER_ENDPOINT_CLOSED);
+    }
+    endpoint.handleCreated = true;
+
+    if (!handleToSend.notifyAssociation(id, this)) {
+      // The peer handle of |handleToSend|, which is supposed to join this
+      // associated group, has been closed.
+      if (endpoint) {

[yzshen1, 2017/04/19 21:11:34]

No need to check "if (endpoint)".

[wangjimmy, 2017/04/20 15:36:44]

Done.

+        this.updateEndpointStateMayRemove(endpoint,
+            EndpointStateUpdateType.ENDPOINT_CLOSED);
+      }
+      pipeControlMessageproxy.notifyPeerEndpointClosed(id,
+          handleToSend.disconnectReason());
+    }
+
+    return id;
+  };
+
   Router.prototype.attachEndpointClient = function(
       interfaceEndpointHandle, interfaceEndpointClient) {
     check(types.isValidInterfaceId(interfaceEndpointHandle.id()));
     check(interfaceEndpointClient);
 
     var endpoint = this.endpoints_.get(interfaceEndpointHandle.id());
     check(endpoint);
     check(!endpoint.client);
     check(!endpoint.closed);
     endpoint.client = interfaceEndpointClient;
@@ skipping 50 matching lines @@
     return new InterfaceEndpointHandle(interfaceId, this);
   };
 
   Router.prototype.accept = function(message) {
     var messageValidator = new Validator(message);
     var err = messageValidator.validateMessageHeader();
 
     var ok = false;
     if (err !== validator.validationError.NONE) {
       validator.reportValidationError(err);
-    } else if (controlMessageHandler.isPipeControlMessage(message)) {
-      ok = this.controlMessageHandler_.accept(message);
     } else {
-      var interfaceId = message.getInterfaceId();
-      var endpoint = this.endpoints_.get(interfaceId);
-      if (!endpoint || endpoint.closed) {
-        return true;
+      if (!message.deserializeAssociatedEndpointHandles(this)) {
+        return false;

[yzshen1, 2017/04/19 21:11:34]

Do we need to also run the logic of line 213 - 215 before returning?

[wangjimmy, 2017/04/20 15:36:44]

In the C++ code

bool Connector::ReadSingleMessage(MojoResult* read_result) {
  receiver_result =
        incoming_receiver_ && incoming_receiver_->Accept(&message);
  if (enforce_errors_from_incoming_receiver_ && !receiver_result) {
    HandleError(true, false);
    return false;
  }
  return true;
}

bool MultiplexRouter::Accept(Message* message) {
  if (!message->DeserializeAssociatedEndpointHandles(this))
    return false;

  bool processed = tasks_.empty() && ProcessIncomingMessage(message, 
    client_call_behavior, connector_.task_runner());

  // Always return true. If we see errors during message processing, we will
  // explicitly call Connector::RaiseError() to disconnect the message pipe.
  return true;
}

// Which calls MultiplexRouter::ProcessIncomingMessage(message,..)

// These are the only cases where RaiseErrorInNonTestingMode() 
// (Connector::RaiseError()) calls gets called.

bool MultiplexRouter::ProcessIncomingMessage(message,..) {
  if (PipeControlMessageHandler::IsPipeControlMessage(message)) {
    result = control_message_handler_.Accept(message);
  if (!result)
      RaiseErrorInNonTestingMode();
  return true;
  }
  
  result = client->HandleIncomingMessage(message);
  if (!result)
    RaiseErrorInNonTestingMode();
  return true;
}

Since RaiseErrorInNonTestingMode() isn't called when you do
      if (!message.deserializeAssociatedEndpointHandles(this)) {
        return false;
      }

      if (!endpoint.client) {
        // We need to wait until a client is attached in order to dispatch
        // further messages.
        return false;
      }

Correct me if I'm wrong. We shouldn't disconnect the message pipe by calling
close() because RaiseErrorInNonTestingMode() is not called in the above 2 cases.
But since HandleError() in bool Connector::ReadSingleMessage is called because
of the returned false value we need to call the connection error handler.

I will change to code to this
      if (!message.deserializeAssociatedEndpointHandles(this)) {
        this.onPipeConnectionError();
        return false;
      }

      if (!endpoint.client) {
        // We need to wait until a client is attached in order to dispatch
        // further messages.
        this.onPipeConnectionError();
        return false;
      }

[yzshen1, 2017/04/20 18:12:13]

Please note that these two "return false" means different things:
- The first is in MultiplexRouter::Accept(), returning false means "the message
is invalid", which will cause HandleError() in Connector::ReadSingleMessage().
"Returning false" here is equivalent to "returning true &&
RaiseErrorInNonTestingMode()".
- The second is in ProcessIncomingMessage(), "returning false" means "the
message is queued and not yet processed".


So your change of adding this.onPipeConnectionError() to these two cases are
incorrect.
Please feel free to chat with me if you have further questions. Thanks!

       }
 
-      if (!endpoint.client) {
-        // We need to wait until a client is attached in order to dispatch
-        // further messages.
-        return false;
+      if (controlMessageHandler.isPipeControlMessage(message)) {
+        ok = this.controlMessageHandler_.accept(message);
+      } else {
+        var interfaceId = message.getInterfaceId();
+        var endpoint = this.endpoints_.get(interfaceId);
+        if (!endpoint || endpoint.closed) {
+          return true;
+        }
+
+        if (!endpoint.client) {
+          // We need to wait until a client is attached in order to dispatch
+          // further messages.

[yzshen1, 2017/04/19 21:11:34]

In this case, you are dropping the message.

Instead, you will need to cache this message and ask the underlying connector to
pause receiving messages.

If later a client is attached to an endpoint, you need to check whether the
cached message is targetting that endpoint, if yes, post a task to process that
cached message, and ask the underlying connector to resume.

[yzshen1, 2017/04/20 18:12:13]

In case you didn't notice: this comment has not been addressed. But I am fine if
you would like to add a TODO and address it in a separate CL.

+          return false;
+        }
+        ok = endpoint.client.handleIncomingMessage_(message, messageValidator);
       }
-      ok = endpoint.client.handleIncomingMessage_(message);
     }
 
     if (!ok) {
       this.handleInvalidIncomingMessage_();
     }
     return ok;
   };
 
   Router.prototype.close = function() {
     this.connector_.close();
@@ skipping 83 matching lines @@
     }
     if (endpoint.closed && endpoint.peerClosed) {
       this.endpoints_.delete(endpoint.id);
     }
   };
 
   var exports = {};
   exports.Router = Router;
   return exports;
 });