serial: Wait for stream closure before finishing close

The SerialPort close() method internally calls this.readable.cancel()
and this.writable.abort() to close its data streams before closing the
port itself. When either of these streams is already erroring the
Promises returned by these methods can be resolved before the underlying
source or sink has actually finished closing the stream.

This patch adds logic to wait until the streams are actually closed
before continuing the close process.

Bug: 1137563
Change-Id: Ic2c1fd10524292f5b33dfc8279804fe89b00ce7c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2462711
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820375}

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

@@ -428,15 +428,21 @@ ScriptPromise SerialPort::close(ScriptState* script_state,
 
 ScriptPromise SerialPort::ContinueClose(ScriptState* script_state) {
   DCHECK(closing_);
-  DCHECK(!readable_);
-  DCHECK(!writable_);
   DCHECK(!close_resolver_);
 
   if (!port_.is_bound())
     return ScriptPromise::CastUndefined(script_state);
 
   close_resolver_ = MakeGarbageCollected<ScriptPromiseResolver>(script_state);
-  port_->Close(WTF::Bind(&SerialPort::OnClose, WrapPersistent(this)));
+
+  // readable.cancel() and writable.abort() can resolve before |readable_| or
+  // |writable_| are set to null if the streams were already erroring. Wait
+  // until UnderlyingSourceClosed() and UnderlyingSinkClosed() have been called
+  // before continuing.
+  if (!readable_ && !writable_) {
+    StreamsClosed();
+  }
+
   return close_resolver_->Promise();
 }
 
@@ -445,6 +451,12 @@ void SerialPort::AbortClose() {
   closing_ = false;
 }
 
+void SerialPort::StreamsClosed() {
+  DCHECK(!readable_);
+  DCHECK(!writable_);
+  port_->Close(WTF::Bind(&SerialPort::OnClose, WrapPersistent(this)));
+}
+
 void SerialPort::Flush(
     device::mojom::blink::SerialPortFlushMode mode,
     device::mojom::blink::SerialPort::FlushCallback callback) {
@@ -462,12 +474,20 @@ void SerialPort::UnderlyingSourceClosed() {
   DCHECK(readable_);
   readable_ = nullptr;
   underlying_source_ = nullptr;
+
+  if (close_resolver_ && !writable_) {
+    StreamsClosed();
+  }
 }
 
 void SerialPort::UnderlyingSinkClosed() {
   DCHECK(writable_);
   writable_ = nullptr;
   underlying_sink_ = nullptr;
+
+  if (close_resolver_ && !readable_) {
+    StreamsClosed();
+  }
 }
 
 void SerialPort::ContextDestroyed() {

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

@@ -60,6 +60,7 @@ class SerialPort final : public ScriptWrappable,
 
   ScriptPromise ContinueClose(ScriptState*);
   void AbortClose();
+  void StreamsClosed();
 
   void Flush(device::mojom::blink::SerialPortFlushMode mode,
              device::mojom::blink::SerialPort::FlushCallback callback);

third_party/blink/web_tests/wpt_internal/serial/serialPort_readable_cancel.https.window.js

@@ -20,6 +20,24 @@ serial_test(async (t, fake) => {
   await port.close();
 }, 'Can cancel while reading');
 
+serial_test(async (t, fake) => {
+  const {port, fakePort} = await getFakeSerialPort(fake);
+  await port.open({baudRate: 9600, bufferSize: 64});
+
+  const reader = port.readable.getReader();
+  const closed = (async () => {
+    const {value, done} = await reader.read();
+    assert_true(done);
+    assert_equals(undefined, value);
+    reader.releaseLock();
+    await port.close();
+    assert_equals(port.readable, null);
+  })();
+
+  await reader.cancel();
+  await closed;
+}, 'Can close while canceling');
+
 serial_test(async (t, fake) => {
   const {port, fakePort} = await getFakeSerialPort(fake);
   await port.open({baudRate: 9600, bufferSize: 64});

third_party/blink/web_tests/wpt_internal/serial/serialPort_writable.https.window.js

@@ -150,6 +150,49 @@ serial_test(async (t, fake) => {
   assert_equals(port.writable, null);
 }, 'abort() discards the write buffer');
 
+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 writer = port.writable.getWriter();
+  const data = new Uint8Array(1024);  // Much larger than bufferSize above.
+  for (let i = 0; i < data.byteLength; ++i)
+    data[i] = i & 0xff;
+  let writePromise = writer.write(data).catch(reason => {
+    assert_equals(reason, 'Aborting.');
+  });
+
+  await writer.abort('Aborting.');
+  await writePromise;
+  await port.close();
+  assert_equals(port.writable, null);
+}, 'abort() does not wait for the write buffer to be cleared');
+
+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 writer = port.writable.getWriter();
+  const data = new Uint8Array(1024);  // Much larger than bufferSize above.
+  for (let i = 0; i < data.byteLength; ++i)
+    data[i] = i & 0xff;
+  let closed = (async () => {
+    try {
+      await writer.write(data);
+    } catch (reason) {
+      assert_equals(reason, 'Aborting.');
+      writer.releaseLock();
+      await port.close();
+      assert_equals(port.writable, null);
+    }
+  })();
+
+  await writer.abort('Aborting.');
+  await closed;
+}, 'Can close while aborting');
+
 serial_test(async (t, fake) => {
   const {port, fakePort} = await getFakeSerialPort(fake);
   // Select a buffer size smaller than the amount of data transferred.