[serial] Handle fatal read and write errors correctly

This change adds special handling for read and write errors for which
recovery is impossible (i.e. disconnection). In this case the readable
and writeable attribute are not initialized with new streams on demand.

Bug: 1013763
Change-Id: Ibd03cb684cb34cf9a8b22848f7b9ebc50f4d6621
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1884357Reviewed-by: Matt Reynolds <mattreynolds@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710088}

third_party/blink/renderer/modules/serial/serial_port.cc

@@ -35,6 +35,18 @@ const char kDeviceLostError[] = "The device has been lost.";
 const char kSystemError[] = "An unknown system error has occurred.";
 const int kMaxBufferSize = 16 * 1024 * 1024; /* 16 MiB */
 
+bool SendErrorIsFatal(SerialSendError error) {
+  switch (error) {
+    case SerialSendError::NONE:
+      NOTREACHED();
+      return false;
+    case SerialSendError::SYSTEM_ERROR:
+      return false;
+    case SerialSendError::DISCONNECTED:
+      return true;
+  }
+}
+
 DOMException* DOMExceptionFromSendError(SerialSendError error) {
   switch (error) {
     case SerialSendError::NONE:
@@ -49,6 +61,24 @@ DOMException* DOMExceptionFromSendError(SerialSendError error) {
   }
 }
 
+bool ReceiveErrorIsFatal(SerialReceiveError error) {
+  switch (error) {
+    case SerialReceiveError::NONE:
+      NOTREACHED();
+      return false;
+    case SerialReceiveError::BREAK:
+    case SerialReceiveError::FRAME_ERROR:
+    case SerialReceiveError::OVERRUN:
+    case SerialReceiveError::BUFFER_OVERFLOW:
+    case SerialReceiveError::PARITY_ERROR:
+    case SerialReceiveError::SYSTEM_ERROR:
+      return false;
+    case SerialReceiveError::DISCONNECTED:
+    case SerialReceiveError::DEVICE_LOST:
+      return true;
+  }
+}
+
 DOMException* DOMExceptionFromReceiveError(SerialReceiveError error) {
   switch (error) {
     case SerialReceiveError::NONE:
@@ -252,7 +282,7 @@ ReadableStream* SerialPort::readable(ScriptState* script_state,
   if (readable_)
     return readable_;
 
-  if (!port_ || open_resolver_ || closing_)
+  if (!port_ || open_resolver_ || closing_ || read_fatal_)
     return nullptr;
 
   mojo::ScopedDataPipeConsumerHandle readable_pipe;
@@ -273,7 +303,7 @@ WritableStream* SerialPort::writable(ScriptState* script_state,
   if (writable_)
     return writable_;
 
-  if (!port_ || open_resolver_ || closing_)
+  if (!port_ || open_resolver_ || closing_ || write_fatal_)
     return nullptr;
 
   mojo::ScopedDataPipeProducerHandle writable_pipe;
@@ -432,15 +462,17 @@ void SerialPort::Dispose() {
 }
 
 void SerialPort::OnReadError(device::mojom::blink::SerialReceiveError error) {
-  if (underlying_source_) {
+  if (ReceiveErrorIsFatal(error))
+    read_fatal_ = true;
+  if (underlying_source_)
     underlying_source_->SignalErrorOnClose(DOMExceptionFromReceiveError(error));
-  }
 }
 
 void SerialPort::OnSendError(device::mojom::blink::SerialSendError error) {
-  if (underlying_sink_) {
+  if (SendErrorIsFatal(error))
+    write_fatal_ = true;
+  if (underlying_sink_)
     underlying_sink_->SignalErrorOnClose(DOMExceptionFromSendError(error));
-  }
 }
 
 bool SerialPort::CreateDataPipe(mojo::ScopedDataPipeProducerHandle* producer,
@@ -461,6 +493,8 @@ bool SerialPort::CreateDataPipe(mojo::ScopedDataPipeProducerHandle* producer,
 
 void SerialPort::OnConnectionError() {
   closing_ = false;
+  read_fatal_ = false;
+  write_fatal_ = false;
   port_.reset();
   client_receiver_.reset();
 
@@ -589,6 +623,8 @@ void SerialPort::OnSetSignals(ScriptPromiseResolver* resolver, bool success) {
 void SerialPort::OnClose() {
   DCHECK(close_resolver_);
   closing_ = false;
+  read_fatal_ = false;
+  write_fatal_ = false;
   port_.reset();
   client_receiver_.reset();
 

third_party/blink/renderer/modules/serial/serial_port.h

@@ -96,6 +96,11 @@ class SerialPort final : public ScriptWrappable,
   Member<WritableStream> writable_;
   Member<SerialPortUnderlyingSink> underlying_sink_;
 
+  // Indicates that the read or write streams have encountered a fatal error and
+  // should not be reopened.
+  bool read_fatal_ = false;
+  bool write_fatal_ = false;
+
   // Indicates that the port is being closed and so the streams should not be
   // reopened on demand.
   bool closing_ = false;

third_party/blink/renderer/modules/serial/serial_port_underlying_sink.cc

@@ -102,6 +102,7 @@ void SerialPortUnderlyingSink::SignalErrorOnClose(DOMException* exception) {
   if (pending_write_) {
     pending_write_->Reject(exception);
     pending_write_ = nullptr;
+    serial_port_->UnderlyingSinkClosed();
   }
 }
 

third_party/blink/web_tests/serial/resources/serial-test-utils.js

@@ -155,12 +155,34 @@ class FakeSerialPort {
     return result;
   }
 
-  simulateParityError() {
+  simulateReadError(error) {
     this.writer_.close();
     this.writer_.releaseLock();
     this.writer_ = undefined;
     this.writable_ = undefined;
-    this.client_.onReadError(device.mojom.SerialReceiveError.PARITY_ERROR);
+    this.client_.onReadError(error);
+  }
+
+  simulateParityError() {
+    this.simulateReadError(device.mojom.SerialReceiveError.PARITY_ERROR);
+  }
+
+  simulateDisconnectOnRead() {
+    this.simulateReadError(device.mojom.SerialReceiveError.DISCONNECTED);
+  }
+
+  simulateWriteError(error) {
+    this.readable_.cancel();
+    this.readable_ = undefined;
+    this.client_.onSendError(error);
+  }
+
+  simulateSystemErrorOnWrite() {
+    this.simulateWriteError(device.mojom.SerialSendError.SYSTEM_ERROR);
+  }
+
+  simulateDisconnectOnWrite() {
+    this.simulateWriteError(device.mojom.SerialSendError.DISCONNECTED);
   }
 
   simulateInputSignals(signals) {
@@ -171,17 +193,30 @@ class FakeSerialPort {
     return this.outputSignals_;
   }
 
-  waitForErrorCleared() {
+  waitForReadErrorCleared() {
     if (this.writable_)
       return Promise.resolve();
 
-    if (!this.errorClearedPromise_) {
-      this.errorClearedPromise_ = new Promise((resolve) => {
-        this.errorCleared_ = resolve;
+    if (!this.readErrorClearedPromise_) {
+      this.readErrorClearedPromise_ = new Promise((resolve) => {
+        this.readErrorCleared_ = resolve;
       });
     }
 
-    return this.errorClearedPromise_;
+    return this.readErrorClearedPromise_;
+  }
+
+  waitForWriteErrorCleared() {
+    if (this.readable_)
+      return Promise.resolve();
+
+    if (!this.writeErrorClearedPromise_) {
+      this.writeErrorClearedPromise_ = new Promise((resolve) => {
+        this.writeErrorCleared_ = resolve;
+      });
+    }
+
+    return this.writeErrorClearedPromise_;
   }
 
   async open(options, in_stream, out_stream, client) {
@@ -195,13 +230,23 @@ class FakeSerialPort {
     return { success: true };
   }
 
-  async clearSendError(in_stream) {}
+  async clearSendError(in_stream) {
+    this.readable_ = new ReadableStream(new DataPipeSource(in_stream));
+    if (this.writeErrorCleared_) {
+      this.writeErrorCleared_();
+      this.writeErrorCleared_ = undefined;
+      this.writeErrorClearedPromise_ = undefined;
+    }
+  }
 
   async clearReadError(out_stream) {
     this.writable_ = new WritableStream(new DataPipeSink(out_stream));
     this.writer_ = this.writable_.getWriter();
-    if (this.errorCleared_)
-      this.errorCleared_();
+    if (this.readErrorCleared_) {
+      this.readErrorCleared_();
+      this.readErrorCleared_ = undefined;
+      this.readErrorClearedPromise_ = undefined;
+    }
   }
 
   async flush() {

third_party/blink/web_tests/serial/serialPort_readable.html

@@ -96,7 +96,7 @@ serial_test(async (t, fake) => {
   assert_true(readable instanceof ReadableStream);
   reader = port.readable.getReader();
 
-  await fakePort.waitForErrorCleared();
+  await fakePort.waitForReadErrorCleared();
   fakePort.write(data);
 
   ({ value, done } = await reader.read());
@@ -108,6 +108,26 @@ serial_test(async (t, fake) => {
   assert_equals(port.readable, null);
 }, 'Parity error closes readable and replaces it with a new stream');
 
+serial_test(async (t, fake) => {
+  const { port, fakePort } = await getFakeSerialPort(fake);
+  // Select a buffer size smaller than the amount of data transferred.
+  await port.open({ baudrate: 9600, buffersize: 64 });
+
+  const data = new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8]);
+  fakePort.write(data);
+  fakePort.simulateDisconnectOnRead();
+
+  const reader = port.readable.getReader();
+  const { value, done } = await reader.read();
+  assert_false(done);
+  compareArrays(data, value);
+
+  await promise_rejects(t, 'NetworkError', reader.read());
+  assert_equals(port.readable, null);
+
+  await port.close();
+}, 'Disconnect error closes readable and sets it to null');
+
 serial_test(async (t, fake) => {
   const { port, fakePort } = await getFakeSerialPort(fake);
   // Select a buffer size larger than the amount of data transferred.

third_party/blink/web_tests/serial/serialPort_writable.html

@@ -76,6 +76,48 @@ serial_test(async (t, fake) => {
   await port.close();
 }, 'Can read a large amount of data');
 
+serial_test(async (t, fake) => {
+  const { port, fakePort } = await getFakeSerialPort(fake);
+  await port.open({ baudrate: 9600 });
+
+  const writable = port.writable;
+  assert_true(writable instanceof WritableStream);
+  let writer = writable.getWriter();
+
+  fakePort.simulateSystemErrorOnWrite();
+  const data = new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8]);
+  await promise_rejects(t, 'UnknownError', writer.write(data));
+
+  assert_true(port.writable instanceof WritableStream);
+  assert_not_equals(port.writable, writable);
+
+  writer = port.writable.getWriter();
+  let writePromise = writer.write(data);
+  writer.close();
+  await fakePort.waitForWriteErrorCleared();
+  let { value, done } = await fakePort.read();
+  await writePromise;
+  compareArrays(value, data);
+
+  await port.close();
+  assert_equals(port.writable, null);
+}, 'System error closes writable and replaces it with a new stream');
+
+serial_test(async (t, fake) => {
+  const { port, fakePort } = await getFakeSerialPort(fake);
+  await port.open({ baudrate: 9600 });
+
+  assert_true(port.writable instanceof WritableStream);
+  const writer = port.writable.getWriter();
+
+  fakePort.simulateDisconnectOnWrite();
+  const data = new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8]);
+  await promise_rejects(t, 'NetworkError', writer.write(data));
+  assert_equals(port.writable, null);
+
+  await port.close();
+}, 'Disconnect error closes writable and sets it to null');
+
 serial_test(async (t, fake) => {
   const { port, fakePort } = await getFakeSerialPort(fake);