From 2c409b040ea59387b8dccec9a6bd729ba057b51f Mon Sep 17 00:00:00 2001 From: Max Schmitt Date: Wed, 16 Dec 2020 05:15:25 +0100 Subject: [PATCH] fix(android): leaking adb socket connections (#4730) --- android-types-internal.d.ts | 1 + src/client/android.ts | 1 + src/client/events.ts | 3 ++- src/dispatchers/androidDispatcher.ts | 4 ++++ src/protocol/channels.ts | 2 ++ src/protocol/protocol.yml | 1 + src/server/android/android.ts | 3 ++- src/server/android/backendAdb.ts | 11 +++++++++-- test/android/android.fixtures.ts | 2 +- test/android/device.spec.ts | 3 +++ 10 files changed, 26 insertions(+), 5 deletions(-) diff --git a/android-types-internal.d.ts b/android-types-internal.d.ts index 675c7fee2c..60f1500db4 100644 --- a/android-types-internal.d.ts +++ b/android-types-internal.d.ts @@ -52,6 +52,7 @@ export interface AndroidDevice exte export interface AndroidSocket extends EventEmitter { on(event: 'data', handler: (data: Buffer) => void): this; + on(event: 'close', handler: () => void): this; write(data: Buffer): Promise close(): Promise } diff --git a/src/client/android.ts b/src/client/android.ts index 874367d36c..880a817e54 100644 --- a/src/client/android.ts +++ b/src/client/android.ts @@ -262,6 +262,7 @@ export class AndroidSocket extends ChannelOwner this.emit(Events.AndroidSocket.Data, Buffer.from(data, 'base64'))); + this._channel.on('close', () => this.emit(Events.AndroidSocket.Close)); } async write(data: Buffer): Promise { diff --git a/src/client/events.ts b/src/client/events.ts index 813b3ed79b..391c8637e4 100644 --- a/src/client/events.ts +++ b/src/client/events.ts @@ -22,7 +22,8 @@ export const Events = { }, AndroidSocket: { - Data: 'data' + Data: 'data', + Close: 'close' }, AndroidWebView: { diff --git a/src/dispatchers/androidDispatcher.ts b/src/dispatchers/androidDispatcher.ts index 1f675515d5..993f18f7d7 100644 --- a/src/dispatchers/androidDispatcher.ts +++ b/src/dispatchers/androidDispatcher.ts @@ -177,6 +177,10 @@ export class AndroidSocketDispatcher extends Dispatcher this._dispatchEvent('data', { data: data.toString('base64') })); + socket.on(Events.AndroidSocket.Close, () => { + this._dispatchEvent('close'); + this._dispose(); + }); } async write(params: channels.AndroidSocketWriteParams, metadata?: channels.Metadata): Promise { diff --git a/src/protocol/channels.ts b/src/protocol/channels.ts index c10fdb9f89..deb0610914 100644 --- a/src/protocol/channels.ts +++ b/src/protocol/channels.ts @@ -2425,12 +2425,14 @@ export type AndroidSetDefaultTimeoutNoReplyResult = void; export type AndroidSocketInitializer = {}; export interface AndroidSocketChannel extends Channel { on(event: 'data', callback: (params: AndroidSocketDataEvent) => void): this; + on(event: 'close', callback: (params: AndroidSocketCloseEvent) => void): this; write(params: AndroidSocketWriteParams, metadata?: Metadata): Promise; close(params?: AndroidSocketCloseParams, metadata?: Metadata): Promise; } export type AndroidSocketDataEvent = { data: Binary, }; +export type AndroidSocketCloseEvent = {}; export type AndroidSocketWriteParams = { data: Binary, }; diff --git a/src/protocol/protocol.yml b/src/protocol/protocol.yml index c2ab226cf6..7210d52b4a 100644 --- a/src/protocol/protocol.yml +++ b/src/protocol/protocol.yml @@ -2097,6 +2097,7 @@ AndroidSocket: data: parameters: data: binary + close: AndroidDevice: type: interface diff --git a/src/server/android/android.ts b/src/server/android/android.ts index 46fb645d9a..0d163b400a 100644 --- a/src/server/android/android.ts +++ b/src/server/android/android.ts @@ -171,7 +171,7 @@ export class AndroidDevice extends EventEmitter { await this.installApk(await readFileAsync(require.resolve(`../../../bin/${file}`))); debug('pw:android')('Starting the new driver'); - this.shell(`am instrument -w com.microsoft.playwright.androiddriver.test/androidx.test.runner.AndroidJUnitRunner`); + this.shell('am instrument -w com.microsoft.playwright.androiddriver.test/androidx.test.runner.AndroidJUnitRunner'); const socket = await this._waitForLocalAbstract('playwright_android_driver_socket'); const transport = new Transport(socket, socket, socket, 'be'); transport.onmessage = message => { @@ -285,6 +285,7 @@ export class AndroidDevice extends EventEmitter { await installSocket.write(content); const success = await new Promise(f => installSocket.on('data', f)); debug('pw:android')('Written driver bytes: ' + success); + await installSocket.close(); } async push(content: Buffer, path: string, mode = 0o644): Promise { diff --git a/src/server/android/backendAdb.ts b/src/server/android/backendAdb.ts index 2eaa9193c0..c0494a8a99 100644 --- a/src/server/android/backendAdb.ts +++ b/src/server/android/backendAdb.ts @@ -68,11 +68,15 @@ async function runCommand(command: string, serial?: string): Promise { await socket.write(encodeMessage(command)); const status = await socket.read(4); assert(status.toString() === 'OKAY', status.toString()); + let commandOutput: Buffer; if (!command.startsWith('shell:')) { const remainingLength = parseInt((await socket.read(4)).toString(), 16); - return (await socket.read(remainingLength)); + commandOutput = await socket.read(remainingLength); + } else { + commandOutput = await socket.readAll(); } - return (await socket.readAll()); + await socket.close(); + return commandOutput; } async function open(command: string, serial?: string): Promise { @@ -122,6 +126,7 @@ class BufferedSocketWrapper extends EventEmitter implements SocketBackend { this._isClosed = true; if (this._notifyReader) this._notifyReader(); + this.close(); this.emit('close'); }); this._socket.on('error', error => this.emit('error', error)); @@ -134,6 +139,8 @@ class BufferedSocketWrapper extends EventEmitter implements SocketBackend { } async close() { + if (this._isClosed) + return; debug('pw:adb')('Close ' + this._command); this._socket.destroy(); } diff --git a/test/android/android.fixtures.ts b/test/android/android.fixtures.ts index b5c3122edb..2a7e3bdc6c 100644 --- a/test/android/android.fixtures.ts +++ b/test/android/android.fixtures.ts @@ -29,7 +29,7 @@ fixtures.device.init(async ({ playwright }, runTest) => { await device.shell('am force-stop com.android.chrome'); device.setDefaultTimeout(120000); await runTest(device); - device.close(); + await device.close(); }); export const folio = fixtures.build(); diff --git a/test/android/device.spec.ts b/test/android/device.spec.ts index 3f49a0c441..4b633923e9 100644 --- a/test/android/device.spec.ts +++ b/test/android/device.spec.ts @@ -31,6 +31,9 @@ if (process.env.PW_ANDROID_TESTS) { await socket.write(Buffer.from('321\n')); const output = await new Promise(resolve => socket.on('data', resolve)); expect(output.toString()).toBe('321\n'); + const closedPromise = new Promise(resolve => socket.on('close', resolve)); + await socket.close(); + await closedPromise; }); it('androidDevice.screenshot', async function({ device, testInfo }) {