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.'); + }); });