fix: fix races in didClose and didDisconnect across browsers (#182)

Also merge initialize and swapSessionOnNavigation in webkit.

src/chromium/Page.ts

@@ -55,7 +55,7 @@ export class Page extends EventEmitter {
   readonly keyboard: input.Keyboard;
   readonly mouse: input.Mouse;
   private _timeoutSettings: TimeoutSettings;
-  private _frameManager: FrameManager;
+  _frameManager: FrameManager;
   readonly accessibility: Accessibility;
   readonly coverage: Coverage;
   readonly overrides: Overrides;
@@ -69,14 +69,6 @@ export class Page extends EventEmitter {
   private _fileChooserInterceptors = new Set<(chooser: FileChooser) => void>();
   private _emulatedMediaType: string | undefined;
 
-  static async create(client: CDPSession, browserContext: BrowserContext, ignoreHTTPSErrors: boolean, defaultViewport: types.Viewport | null): Promise<Page> {
-    const page = new Page(client, browserContext, ignoreHTTPSErrors);
-    await page._frameManager.initialize();
-    if (defaultViewport)
-      await page.setViewport(defaultViewport);
-    return page;
-  }
-
   constructor(client: CDPSession, browserContext: BrowserContext, ignoreHTTPSErrors: boolean) {
     super();
     this._client = client;

src/chromium/Target.ts

@@ -35,6 +35,7 @@ export class Target {
   private _ignoreHTTPSErrors: boolean;
   private _defaultViewport: types.Viewport;
   private _pagePromise: Promise<Page> | null = null;
+  private _page: Page | null = null;
   private _workerPromise: Promise<Worker> | null = null;
   _initializedPromise: Promise<boolean>;
   _initializedCallback: (value?: unknown) => void;
@@ -75,14 +76,15 @@ export class Target {
   }
 
   _didClose() {
-    if (this._pagePromise)
+    if (this._page)
-      this._pagePromise.then(page => page._didClose());
+      this._page._didClose();
   }
 
   async page(): Promise<Page | null> {
     if ((this._targetInfo.type === 'page' || this._targetInfo.type === 'background_page') && !this._pagePromise) {
       this._pagePromise = this._sessionFactory().then(async client => {
-        const page = await Page.create(client, this._browserContext, this._ignoreHTTPSErrors, this._defaultViewport);
+        const page = new Page(client, this._browserContext, this._ignoreHTTPSErrors);
+        this._page = page;
         page[targetSymbol] = this;
         client.once(CDPSessionEvents.Disconnected, () => page._didDisconnect());
         client.on('Target.attachedToTarget', event => {
@@ -91,7 +93,10 @@ export class Target {
             client.send('Target.detachFromTarget', { sessionId: event.sessionId }).catch(debugError);
           }
         });
+        await page._frameManager.initialize();
         await client.send('Target.setAutoAttach', {autoAttach: true, waitForDebuggerOnStart: false, flatten: true});
+        if (this._defaultViewport)
+          await page.setViewport(this._defaultViewport);
         return page;
       });
     }

src/firefox/Browser.ts

@@ -189,9 +189,10 @@ export class Browser extends EventEmitter {
 
 export class Target {
   _pagePromise?: Promise<Page>;
+  private _page: Page | null = null;
   private _browser: Browser;
   _context: BrowserContext;
-  private _connection: any;
+  private _connection: Connection;
   private _targetId: string;
   private _type: 'page' | 'browser';
   _url: string;
@@ -208,8 +209,8 @@ export class Target {
   }
 
   _didClose() {
-    if (this._pagePromise)
+    if (this._page)
-      this._pagePromise.then(page => page._didClose());
+      this._page._didClose();
   }
 
   opener(): Target | null {
@@ -230,12 +231,17 @@ export class Target {
     return this._context;
   }
 
-  async page() {
+  page(): Promise<Page> {
     if (this._type === 'page' && !this._pagePromise) {
-      const session = await this._connection.createSession(this._targetId);
+      this._pagePromise = new Promise(async f => {
-      this._pagePromise = Page.create(session, this._context, this._browser._defaultViewport).then(page => {
+        const session = await this._connection.createSession(this._targetId);
+        const page = new Page(session, this._context);
+        this._page = page;
         session.once(JugglerSessionEvents.Disconnected, () => page._didDisconnect());
-        return page;
+        await page._frameManager._initialize();
+        if (this._browser._defaultViewport)
+          await page.setViewport(this._browser._defaultViewport);
+        f(page);
       });
     }
     return this._pagePromise;

src/firefox/FrameManager.ts

@@ -82,6 +82,15 @@ export class FrameManager extends EventEmitter implements frames.FrameDelegate {
     ];
   }
 
+  async _initialize() {
+    await Promise.all([
+      this._session.send('Runtime.enable'),
+      this._session.send('Network.enable'),
+      this._session.send('Page.enable'),
+      this._session.send('Page.setInterceptFileChooserDialog', { enabled: true })
+    ]);
+  }
+
   executionContextById(executionContextId) {
     return this._contextIdToContext.get(executionContextId) || null;
   }

src/firefox/Page.ts

@@ -61,20 +61,6 @@ export class Page extends EventEmitter {
   private _fileChooserInterceptors = new Set<(chooser: FileChooser) => void>();
   _screenshotter: Screenshotter;
 
-  static async create(session: JugglerSession, browserContext: BrowserContext, defaultViewport: types.Viewport | null) {
-    const page = new Page(session, browserContext);
-    await Promise.all([
-      session.send('Runtime.enable'),
-      session.send('Network.enable'),
-      session.send('Page.enable'),
-      session.send('Page.setInterceptFileChooserDialog', { enabled: true })
-    ]);
-
-    if (defaultViewport)
-      await page.setViewport(defaultViewport);
-    return page;
-  }
-
   constructor(session: JugglerSession, browserContext: BrowserContext) {
     super();
     this._timeoutSettings = new TimeoutSettings();

src/webkit/Connection.ts

@@ -228,8 +228,7 @@ export class TargetSession extends EventEmitter {
     }
     this._callbacks.clear();
     this._connection = null;
-    if (!this._swappedOut)
+    this.emit(TargetSessionEvents.Disconnected);
-      this.emit(TargetSessionEvents.Disconnected);
   }
 }
 

src/webkit/FrameManager.ts

@@ -53,23 +53,25 @@ export class FrameManager extends EventEmitter implements frames.FrameDelegate {
   _frames: Map<string, frames.Frame>;
   _contextIdToContext: Map<number, js.ExecutionContext>;
   _isolatedWorlds: Set<string>;
-  _sessionListeners: RegisteredListener[];
+  _sessionListeners: RegisteredListener[] = [];
   _mainFrame: frames.Frame;
 
-  constructor(session: TargetSession, page: Page, timeoutSettings: TimeoutSettings) {
+  constructor(page: Page, timeoutSettings: TimeoutSettings) {
     super();
-    this._session = session;
     this._page = page;
-    this._networkManager = new NetworkManager(session, this);
+    this._networkManager = new NetworkManager(this);
     this._timeoutSettings = timeoutSettings;
     this._frames = new Map();
     this._contextIdToContext = new Map();
     this._isolatedWorlds = new Set();
-
-    this._addSessionListeners();
   }
 
-  async initialize() {
+  async initialize(session: TargetSession) {
+    helper.removeEventListeners(this._sessionListeners);
+    this.disconnectFromTarget();
+    this._session = session;
+    this._addSessionListeners();
+    this.emit(FrameManagerEvents.TargetSwappedOnNavigation);
     const [,{frameTree}] = await Promise.all([
       // Page agent must be enabled before Runtime.
       this._session.send('Page.enable'),
@@ -81,7 +83,7 @@ export class FrameManager extends EventEmitter implements frames.FrameDelegate {
       this._session.send('Console.enable'),
       this._session.send('Dialog.enable'),
       this._session.send('Page.setInterceptFileChooserDialog', { enabled: true }),
-      this._networkManager.initialize(),
+      this._networkManager.initialize(session),
     ]);
     if (this._page._userAgent !== null)
       await this._session.send('Page.overrideUserAgent', { value: this._page._userAgent });
@@ -106,16 +108,6 @@ export class FrameManager extends EventEmitter implements frames.FrameDelegate {
     ];
   }
 
-  async _swapSessionOnNavigation(newSession: TargetSession) {
-    helper.removeEventListeners(this._sessionListeners);
-    this.disconnectFromTarget();
-    this._session = newSession;
-    this._addSessionListeners();
-    this._networkManager.setSession(newSession);
-    this.emit(FrameManagerEvents.TargetSwappedOnNavigation);
-    await this.initialize();
-  }
-
   disconnectFromTarget() {
     for (const context of this._contextIdToContext.values()) {
       (context._delegate as ExecutionContextDelegate)._dispose();

src/webkit/NetworkManager.ts

@@ -39,29 +39,20 @@ export class NetworkManager extends EventEmitter {
   private _userCacheDisabled = false;
   private _sessionListeners: RegisteredListener[] = [];
 
-  constructor(client: TargetSession, frameManager: FrameManager) {
+  constructor(frameManager: FrameManager) {
     super();
-    this._sesssion = client;
     this._frameManager = frameManager;
-
-    this._sesssion.on('Network.requestWillBeSent', this._onRequestWillBeSent.bind(this));
-    this._sesssion.on('Network.responseReceived', this._onResponseReceived.bind(this));
-    this._sesssion.on('Network.loadingFinished', this._onLoadingFinished.bind(this));
-    this._sesssion.on('Network.loadingFailed', this._onLoadingFailed.bind(this));
   }
 
-  setSession(newSession: TargetSession) {
+  async initialize(session: TargetSession) {
     helper.removeEventListeners(this._sessionListeners);
-    this._sesssion = newSession;
+    this._sesssion = session;
     this._sessionListeners = [
       helper.addEventListener(this._sesssion, 'Network.requestWillBeSent', this._onRequestWillBeSent.bind(this)),
       helper.addEventListener(this._sesssion, 'Network.responseReceived', this._onResponseReceived.bind(this)),
       helper.addEventListener(this._sesssion, 'Network.loadingFinished', this._onLoadingFinished.bind(this)),
       helper.addEventListener(this._sesssion, 'Network.loadingFailed', this._onLoadingFailed.bind(this)),
     ];
-  }
-
-  async initialize() {
     await this._sesssion.send('Network.enable');
     await this._sesssion.send('Network.setExtraHTTPHeaders', { headers: this._extraHTTPHeaders });
   }

src/webkit/Page.ts

@@ -57,18 +57,13 @@ export class Page extends EventEmitter {
   _screenshotter: Screenshotter;
   private _fileChooserInterceptors = new Set<(chooser: FileChooser) => void>();
 
-  constructor(session: TargetSession, browserContext: BrowserContext) {
+  constructor(browserContext: BrowserContext) {
     super();
     this._closedPromise = new Promise(f => this._closedCallback = f);
     this._disconnectedPromise = new Promise(f => this._disconnectedCallback = f);
-    this._keyboard = new input.Keyboard(new RawKeyboardImpl(session));
-    this._mouse = new input.Mouse(new RawMouseImpl(session), this._keyboard);
     this._timeoutSettings = new TimeoutSettings();
-    this._frameManager = new FrameManager(session, this, this._timeoutSettings);
+    this._frameManager = new FrameManager(this, this._timeoutSettings);
 
-    this._screenshotter = new Screenshotter(this, new WKScreenshotDelegate(session), browserContext.browser());
-
-    this._session = session;
     this._browserContext = browserContext;
 
     this._frameManager.on(FrameManagerEvents.FrameAttached, event => this.emit(Events.Page.FrameAttached, event));
@@ -96,13 +91,12 @@ export class Page extends EventEmitter {
     this._disconnectedCallback(new Error('Target closed'));
   }
 
-  _initialize() {
+  _initialize(session: TargetSession) {
-    return this._frameManager.initialize();
+    this._session = session;
-  }
+    this._keyboard = new input.Keyboard(new RawKeyboardImpl(session));
-
+    this._mouse = new input.Mouse(new RawMouseImpl(session), this._keyboard);
-  async _swapSessionOnNavigation(newSession: TargetSession) {
+    this._screenshotter = new Screenshotter(this, new WKScreenshotDelegate(session), this._browserContext.browser());
-    this._session = newSession;
+    return this._frameManager.initialize(session);
-    await this._frameManager._swapSessionOnNavigation(newSession);
   }
 
   browser(): Browser {

src/webkit/Target.ts

@@ -28,6 +28,7 @@ export class Target {
   _targetId: string;
   private _type: 'page' | 'service-worker' | 'worker';
   private _pagePromise: Promise<Page> | null = null;
+  private _page: Page | null = null;
   private _url: string;
   _initializedPromise: Promise<boolean>;
   _initializedCallback: (value?: unknown) => void;
@@ -49,36 +50,45 @@ export class Target {
   }
 
   _didClose() {
-    if (this._pagePromise)
+    if (this._page)
-      this._pagePromise.then(page => page._didClose());
+      this._page._didClose();
   }
 
   async _swappedIn(oldTarget: Target, session: TargetSession) {
     this._pagePromise = oldTarget._pagePromise;
+    this._page = oldTarget._page;
     // Swapped out target should not be accessed by anyone. Reset page promise so that
     // old target does not close the page on connection reset.
     oldTarget._pagePromise = null;
-    if (!this._pagePromise)
+    oldTarget._page = null;
-      return;
+    if (this._pagePromise)
-    const page = await this._pagePromise;
+      this._adoptPage(this._page || await this._pagePromise, session);
-    (page as any)[targetSymbol] = this;
-    page._swapSessionOnNavigation(session).catch(rethrowIfNotSwapped);
   }
 
-  async page(): Promise<Page | null> {
+  private async _adoptPage(page: Page, session: TargetSession) {
+    this._page = page;
+    (page as any)[targetSymbol] = this;
+    session.once(TargetSessionEvents.Disconnected, () => {
+      // Once swapped out, we reset _page and won't call _didDisconnect for old session.
+      if (this._page === page)
+        page._didDisconnect();
+    });
+    await page._initialize(session).catch(e => {
+      // Swallow initialization errors due to newer target swap in,
+      // since we will reinitialize again.
+      if (!isSwappedOutError(e))
+        throw e;
+    });
+  }
+
+  async page(): Promise<Page> {
     if (this._type === 'page' && !this._pagePromise) {
       const session = this.browser()._connection.session(this._targetId);
       this._pagePromise = new Promise(async f => {
-        const page = new Page(session, this._browserContext);
+        const page = new Page(this._browserContext);
-        await page._initialize().catch(rethrowIfNotSwapped);
+        await this._adoptPage(page, session);
         if (this.browser()._defaultViewport)
           await page.setViewport(this.browser()._defaultViewport);
-        (page as any)[targetSymbol] = this;
-        session.once(TargetSessionEvents.Disconnected, () => {
-          // Check that this target has not been swapped out.
-          if ((page as any)[targetSymbol] === this)
-            page._didDisconnect();
-        });
         f(page);
       });
     }
@@ -101,8 +111,3 @@ export class Target {
     return this._browserContext;
   }
 }
-
-function rethrowIfNotSwapped(e: Error) {
-  if (!isSwappedOutError(e))
-    throw e;
-}