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.

Author: dgozman

src/dispatchers/downloadDispatcher.ts

@@ -39,7 +39,7 @@ export class DownloadDispatcher extends Dispatcher<Download, channels.DownloadIn
     return await new Promise((resolve, reject) => {
       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<Download, channels.DownloadIn
     return await new Promise((resolve, reject) => {
       this._object.saveAs(async (localPath, error) => {
         if (error !== undefined) {
-          reject(error);
+          reject(new Error(error));
           return;
         }
 

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<any>[] = [];
-      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<void>[] = [];
+      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.

src/server/download.ts

@@ -107,7 +107,22 @@ export class Download {
       await util.promisify(fs.unlink)(fileName).catch(e => {});
   }
 
+  async deleteOnContextClose(): Promise<void> {
+    // 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;
 

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(`<a href="${httpsServer.PREFIX}/downloadWithFilename" download="file.txt">click me</a>`);
+    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(`<a href="${server.PREFIX}/downloadStall" download="file.txt">click me</a>`);
+    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.');
+  });
 });