From 1f3449c7dad60e00f81ed37a0faf16490cc6e52e Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Sun, 14 Feb 2021 16:46:26 -0800 Subject: [PATCH] fix(download): do not stall BrowserContext.close waiting for downloads (#5424) We might not ever get the "download finished" event when closing the context: - in Chromium, for any ongoing download; - in all browsers, for failed downloads. This should not prevent closing the context. Instead of waiting for the download and then deleting it, we force delete it immediately and reject any promises waiting for the download completion. --- src/dispatchers/downloadDispatcher.ts | 4 +-- src/server/browserContext.ts | 22 ++++++------ src/server/download.ts | 15 +++++++++ test/download.spec.ts | 48 +++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 12 deletions(-) diff --git a/src/dispatchers/downloadDispatcher.ts b/src/dispatchers/downloadDispatcher.ts index 1e7ba787a8ae4..449522bd30527 100644 --- a/src/dispatchers/downloadDispatcher.ts +++ b/src/dispatchers/downloadDispatcher.ts @@ -39,7 +39,7 @@ export class DownloadDispatcher extends Dispatcher { this._object.saveAs(async (localPath, error) => { if (error !== undefined) { - reject(error); + reject(new Error(error)); return; } @@ -58,7 +58,7 @@ export class DownloadDispatcher extends Dispatcher { this._object.saveAs(async (localPath, error) => { if (error !== undefined) { - reject(error); + reject(new Error(error)); return; } diff --git a/src/server/browserContext.ts b/src/server/browserContext.ts index 2b0f52f9db102..62411f030b957 100644 --- a/src/server/browserContext.ts +++ b/src/server/browserContext.ts @@ -252,15 +252,6 @@ export abstract class BrowserContext extends SdkObject { await this.instrumentation.onContextWillDestroy(this); - // Collect videos/downloads that we will await. - const promises: Promise[] = []; - for (const download of this._downloads) - promises.push(download.delete()); - for (const video of this._browser._idToVideo.values()) { - if (video._context === this) - promises.push(video._finishedPromise); - } - if (this._isPersistentContext) { // Close all the pages instead of the context, // because we cannot close the default context. @@ -270,7 +261,18 @@ export abstract class BrowserContext extends SdkObject { await this._doClose(); } - // Wait for the videos/downloads to finish. + // Cleanup. + const promises: Promise[] = []; + for (const video of this._browser._idToVideo.values()) { + // Wait for the videos to finish. + if (video._context === this) + promises.push(video._finishedPromise); + } + for (const download of this._downloads) { + // We delete downloads after context closure + // so that browser does not write to the download file anymore. + promises.push(download.deleteOnContextClose()); + } await Promise.all(promises); // Persistent context should also close the browser. diff --git a/src/server/download.ts b/src/server/download.ts index 98c8257215703..091fa9d454bf6 100644 --- a/src/server/download.ts +++ b/src/server/download.ts @@ -107,7 +107,22 @@ export class Download { await util.promisify(fs.unlink)(fileName).catch(e => {}); } + async deleteOnContextClose(): Promise { + // Compared to "delete", this method does not wait for the download to finish. + // We use it when closing the context to avoid stalling. + if (this._deleted) + return; + this._deleted = true; + if (this._acceptDownloads) { + const fileName = path.join(this._downloadsPath, this._uuid); + await util.promisify(fs.unlink)(fileName).catch(e => {}); + } + this._reportFinished('Download deleted upon browser context closure.'); + } + async _reportFinished(error?: string) { + if (this._finished) + return; this._finished = true; this._failure = error || null; diff --git a/test/download.spec.ts b/test/download.spec.ts index 24817743abce0..c3c78dfade118 100644 --- a/test/download.spec.ts +++ b/test/download.spec.ts @@ -365,4 +365,52 @@ describe('download event', () => { expect(fs.existsSync(path2)).toBeFalsy(); expect(fs.existsSync(path.join(path1, '..'))).toBeFalsy(); }); + + it('should close the context without awaiting the failed download', (test, { browserName }) => { + test.skip(browserName !== 'chromium', 'Only Chromium downloads on alt-click'); + }, async ({browser, server, httpsServer, testInfo}) => { + const page = await browser.newPage({ acceptDownloads: true }); + await page.goto(server.EMPTY_PAGE); + await page.setContent(`click me`); + const [download] = await Promise.all([ + page.waitForEvent('download'), + // Use alt-click to force the download. Otherwise browsers might try to navigate first, + // probably because of http -> https link. + page.click('a', { modifiers: ['Alt']}) + ]); + const [downloadPath, saveError] = await Promise.all([ + download.path(), + download.saveAs(testInfo.outputPath('download.txt')).catch(e => e), + page.context().close(), + ]); + expect(downloadPath).toBe(null); + expect(saveError.message).toContain('Download deleted upon browser context closure.'); + }); + + it('should close the context without awaiting the download', (test, { browserName, platform }) => { + test.skip(browserName === 'webkit' && platform === 'linux', 'WebKit on linux does not convert to the download immediately upon receiving headers'); + }, async ({browser, server, testInfo}) => { + server.setRoute('/downloadStall', (req, res) => { + res.setHeader('Content-Type', 'application/octet-stream'); + res.setHeader('Content-Disposition', 'attachment; filename=file.txt'); + res.writeHead(200); + res.flushHeaders(); + res.write(`Hello world`); + }); + + const page = await browser.newPage({ acceptDownloads: true }); + await page.goto(server.EMPTY_PAGE); + await page.setContent(`click me`); + const [download] = await Promise.all([ + page.waitForEvent('download'), + page.click('a') + ]); + const [downloadPath, saveError] = await Promise.all([ + download.path(), + download.saveAs(testInfo.outputPath('download.txt')).catch(e => e), + page.context().close(), + ]); + expect(downloadPath).toBe(null); + expect(saveError.message).toContain('Download deleted upon browser context closure.'); + }); });