Skip to content

Commit

Permalink
fix: correctly handle nexttick scheduling in stream reads
Browse files Browse the repository at this point in the history
  • Loading branch information
pfrazee authored and zcbenz committed Jun 15, 2020
1 parent 32b45e7 commit 6c7400f
Show file tree
Hide file tree
Showing 3 changed files with 135 additions and 32 deletions.
12 changes: 11 additions & 1 deletion shell/browser/net/node_stream_loader.cc
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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_);
}
Expand Down
5 changes: 5 additions & 0 deletions shell/browser/net/node_stream_loader.h
Expand Up @@ -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_;

Expand Down
150 changes: 119 additions & 31 deletions spec-main/api-protocol-spec.ts
Expand Up @@ -26,6 +26,38 @@ const uninterceptProtocol = promisify(protocol.uninterceptProtocol)

const text = 'valar morghulis'
const protocolName = 'sp'
=======
import { expect } from 'chai';
import { protocol, webContents, WebContents, session, BrowserWindow, ipcMain } from 'electron/main';
import { AddressInfo } from 'net';
import * as ChildProcess from 'child_process';
import * as path from 'path';
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';

const fixturesPath = path.resolve(__dirname, '..', 'spec', 'fixtures');

const registerStringProtocol = protocol.registerStringProtocol;
const registerBufferProtocol = protocol.registerBufferProtocol;
const registerFileProtocol = protocol.registerFileProtocol;
const registerHttpProtocol = protocol.registerHttpProtocol;
const registerStreamProtocol = protocol.registerStreamProtocol;
const interceptStringProtocol = protocol.interceptStringProtocol;
const interceptBufferProtocol = protocol.interceptBufferProtocol;
const interceptHttpProtocol = protocol.interceptHttpProtocol;
const interceptStreamProtocol = protocol.interceptStreamProtocol;
const unregisterProtocol = protocol.unregisterProtocol;
const uninterceptProtocol = protocol.uninterceptProtocol;

const text = 'valar morghulis';
const protocolName = 'sp';
>>>>>>> 81d09bea4... fix: correctly handle nexttick scheduling in stream reads (#24022)
const postData = {
name: 'post test',
type: 'string'
Expand Down Expand Up @@ -406,38 +438,94 @@ describe('protocol module', () => {
statusCode: 200,
headers: { 'Content-Type': 'text/plain' },
data: getStream(1024 * 1024, Buffer.alloc(1024 * 1024 * 2)).pipe(dumbPassthrough())
})
})
const r = await ajax(protocolName + '://fake-host')
expect(r.data).to.have.lengthOf(1024 * 1024 * 2)
})
})

describe('protocol.isProtocolHandled', () => {
it('returns true for built-in protocols', async () => {
for (const p of ['about', 'file', 'http', 'https']) {
const handled = await protocol.isProtocolHandled(p)
expect(handled).to.be.true(`${p}: is handled`)
});
});
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;
}
})

it('returns false when scheme is not registered', async () => {
const result = await protocol.isProtocolHandled('no-exist')
expect(result).to.be.false('no-exist: is handled')
})

it('returns true for custom protocol', async () => {
await registerStringProtocol(protocolName, (request, callback) => callback())
const result = await protocol.isProtocolHandled(protocolName)
expect(result).to.be.true('custom protocol is handled')
})

it('returns true for intercepted protocol', async () => {
await interceptStringProtocol('http', (request, callback) => callback())
const result = await protocol.isProtocolHandled('http')
expect(result).to.be.true('intercepted protocol is handled')
})
})
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;
});

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', () => {
it('returns false when scheme is not registered', () => {
const result = protocol.isProtocolRegistered('no-exist');
expect(result).to.be.false('no-exist: is handled');
});

it('returns true for custom protocol', () => {
registerStringProtocol(protocolName, (request, callback) => callback(''));
const result = protocol.isProtocolRegistered(protocolName);
expect(result).to.be.true('custom protocol is handled');
});
});

describe('protocol.isProtocolIntercepted', () => {
it('returns true for intercepted protocol', () => {
interceptStringProtocol('http', (request, callback) => callback(''));
const result = protocol.isProtocolIntercepted('http');
expect(result).to.be.true('intercepted protocol is handled');
});
});
>>>>>>> 81d09bea4... fix: correctly handle nexttick scheduling in stream reads (#24022)

describe('protocol.intercept(Any)Protocol', () => {
it('throws error when scheme is already intercepted', (done) => {
Expand Down

0 comments on commit 6c7400f

Please sign in to comment.