fix: correctly handle nexttick scheduling in stream reads (electron#24022)

pfrazee · web-flow · commit 81d09bea44bd · 2020-06-11T09:55:59.000-07:00
diff --git a/shell/browser/net/node_stream_loader.cc b/shell/browser/net/node_stream_loader.cc
@@ -68,6 +68,8 @@ void NodeStreamLoader::Start(network::mojom::URLResponseHeadPtr head) {
 void NodeStreamLoader::NotifyReadable() {
   if (!readable_)
     ReadMore();
+  else if (is_reading_)
+    has_read_waiting_ = true;
   readable_ = true;
 }
 
@@ -101,8 +103,16 @@ void NodeStreamLoader::ReadMore() {
   // If there is no buffer read, wait until |readable| is emitted again.
   v8::Local<v8::Value> buffer;
   if (!ret.ToLocal(&buffer) || !node::Buffer::HasInstance(buffer)) {
-    readable_ = false;
     is_reading_ = false;
+
+    // If 'readable' was called after 'read()', try again
+    if (has_read_waiting_) {
+      has_read_waiting_ = false;
+      ReadMore();
+      return;
+    }
+
+    readable_ = false;
     if (ended_) {
       NotifyComplete(result_);
     }
diff --git a/shell/browser/net/node_stream_loader.h b/shell/browser/net/node_stream_loader.h
@@ -87,6 +87,11 @@ class NodeStreamLoader : public network::mojom::URLLoader {
   // flag.
   bool readable_ = false;
 
+  // It's possible for reads to be queued using nextTick() during read()
+  // which will cause 'readable' to emit during ReadMore, so we track if
+  // that occurred in a flag.
+  bool has_read_waiting_ = false;
+
   // Store the V8 callbacks to unsubscribe them later.
   std::map<std::string, v8::Global<v8::Value>> handlers_;
 
diff --git a/spec-main/api-protocol-spec.ts b/spec-main/api-protocol-spec.ts
@@ -7,6 +7,7 @@ import * as http from 'http';
 import * as fs from 'fs';
 import * as qs from 'querystring';
 import * as stream from 'stream';
+import { EventEmitter } from 'events';
 import { closeWindow } from './window-helpers';
 import { emittedOnce } from './events-helpers';
 import { WebmGenerator } from './video-helpers';
@@ -410,6 +411,36 @@ describe('protocol module', () => {
       const r = await ajax(protocolName + '://fake-host');
       expect(r.data).to.have.lengthOf(1024 * 1024 * 2);
     });
+
+    it('can handle next-tick scheduling during read calls', async () => {
+      const events = new EventEmitter();
+      function createStream () {
+        const buffers = [
+          Buffer.alloc(65536),
+          Buffer.alloc(65537),
+          Buffer.alloc(39156)
+        ];
+        const e = new stream.Readable({ highWaterMark: 0 });
+        e.push(buffers.shift());
+        e._read = function () {
+          process.nextTick(() => this.push(buffers.shift() || null));
+        };
+        e.on('end', function () {
+          events.emit('end');
+        });
+        return e;
+      }
+      registerStreamProtocol(protocolName, (request, callback) => {
+        callback({
+          statusCode: 200,
+          headers: { 'Content-Type': 'text/plain' },
+          data: createStream()
+        });
+      });
+      const hasEndedPromise = emittedOnce(events, 'end');
+      ajax(protocolName + '://fake-host');
+      await hasEndedPromise;
+    });
   });
 
   describe('protocol.isProtocolRegistered', () => {
