From 351a99782b9677706b5dc0dd78e85978fa4ab130 Mon Sep 17 00:00:00 2001
From: Reilly Grant <reillyg@chromium.org>
Date: Wed, 9 Dec 2020 11:38:10 -0800
Subject: [PATCH] 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}
---
 resources/chromium/fake-serial.js         | 18 ++++++++++++++++++
 serial/serialPort_getSignals.https.any.js | 20 ++++++++++++++++++++
 serial/serialPort_setSignals.https.any.js | 14 ++++++++++++++
 3 files changed, 52 insertions(+)

diff --git a/resources/chromium/fake-serial.js b/resources/chromium/fake-serial.js
index 4e3dfcb5bdbae0..46a79f0147afa4 100644
--- a/resources/chromium/fake-serial.js
+++ b/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;
     }
diff --git a/serial/serialPort_getSignals.https.any.js b/serial/serialPort_getSignals.https.any.js
index 968697f36df693..a570e35987f4ce 100644
--- a/serial/serialPort_getSignals.https.any.js
+++ b/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');
diff --git a/serial/serialPort_setSignals.https.any.js b/serial/serialPort_setSignals.https.any.js
index ac20b25698cb05..27911ab7db5448 100644
--- a/serial/serialPort_setSignals.https.any.js
+++ b/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');