[serial] Make SerialPortClient interface non-associated

The SerialPort.Open method originally took a SerialPortClient as an
associated interface. This ensures the ordering of responses to
SerialPort methods is synchronized with events on the SerialPortClient
interface.

This prevents SerialPort.Open from being used from the Mojo JS bindings
because they do not support associated interfaces. Web tests for the
Serial API implementation in Blink would like to use these bindings.

Upon deeper investigation the only method for which ordering really
matters is SerialPort.Open itself. This is because serial data is sent
using a pair of data pipes and those do not have any ordering guarantee
with either the SerialPort or SerialPortClient pipes.

To avoid messages on the now-unassociated SerialPortClient interface
from racing ahead of the reply to the SerialPort.Open method the
SerialConnection class is modified to only create its client binding
and start watchers on the data pipes after the reply to the Open method
is received.

Bug: 893334
Change-Id: I511060d45c5d537af1edddaeccd0436e34c0b0f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1660603Reviewed-by: Dominick Ng <dominickn@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#669578}

extensions/browser/api/serial/serial_apitest.cc

@@ -76,11 +76,11 @@ class FakeSerialPort : public device::mojom::SerialPort {
   void Open(device::mojom::SerialConnectionOptionsPtr options,
             mojo::ScopedDataPipeConsumerHandle in_stream,
             mojo::ScopedDataPipeProducerHandle out_stream,
-            device::mojom::SerialPortClientAssociatedPtrInfo client,
+            device::mojom::SerialPortClientPtr client,
             OpenCallback callback) override {
     DoConfigurePort(*options);
     DCHECK(client);
-    client_.Bind(std::move(client));
+    client_ = std::move(client);
     SetUpInStreamPipe(std::move(in_stream));
     SetUpOutStreamPipe(std::move(out_stream));
     std::move(callback).Run(true);
@@ -250,7 +250,7 @@ class FakeSerialPort : public device::mojom::SerialPort {
   std::vector<uint8_t> buffer_;
   int read_step_ = 0;
   int write_step_ = 0;
-  device::mojom::SerialPortClientAssociatedPtr client_;
+  device::mojom::SerialPortClientPtr client_;
   mojo::ScopedDataPipeConsumerHandle in_stream_;
   mojo::SimpleWatcher in_stream_watcher_;
   mojo::ScopedDataPipeProducerHandle out_stream_;

extensions/browser/api/serial/serial_connection.cc

@@ -215,9 +215,11 @@ void SerialConnection::SetPaused(bool paused) {
     // If |receive_pipe_| is closed and there is no pending ReceiveError event,
     // try to reconnect the data pipe.
     if (!receive_pipe_ && !read_error_) {
-      mojo::ScopedDataPipeProducerHandle receive_producer;
-      SetUpReceiveDataPipe(&receive_producer);
-      serial_port_->ClearReadError(std::move(receive_producer));
+      mojo::ScopedDataPipeProducerHandle producer;
+      mojo::ScopedDataPipeConsumerHandle consumer;
+      CreatePipe(&producer, &consumer);
+      SetUpReceiveDataPipe(std::move(consumer));
+      serial_port_->ClearReadError(std::move(producer));
     }
     receive_pipe_watcher_.ArmOrNotify();
     receive_timeout_task_.Cancel();
@@ -239,6 +241,8 @@ void SerialConnection::Open(const api::serial::ConnectionOptions& options,
                             OpenCompleteCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(serial_port_);
+  DCHECK(!send_pipe_);
+  DCHECK(!receive_pipe_);
 
   if (options.persistent.get())
     set_persistent(*options.persistent);
@@ -251,42 +255,42 @@ void SerialConnection::Open(const api::serial::ConnectionOptions& options,
   if (options.send_timeout.get())
     set_send_timeout(*options.send_timeout);
 
-  mojo::ScopedDataPipeConsumerHandle consumer;
-  mojo::ScopedDataPipeProducerHandle producer;
-  if (!send_pipe_) {
-    SetUpSendDataPipe(&consumer);
-  }
-  DCHECK(send_pipe_);
-  // Make sure receive_pipe_ only be initialized once.
-  if (!receive_pipe_) {
-    SetUpReceiveDataPipe(&producer);
-  }
-  DCHECK(receive_pipe_);
-  // In case Open() being called more than once.
-  if (client_binding_) {
-    client_binding_.Close();
-  }
-  device::mojom::SerialPortClientAssociatedPtrInfo client;
-  client_binding_.Bind(mojo::MakeRequest(&client));
-  client_binding_.set_connection_error_handler(base::BindOnce(
-      &SerialConnection::OnClientBindingClosed, weak_factory_.GetWeakPtr()));
+  mojo::ScopedDataPipeProducerHandle receive_producer;
+  mojo::ScopedDataPipeConsumerHandle receive_consumer;
+  CreatePipe(&receive_producer, &receive_consumer);
+
+  mojo::ScopedDataPipeProducerHandle send_producer;
+  mojo::ScopedDataPipeConsumerHandle send_consumer;
+  CreatePipe(&send_producer, &send_consumer);
+
+  device::mojom::SerialPortClientPtr client;
+  auto client_request = mojo::MakeRequest(&client);
 
   serial_port_->Open(
       device::mojom::SerialConnectionOptions::From(options),
-      std::move(consumer), std::move(producer), std::move(client),
-      mojo::WrapCallbackWithDefaultInvokeIfNotRun(std::move(callback), false));
+      std::move(send_consumer), std::move(receive_producer), std::move(client),
+      mojo::WrapCallbackWithDefaultInvokeIfNotRun(
+          base::BindOnce(&SerialConnection::OnOpen, weak_factory_.GetWeakPtr(),
+                         std::move(receive_consumer), std::move(send_producer),
+                         std::move(client_request), std::move(callback)),
+          false));
 }
 
-void SerialConnection::SetUpReceiveDataPipe(
-    mojo::ScopedDataPipeProducerHandle* producer) {
+void SerialConnection::CreatePipe(
+    mojo::ScopedDataPipeProducerHandle* producer,
+    mojo::ScopedDataPipeConsumerHandle* consumer) {
   MojoCreateDataPipeOptions options;
   options.struct_size = sizeof(MojoCreateDataPipeOptions);
   options.flags = MOJO_CREATE_DATA_PIPE_FLAG_NONE;
   options.element_num_bytes = 1;
   options.capacity_num_bytes = buffer_size_;
 
-  CHECK_EQ(MOJO_RESULT_OK,
-           mojo::CreateDataPipe(&options, producer, &receive_pipe_));
+  CHECK_EQ(MOJO_RESULT_OK, mojo::CreateDataPipe(&options, producer, consumer));
+}
+
+void SerialConnection::SetUpReceiveDataPipe(
+    mojo::ScopedDataPipeConsumerHandle consumer) {
+  receive_pipe_ = std::move(consumer);
   receive_pipe_watcher_.Watch(
       receive_pipe_.get(),
       MOJO_HANDLE_SIGNAL_READABLE | MOJO_HANDLE_SIGNAL_PEER_CLOSED,
@@ -296,15 +300,8 @@ void SerialConnection::SetUpReceiveDataPipe(
 }
 
 void SerialConnection::SetUpSendDataPipe(
-    mojo::ScopedDataPipeConsumerHandle* consumer) {
-  MojoCreateDataPipeOptions options;
-  options.struct_size = sizeof(MojoCreateDataPipeOptions);
-  options.flags = MOJO_CREATE_DATA_PIPE_FLAG_NONE;
-  options.element_num_bytes = 1;
-  options.capacity_num_bytes = buffer_size_;
-
-  CHECK_EQ(MOJO_RESULT_OK,
-           mojo::CreateDataPipe(&options, &send_pipe_, consumer));
+    mojo::ScopedDataPipeProducerHandle producer) {
+  send_pipe_ = std::move(producer);
   send_pipe_watcher_.Watch(
       send_pipe_.get(),
       MOJO_HANDLE_SIGNAL_WRITABLE | MOJO_HANDLE_SIGNAL_PEER_CLOSED,
@@ -339,6 +336,25 @@ void SerialConnection::OnSendError(device::mojom::SerialSendError error) {
   bytes_written_ = 0;
 }
 
+void SerialConnection::OnOpen(
+    mojo::ScopedDataPipeConsumerHandle consumer,
+    mojo::ScopedDataPipeProducerHandle producer,
+    device::mojom::SerialPortClientRequest client_request,
+    OpenCompleteCallback callback,
+    bool success) {
+  if (!success) {
+    std::move(callback).Run(false);
+    return;
+  }
+
+  SetUpReceiveDataPipe(std::move(consumer));
+  SetUpSendDataPipe(std::move(producer));
+  client_binding_.Bind(std::move(client_request));
+  client_binding_.set_connection_error_handler(base::BindOnce(
+      &SerialConnection::OnClientBindingClosed, weak_factory_.GetWeakPtr()));
+  std::move(callback).Run(true);
+}
+
 void SerialConnection::OnReadPipeClosed() {
   receive_pipe_watcher_.Cancel();
   receive_pipe_.reset();
@@ -414,8 +430,10 @@ bool SerialConnection::Send(const std::vector<uint8_t>& data,
   data_to_send_.assign(data.begin(), data.end());
 
   if (!send_pipe_) {
+    mojo::ScopedDataPipeProducerHandle producer;
     mojo::ScopedDataPipeConsumerHandle consumer;
-    SetUpSendDataPipe(&consumer);
+    CreatePipe(&producer, &consumer);
+    SetUpSendDataPipe(std::move(producer));
     serial_port_->ClearSendError(std::move(consumer));
   }
   send_pipe_watcher_.ArmOrNotify();

extensions/browser/api/serial/serial_connection.h

@@ -18,7 +18,7 @@
 #include "extensions/browser/api/api_resource.h"
 #include "extensions/browser/api/api_resource_manager.h"
 #include "extensions/common/api/serial.h"
-#include "mojo/public/cpp/bindings/associated_binding.h"
+#include "mojo/public/cpp/bindings/binding.h"
 #include "mojo/public/cpp/system/data_pipe.h"
 #include "mojo/public/cpp/system/simple_watcher.h"
 #include "net/base/io_buffer.h"
@@ -153,15 +153,22 @@ class SerialConnection : public ApiResource,
   void OnReadError(device::mojom::SerialReceiveError error) override;
   void OnSendError(device::mojom::SerialSendError error) override;
 
+  void OnOpen(mojo::ScopedDataPipeConsumerHandle consumer,
+              mojo::ScopedDataPipeProducerHandle producer,
+              device::mojom::SerialPortClientRequest client_request,
+              OpenCompleteCallback callback,
+              bool success);
+
   // Read data from |receive_pipe_| when the data is ready or dispatch error
   // events in error cases.
   void OnReadPipeReadableOrClosed(MojoResult result,
                                   const mojo::HandleSignalsState& state);
   void OnReadPipeClosed();
 
-  void SetUpReceiveDataPipe(mojo::ScopedDataPipeProducerHandle* producer);
-
-  void SetUpSendDataPipe(mojo::ScopedDataPipeConsumerHandle* consumer);
+  void CreatePipe(mojo::ScopedDataPipeProducerHandle* producer,
+                  mojo::ScopedDataPipeConsumerHandle* consumer);
+  void SetUpReceiveDataPipe(mojo::ScopedDataPipeConsumerHandle producer);
+  void SetUpSendDataPipe(mojo::ScopedDataPipeProducerHandle consumer);
 
   void SetTimeoutCallback();
 
@@ -233,7 +240,7 @@ class SerialConnection : public ApiResource,
   mojo::ScopedDataPipeProducerHandle send_pipe_;
   mojo::SimpleWatcher send_pipe_watcher_;
 
-  mojo::AssociatedBinding<device::mojom::SerialPortClient> client_binding_;
+  mojo::Binding<device::mojom::SerialPortClient> client_binding_;
 
   // Closure which is set by client and will be called when |serial_port_|
   // connection encountered an error.

services/device/public/cpp/test/fake_serial_port_manager.cc

@@ -32,7 +32,7 @@ class FakeSerialPort : public mojom::SerialPort {
   void Open(mojom::SerialConnectionOptionsPtr options,
             mojo::ScopedDataPipeConsumerHandle in_stream,
             mojo::ScopedDataPipeProducerHandle out_stream,
-            mojom::SerialPortClientAssociatedPtrInfo client,
+            mojom::SerialPortClientPtr client,
             OpenCallback callback) override {
     in_stream_ = std::move(in_stream);
     out_stream_ = std::move(out_stream);
@@ -77,7 +77,7 @@ class FakeSerialPort : public mojom::SerialPort {
   // Mojo handles to keep open in order to simulate an active connection.
   mojo::ScopedDataPipeConsumerHandle in_stream_;
   mojo::ScopedDataPipeProducerHandle out_stream_;
-  mojom::SerialPortClientAssociatedPtrInfo client_;
+  mojom::SerialPortClientPtr client_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeSerialPort);
 };

services/device/public/mojom/serial.mojom

@@ -106,7 +106,7 @@ interface SerialPort {
   Open(SerialConnectionOptions options,
        handle<data_pipe_consumer> in_stream,
        handle<data_pipe_producer> out_stream,
-       associated SerialPortClient client) => (bool success);
+       SerialPortClient client) => (bool success);
 
   // Try to clear existing send error and reconnect the data pipe for send.
   // This is supposed to be called after SerialPortClient#OnSendError is

services/device/serial/serial_port_impl.cc

@@ -51,14 +51,14 @@ SerialPortImpl::~SerialPortImpl() = default;
 void SerialPortImpl::Open(mojom::SerialConnectionOptionsPtr options,
                           mojo::ScopedDataPipeConsumerHandle in_stream,
                           mojo::ScopedDataPipeProducerHandle out_stream,
-                          mojom::SerialPortClientAssociatedPtrInfo client,
+                          mojom::SerialPortClientPtr client,
                           OpenCallback callback) {
   DCHECK(in_stream);
   DCHECK(out_stream);
   in_stream_ = std::move(in_stream);
   out_stream_ = std::move(out_stream);
   if (client) {
-    client_.Bind(std::move(client));
+    client_ = std::move(client);
   }
   io_handler_->Open(*options, base::BindOnce(&SerialPortImpl::OnOpenCompleted,
                                              weak_factory_.GetWeakPtr(),

services/device/serial/serial_port_impl.h

@@ -47,7 +47,7 @@ class SerialPortImpl : public mojom::SerialPort {
   void Open(mojom::SerialConnectionOptionsPtr options,
             mojo::ScopedDataPipeConsumerHandle in_stream,
             mojo::ScopedDataPipeProducerHandle out_stream,
-            mojom::SerialPortClientAssociatedPtrInfo client,
+            mojom::SerialPortClientPtr client,
             OpenCallback callback) override;
   void ClearSendError(mojo::ScopedDataPipeConsumerHandle consumer) override;
   void ClearReadError(mojo::ScopedDataPipeProducerHandle producer) override;
@@ -76,7 +76,7 @@ class SerialPortImpl : public mojom::SerialPort {
   scoped_refptr<SerialIoHandler> io_handler_;
 
   // Client interfaces.
-  mojom::SerialPortClientAssociatedPtr client_;
+  mojom::SerialPortClientPtr client_;
   mojom::SerialPortConnectionWatcherPtr watcher_;
 
   // Data pipes for input and output.