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

Co-authored-by: Paul Frazee <[email protected]>

shell/browser/net/node_stream_loader.cc

@@ -68,6 +68,8 @@ void NodeStreamLoader::Start(network::ResourceResponseHead head) {
 void NodeStreamLoader::NotifyReadable() {
   if (!readable_)
     ReadMore();
+  else if (is_reading_)
+    has_read_waiting_ = true;
   readable_ = true;
 }
 
@@ -100,8 +102,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_);
     }

shell/browser/net/node_stream_loader.h

@@ -85,6 +85,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_;
 

spec-main/api-protocol-spec.ts

@@ -2,6 +2,7 @@ import { expect } from 'chai'
 import { protocol, webContents, WebContents, session, BrowserWindow, ipcMain } from 'electron'
 import { promisify } from 'util'
 import { AddressInfo } from 'net'
+import { EventEmitter } from 'events'
 import * as path from 'path'
 import * as http from 'http'
 import * as fs from 'fs'
@@ -411,6 +412,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.isProtocolHandled', () => {