feat(downloads): subscribe to download events in Browser domain instead of Page (#6082)

Author: yury-s

src/server/chromium/crBrowser.ts

@@ -81,6 +81,8 @@ export class CRBrowser extends Browser {
     this._connection.on(ConnectionEvents.Disconnected, () => this._didClose());
     this._session.on('Target.attachedToTarget', this._onAttachedToTarget.bind(this));
     this._session.on('Target.detachedFromTarget', this._onDetachedFromTarget.bind(this));
+    this._session.on('Browser.downloadWillBegin', this._onDownloadWillBegin.bind(this));
+    this._session.on('Browser.downloadProgress', this._onDownloadProgress.bind(this));
   }
 
   async newContext(options: types.BrowserContextOptions): Promise<BrowserContext> {
@@ -188,6 +190,36 @@ export class CRBrowser extends Browser {
     }
   }
 
+  private _findOwningPage(frameId: string) {
+    for (const crPage of this._crPages.values()) {
+      const frame = crPage._page._frameManager.frame(frameId);
+      if (frame)
+        return crPage;
+    }
+    return null;
+  }
+
+  _onDownloadWillBegin(payload: Protocol.Browser.downloadWillBeginPayload) {
+    const page = this._findOwningPage(payload.frameId);
+    assert(page, 'Download started in unknown page: ' + JSON.stringify(payload));
+    page.willBeginDownload();
+
+    let originPage = page._initializedPage;
+    // If it's a new window download, report it on the opener page.
+    if (!originPage && page._opener)
+      originPage = page._opener._initializedPage;
+    if (!originPage)
+      return;
+    this._downloadCreated(originPage, payload.guid, payload.url, payload.suggestedFilename);
+  }
+
+  _onDownloadProgress(payload: any) {
+    if (payload.state === 'completed')
+      this._downloadFinished(payload.guid, '');
+    if (payload.state === 'canceled')
+      this._downloadFinished(payload.guid, 'canceled');
+  }
+
   async _closePage(crPage: CRPage) {
     await this._session.send('Target.closeTarget', { targetId: crPage._targetId });
   }
@@ -283,7 +315,8 @@ export class CRBrowserContext extends BrowserContext {
       promises.push(this._browser._session.send('Browser.setDownloadBehavior', {
         behavior: this._options.acceptDownloads ? 'allowAndName' : 'deny',
         browserContextId: this._browserContextId,
-        downloadPath: this._browser.options.downloadsPath
+        downloadPath: this._browser.options.downloadsPath,
+        eventsEnabled: true,
       }));
     }
     if (this._options.permissions)

src/server/chromium/crPage.ts

@@ -149,6 +149,10 @@ export class CRPage implements PageDelegate {
     return this._sessionForFrame(frame);
   }
 
+  willBeginDownload() {
+    this._mainFrameSession._willBeginDownload();
+  }
+
   async pageOrError(): Promise<Page | Error> {
     return this._pagePromise;
   }
@@ -415,8 +419,6 @@ class FrameSession {
   private _addBrowserListeners() {
     this._eventListeners.push(...[
       helper.addEventListener(this._client, 'Inspector.targetCrashed', event => this._onTargetCrashed()),
-      helper.addEventListener(this._client, 'Page.downloadWillBegin', event => this._onDownloadWillBegin(event)),
-      helper.addEventListener(this._client, 'Page.downloadProgress', event => this._onDownloadProgress(event)),
       helper.addEventListener(this._client, 'Page.screencastFrame', event => this._onScreencastFrame(event)),
       helper.addEventListener(this._client, 'Page.windowOpen', event => this._onWindowOpen(event)),
     ]);
@@ -818,26 +820,13 @@ class FrameSession {
     await this._page._onFileChooserOpened(handle);
   }
 
-  _onDownloadWillBegin(payload: Protocol.Page.downloadWillBeginPayload) {
-    let originPage = this._crPage._initializedPage;
-    // If it's a new window download, report it on the opener page.
+  _willBeginDownload() {
+    const originPage = this._crPage._initializedPage;
     if (!originPage) {
       // Resume the page creation with an error. The page will automatically close right
       // after the download begins.
       this._firstNonInitialNavigationCommittedReject(new Error('Starting new page download'));
-      if (this._crPage._opener)
-        originPage = this._crPage._opener._initializedPage;
     }
-    if (!originPage)
-      return;
-    this._crPage._browserContext._browser._downloadCreated(originPage, payload.guid, payload.url, payload.suggestedFilename);
-  }
-
-  _onDownloadProgress(payload: Protocol.Page.downloadProgressPayload) {
-    if (payload.state === 'completed')
-      this._crPage._browserContext._browser._downloadFinished(payload.guid, '');
-    if (payload.state === 'canceled')
-      this._crPage._browserContext._browser._downloadFinished(payload.guid, 'canceled');
   }
 
   _onScreencastFrame(payload: Protocol.Page.screencastFramePayload) {

tests/download.spec.ts

@@ -18,7 +18,6 @@ import { browserTest as it, expect } from './config/browserTest';
 import fs from 'fs';
 import path from 'path';
 import crypto from 'crypto';
-import { chromiumVersionLessThan } from './config/utils';
 
 it.describe('download event', () => {
   it.beforeEach(async ({server}) => {
@@ -272,12 +271,7 @@ it.describe('download event', () => {
     await page.close();
   });
 
-  it('should report new window downloads', async ({browser, server, browserName, headless}) => {
-    it.fixme(browserName === 'chromium' && !headless);
-
-    // TODO: - the test fails in headed Chromium as the popup page gets closed along
-    // with the session before download completed event arrives.
-    // - WebKit doesn't close the popup page
+  it('should report new window downloads', async ({browser, server}) => {
     const page = await browser.newPage({ acceptDownloads: true });
     await page.setContent(`<a target=_blank href="${server.PREFIX}/download">download</a>`);
     const [ download ] = await Promise.all([
@@ -360,7 +354,7 @@ it.describe('download event', () => {
     expect(fs.existsSync(path.join(path1, '..'))).toBeFalsy();
   });
 
-  it('should close the context without awaiting the failed download', async ({browser, server, httpsServer, browserName, browserVersion}, testInfo) => {
+  it('should close the context without awaiting the failed download', async ({browser, server, httpsServer, browserName, headless}, testInfo) => {
     it.skip(browserName !== 'chromium', 'Only Chromium downloads on alt-click');
 
     const page = await browser.newPage({ acceptDownloads: true });
@@ -378,13 +372,10 @@ it.describe('download event', () => {
       page.context().close(),
     ]);
     expect(downloadPath).toBe(null);
-    if (chromiumVersionLessThan(browserVersion, '91.0.4472.0'))
-      expect(saveError.message).toContain('File deleted upon browser context closure.');
-    else
-      expect(saveError.message).toContain('File not found on disk. Check download.failure() for details.');
+    expect(saveError.message).toContain('File not found on disk. Check download.failure() for details.');
   });
 
-  it('should close the context without awaiting the download', async ({browser, server, browserName, platform}, testInfo) => {
+  it('should close the context without awaiting the download', async ({browser, server, browserName, platform, headless}, testInfo) => {
     it.skip(browserName === 'webkit' && platform === 'linux', 'WebKit on linux does not convert to the download immediately upon receiving headers');
 
     server.setRoute('/downloadStall', (req, res) => {
@@ -408,7 +399,10 @@ it.describe('download event', () => {
       page.context().close(),
     ]);
     expect(downloadPath).toBe(null);
-    expect(saveError.message).toContain('File deleted upon browser context closure.');
+    if (browserName === 'chromium' && headless)
+      expect(saveError.message).toContain('.saveAs: canceled');
+    else
+      expect(saveError.message).toContain('File deleted upon browser context closure.');
   });
 
   it('should throw if browser dies', async ({ server, browserType, browserName, browserOptions, platform}, testInfo) => {