serial: Fix getSignals() error handling

This change fixes error handling for the getSignals() method. It was
assumed that the Mojo reply value could be null on error but this was
not allowed by serial.mojom. This meant that if a call to
GetControlSignals() failed it would trigger a Mojo connection failure
rather than only reporting the (potentially recoverable) error.

Tests exercising this case on both the renderer and browser process
sides have been added.

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

services/device/public/mojom/serial.mojom

@@ -188,8 +188,8 @@ interface SerialPort {
   // all buffered data to be transmitted by the port.
   Drain() => ();
 
-  // Reads current control signals (DCD, CTS, etc.).
-  GetControlSignals() => (SerialPortControlSignals signals);
+  // Reads current control signals (DCD, CTS, etc.). Returns null on failure.
+  GetControlSignals() => (SerialPortControlSignals? signals);
 
   // Sets one or more control signals and returns result.
   SetControlSignals(SerialHostControlSignals signals) => (bool success);

services/device/serial/serial_port_impl_unittest.cc

@@ -24,20 +24,38 @@ class FakeSerialIoHandler : public SerialIoHandler {
   FakeSerialIoHandler()
       : SerialIoHandler(base::FilePath(), /*ui_thread_task_runner=*/nullptr) {}
 
+  void SimulateOpenFailure(bool fail) { fail_open_ = fail; }
+
+  void SimulateGetControlSignalsFailure(bool fail) {
+    fail_get_control_signals_ = fail;
+  }
+
+  void SimulateSetControlSignalsFailure(bool fail) {
+    fail_set_control_signals_ = fail;
+  }
+
+  // SerialIoHandler implementation
   void Open(const mojom::SerialConnectionOptions& options,
             OpenCompleteCallback callback) override {
-    std::move(callback).Run(true);
+    std::move(callback).Run(!fail_open_);
   }
 
   void Flush(mojom::SerialPortFlushMode mode) const override {}
   void Drain() override {}
 
   mojom::SerialPortControlSignalsPtr GetControlSignals() const override {
-    return mojom::SerialPortControlSignals::New();
+    if (fail_get_control_signals_)
+      return nullptr;
+
+    return input_signals_.Clone();
   }
 
   bool SetControlSignals(
       const mojom::SerialHostControlSignals& control_signals) override {
+    if (fail_set_control_signals_)
+      return false;
+
+    output_signals_ = control_signals;
     return true;
   }
 
@@ -57,10 +75,20 @@ class FakeSerialIoHandler : public SerialIoHandler {
     QueueWriteCompleted(/*bytes_written=*/0, mojom::SerialSendError::NONE);
   }
 
-  bool ConfigurePortImpl() override { return true; }
+  bool ConfigurePortImpl() override {
+    // Open() is overridden so this should never be called.
+    ADD_FAILURE() << "ConfigurePortImpl() should not be reached.";
+    return false;
+  }
 
  private:
   ~FakeSerialIoHandler() override = default;
+
+  mojom::SerialPortControlSignals input_signals_;
+  mojom::SerialHostControlSignals output_signals_;
+  bool fail_open_ = false;
+  bool fail_get_control_signals_ = false;
+  bool fail_set_control_signals_ = false;
 };
 
 }  // namespace
@@ -72,17 +100,17 @@ class SerialPortImplTest : public DeviceServiceTestBase {
   void operator=(const SerialPortImplTest& other) = delete;
   ~SerialPortImplTest() override = default;
 
-  void CreatePort(
+  scoped_refptr<FakeSerialIoHandler> CreatePort(
       mojo::Remote<mojom::SerialPort>* port,
       mojo::SelfOwnedReceiverRef<mojom::SerialPortConnectionWatcher>* watcher) {
+    auto io_handler = base::MakeRefCounted<FakeSerialIoHandler>();
     mojo::PendingRemote<mojom::SerialPortConnectionWatcher> watcher_remote;
     *watcher = mojo::MakeSelfOwnedReceiver(
         std::make_unique<mojom::SerialPortConnectionWatcher>(),
         watcher_remote.InitWithNewPipeAndPassReceiver());
     base::RunLoop loop;
     SerialPortImpl::OpenForTesting(
-        base::MakeRefCounted<FakeSerialIoHandler>(),
-        mojom::SerialConnectionOptions::New(), mojo::NullRemote(),
+        io_handler, mojom::SerialConnectionOptions::New(), mojo::NullRemote(),
         std::move(watcher_remote),
         base::BindLambdaForTesting(
             [&](mojo::PendingRemote<mojom::SerialPort> pending_remote) {
@@ -91,6 +119,7 @@ class SerialPortImplTest : public DeviceServiceTestBase {
               loop.Quit();
             }));
     loop.Run();
+    return io_handler;
   }
 
   void CreateDataPipe(mojo::ScopedDataPipeProducerHandle* producer,
@@ -190,6 +219,61 @@ TEST_F(SerialPortImplTest, FlushRead) {
   watcher_loop.Run();
 }
 
+TEST_F(SerialPortImplTest, OpenFailure) {
+  auto io_handler = base::MakeRefCounted<FakeSerialIoHandler>();
+  io_handler->SimulateOpenFailure(true);
+
+  mojo::PendingRemote<mojom::SerialPortConnectionWatcher> watcher_remote;
+  mojo::MakeSelfOwnedReceiver(
+      std::make_unique<mojom::SerialPortConnectionWatcher>(),
+      watcher_remote.InitWithNewPipeAndPassReceiver());
+  base::RunLoop loop;
+  SerialPortImpl::OpenForTesting(
+      io_handler, mojom::SerialConnectionOptions::New(), mojo::NullRemote(),
+      std::move(watcher_remote),
+      base::BindLambdaForTesting(
+          [&](mojo::PendingRemote<mojom::SerialPort> pending_remote) {
+            EXPECT_FALSE(pending_remote.is_valid());
+            loop.Quit();
+          }));
+  loop.Run();
+}
+
+TEST_F(SerialPortImplTest, GetControlSignalsFailure) {
+  mojo::Remote<mojom::SerialPort> serial_port;
+  mojo::SelfOwnedReceiverRef<mojom::SerialPortConnectionWatcher> watcher;
+  scoped_refptr<FakeSerialIoHandler> io_handler =
+      CreatePort(&serial_port, &watcher);
+  io_handler->SimulateGetControlSignalsFailure(true);
+
+  base::RunLoop loop;
+  serial_port->GetControlSignals(base::BindLambdaForTesting(
+      [&](mojom::SerialPortControlSignalsPtr signals) {
+        EXPECT_FALSE(signals);
+        loop.Quit();
+      }));
+  loop.Run();
+}
+
+TEST_F(SerialPortImplTest, SetControlSignalsFailure) {
+  mojo::Remote<mojom::SerialPort> serial_port;
+  mojo::SelfOwnedReceiverRef<mojom::SerialPortConnectionWatcher> watcher;
+  scoped_refptr<FakeSerialIoHandler> io_handler =
+      CreatePort(&serial_port, &watcher);
+  io_handler->SimulateSetControlSignalsFailure(true);
+
+  base::RunLoop loop;
+  auto signals = mojom::SerialHostControlSignals::New();
+  signals->has_dtr = true;
+  signals->dtr = true;
+  serial_port->SetControlSignals(std::move(signals),
+                                 base::BindLambdaForTesting([&](bool success) {
+                                   EXPECT_FALSE(success);
+                                   loop.Quit();
+                                 }));
+  loop.Run();
+}
+
 TEST_F(SerialPortImplTest, FlushWrite) {
   mojo::Remote<mojom::SerialPort> serial_port;
   mojo::SelfOwnedReceiverRef<mojom::SerialPortConnectionWatcher> watcher;

third_party/blink/web_tests/external/wpt/resources/chromium/fake-serial.js

@@ -93,11 +93,13 @@ class FakeSerialPort {
       ringIndicator: false,
       dataSetReady: false
     };
+    this.inputSignalFailure_ = false;
     this.outputSignals_ = {
       dataTerminalReady: false,
       requestToSend: false,
       break: false
     };
+    this.outputSignalFailure_ = false;
   }
 
   open(options, client) {
@@ -170,10 +172,18 @@ class FakeSerialPort {
     this.inputSignals_ = signals;
   }
 
+  simulateInputSignalFailure(fail) {
+    this.inputSignalFailure_ = fail;
+  }
+
   get outputSignals() {
     return this.outputSignals_;
   }
 
+  simulateOutputSignalFailure(fail) {
+    this.outputSignalFailure_ = fail;
+  }
+
   writable() {
     if (this.writable_)
       return Promise.resolve();
@@ -241,6 +251,10 @@ class FakeSerialPort {
   }
 
   async getControlSignals() {
+    if (this.inputSignalFailure_) {
+      return {signals: null};
+    }
+
     const signals = {
       dcd: this.inputSignals_.dataCarrierDetect,
       cts: this.inputSignals_.clearToSend,
@@ -251,6 +265,10 @@ class FakeSerialPort {
   }
 
   async setControlSignals(signals) {
+    if (this.outputSignalFailure_) {
+      return {success: false};
+    }
+
     if (signals.hasDtr) {
       this.outputSignals_.dataTerminalReady = signals.dtr;
     }

third_party/blink/web_tests/external/wpt/serial/serialPort_getSignals.https.any.js

@@ -41,3 +41,23 @@ serial_test(async (t, fake) => {
   signals = await port.getSignals();
   assert_object_equals(signals, expectedSignals, 'DSR set');
 }, 'getSignals() returns the current state of input control signals');
+
+serial_test(async (t, fake) => {
+  const {port, fakePort} = await getFakeSerialPort(fake);
+  await port.open({baudRate: 9600});
+
+  fakePort.simulateInputSignalFailure(true);
+  await promise_rejects_dom(t, 'NetworkError', port.getSignals());
+
+  fakePort.simulateInputSignalFailure(false);
+  const expectedSignals = {
+    dataCarrierDetect: false,
+    clearToSend: false,
+    ringIndicator: false,
+    dataSetReady: false
+  };
+  const signals = await port.getSignals();
+  assert_object_equals(signals, expectedSignals);
+
+  await port.close();
+}, 'getSignals() rejects on failure');

third_party/blink/web_tests/external/wpt/serial/serialPort_setSignals.https.any.js

@@ -43,3 +43,17 @@ serial_test(async (t, fake) => {
   expectedSignals.break = false;
   assert_object_equals(fakePort.outputSignals, expectedSignals, 'invert');
 }, 'setSignals() modifies the state of the port');
+
+serial_test(async (t, fake) => {
+  const {port, fakePort} = await getFakeSerialPort(fake);
+  await port.open({baudRate: 9600});
+
+  fakePort.simulateOutputSignalFailure(true);
+  await promise_rejects_dom(t, 'NetworkError', port.setSignals({break: true}));
+
+  fakePort.simulateOutputSignalFailure(false);
+  await port.setSignals({break: true});
+  assert_true(fakePort.outputSignals.break);
+
+  await port.close();
+}, 'setSignals() rejects on failure');