feat(rpc): implement waitForNavigation on the client (#2949)

Drive-by: fix electron issues, exposed by the test using
waitForNavigation.

Drive-by: mark some tests skip(CHANNEL) that were mistakenly
marked skip(USES_HOOKS).

Author: dgozman

src/frames.ts

@@ -53,8 +53,16 @@ type ConsoleTagHandler = () => void;
 export type FunctionWithSource = (source: { context: BrowserContext, page: Page, frame: Frame}, ...args: any) => any;
 
 export const kNavigationEvent = Symbol('navigation');
-type NavigationEvent = {
-  documentInfo?: DocumentInfo,  // Undefined for same-document navigations.
+export type NavigationEvent = {
+  // New frame url after navigation.
+  url: string,
+  // New frame name after navigation.
+  name: string,
+  // Information about the new document for cross-document navigations.
+  // Undefined for same-document navigations.
+  newDocument?: DocumentInfo,
+  // Error for cross-document navigations if any. When error is present,
+  // the navigation did not commit.
   error?: Error,
 };
 export const kAddLifecycleEvent = Symbol('addLifecycle');
@@ -172,9 +180,9 @@ export class FrameManager {
     else
       frame._currentDocument = { documentId, request: undefined };
     frame._pendingDocument = undefined;
-    const navigationEvent: NavigationEvent = { documentInfo: frame._currentDocument };
-    frame._eventEmitter.emit(kNavigationEvent, navigationEvent);
     frame._onClearLifecycle();
+    const navigationEvent: NavigationEvent = { url, name, newDocument: frame._currentDocument };
+    frame._eventEmitter.emit(kNavigationEvent, navigationEvent);
     if (!initial) {
       this._page._logger.info(`  navigated to "${url}"`);
       this._page.emit(Events.Page.FrameNavigated, frame);
@@ -186,7 +194,7 @@ export class FrameManager {
     if (!frame)
       return;
     frame._url = url;
-    const navigationEvent: NavigationEvent = {};
+    const navigationEvent: NavigationEvent = { url, name: frame._name };
     frame._eventEmitter.emit(kNavigationEvent, navigationEvent);
     this._page._logger.info(`  navigated to "${url}"`);
     this._page.emit(Events.Page.FrameNavigated, frame);
@@ -198,7 +206,12 @@ export class FrameManager {
       return;
     if (documentId !== undefined && frame._pendingDocument.documentId !== documentId)
       return;
-    const navigationEvent: NavigationEvent = { documentInfo: frame._pendingDocument, error: new Error(errorText) };
+    const navigationEvent: NavigationEvent = {
+      url: frame._url,
+      name: frame._name,
+      newDocument: frame._pendingDocument,
+      error: new Error(errorText),
+    };
     frame._pendingDocument = undefined;
     frame._eventEmitter.emit(kNavigationEvent, navigationEvent);
   }
@@ -410,7 +423,7 @@ export class Frame {
       }
       url = helper.completeUserURL(url);
 
-      const sameDocument = helper.waitForEvent(progress, this._eventEmitter, kNavigationEvent, (e: NavigationEvent) => !e.documentInfo);
+      const sameDocument = helper.waitForEvent(progress, this._eventEmitter, kNavigationEvent, (e: NavigationEvent) => !e.newDocument);
       const navigateResult = await this._page._delegate.navigateFrame(this, url, referer);
 
       let event: NavigationEvent;
@@ -419,10 +432,10 @@ export class Frame {
         event = await helper.waitForEvent(progress, this._eventEmitter, kNavigationEvent, (event: NavigationEvent) => {
           // We are interested either in this specific document, or any other document that
           // did commit and replaced the expected document.
-          return event.documentInfo && (event.documentInfo.documentId === navigateResult.newDocumentId || !event.error);
+          return event.newDocument && (event.newDocument.documentId === navigateResult.newDocumentId || !event.error);
         }).promise;
 
-        if (event.documentInfo!.documentId !== navigateResult.newDocumentId) {
+        if (event.newDocument!.documentId !== navigateResult.newDocumentId) {
           // This is just a sanity check. In practice, new navigation should
           // cancel the previous one and report "request cancelled"-like error.
           throw new Error('Navigation interrupted by another one');
@@ -436,7 +449,7 @@ export class Frame {
       if (!this._subtreeLifecycleEvents.has(waitUntil))
         await helper.waitForEvent(progress, this._eventEmitter, kAddLifecycleEvent, (e: types.LifecycleEvent) => e === waitUntil).promise;
 
-      const request = event.documentInfo ? event.documentInfo.request : undefined;
+      const request = event.newDocument ? event.newDocument.request : undefined;
       return request ? request._finalRequest().response() : null;
     });
   }
@@ -460,7 +473,7 @@ export class Frame {
       if (!this._subtreeLifecycleEvents.has(waitUntil))
         await helper.waitForEvent(progress, this._eventEmitter, kAddLifecycleEvent, (e: types.LifecycleEvent) => e === waitUntil).promise;
 
-      const request = navigationEvent.documentInfo ? navigationEvent.documentInfo.request : undefined;
+      const request = navigationEvent.newDocument ? navigationEvent.newDocument.request : undefined;
       return request ? request._finalRequest().response() : null;
     });
   }

src/rpc/channels.ts

@@ -119,7 +119,6 @@ export interface PageChannel extends Channel {
   on(event: 'fileChooser', callback: (params: { element: ElementHandleChannel, isMultiple: boolean }) => void): this;
   on(event: 'frameAttached', callback: (params: { frame: FrameChannel }) => void): this;
   on(event: 'frameDetached', callback: (params: { frame: FrameChannel }) => void): this;
-  on(event: 'frameNavigated', callback: (params: { frame: FrameChannel, url: string, name: string }) => void): this;
   on(event: 'load', callback: () => void): this;
   on(event: 'pageError', callback: (params: { error: types.Error }) => void): this;
   on(event: 'popup', callback: (params: { page: PageChannel }) => void): this;
@@ -173,8 +172,11 @@ export type PageInitializer = {
   isClosed: boolean
 };
 
+export type FrameNavigatedEvent = { url: string, name: string, newDocument?: { request?: RequestChannel }, error?: string };
+
 export interface FrameChannel extends Channel {
   on(event: 'loadstate', callback: (params: { add?: types.LifecycleEvent, remove?: types.LifecycleEvent }) => void): this;
+  on(event: 'navigated', callback: (params: FrameNavigatedEvent) => void): this;
 
   evalOnSelector(params: { selector: string; expression: string, isFunction: boolean, arg: any}): Promise<{ value: any }>;
   evalOnSelectorAll(params: { selector: string; expression: string, isFunction: boolean, arg: any}): Promise<{ value: any }>;
@@ -206,7 +208,6 @@ export interface FrameChannel extends Channel {
   type(params: { selector: string, text: string, delay?: number, noWaitAfter?: boolean } & types.TimeoutOptions): Promise<void>;
   uncheck(params: { selector: string, force?: boolean, noWaitAfter?: boolean } & types.TimeoutOptions): Promise<void>;
   waitForFunction(params: { expression: string, isFunction: boolean, arg: any } & types.WaitForFunctionOptions): Promise<{ handle: JSHandleChannel }>;
-  waitForNavigation(params: types.WaitForNavigationOptions): Promise<{ response: ResponseChannel | null }>;
   waitForSelector(params: { selector: string } & types.WaitForElementOptions): Promise<{ element: ElementHandleChannel | null }>;
 }
 export type FrameInitializer = {
@@ -403,7 +404,7 @@ export type ElectronInitializer = {};
 
 export interface ElectronApplicationChannel extends Channel {
   on(event: 'close', callback: () => void): this;
-  on(event: 'window', callback: (params: { page: PageChannel }) => void): this;
+  on(event: 'window', callback: (params: { page: PageChannel, browserWindow: JSHandleChannel }) => void): this;
 
   newBrowserWindow(params: { arg: any }): Promise<{ page: PageChannel }>;
   evaluateExpression(params: { expression: string, isFunction: boolean, arg: any }): Promise<{ value: any }>;

src/rpc/client/electron.ts

@@ -24,6 +24,7 @@ import { ElectronEvents } from '../../server/electron';
 import { TimeoutSettings } from '../../timeoutSettings';
 import { Waiter } from './waiter';
 import { TimeoutError } from '../../errors';
+import { Events } from '../../events';
 
 export class Electron extends ChannelOwner<ElectronChannel, ElectronInitializer> {
   static from(electron: ElectronChannel): Electron {
@@ -53,10 +54,12 @@ export class ElectronApplication extends ChannelOwner<ElectronApplicationChannel
   constructor(parent: ChannelOwner, type: string, guid: string, initializer: ElectronApplicationInitializer) {
     super(parent, type, guid, initializer);
     this._context = BrowserContext.from(initializer.context);
-    this._channel.on('window', ({page}) => {
+    this._channel.on('window', ({ page, browserWindow }) => {
       const window = Page.from(page);
+      (window as any).browserWindow = JSHandle.from(browserWindow);
       this._windows.add(window);
       this.emit(ElectronEvents.ElectronApplication.Window, window);
+      window.once(Events.Page.Close, () => this._windows.delete(window));
     });
     this._channel.on('close', () => {
       this.emit(ElectronEvents.ElectronApplication.Close);

src/rpc/client/frame.ts

@@ -15,9 +15,9 @@
  * limitations under the License.
  */
 
-import { assertMaxArguments } from '../../helper';
+import { assertMaxArguments, helper } from '../../helper';
 import * as types from '../../types';
-import { FrameChannel, FrameInitializer } from '../channels';
+import { FrameChannel, FrameInitializer, FrameNavigatedEvent } from '../channels';
 import { BrowserContext } from './browserContext';
 import { ChannelOwner } from './channelOwner';
 import { ElementHandle, convertSelectOptionValues, convertInputFiles } from './elementHandle';
@@ -75,6 +75,13 @@ export class Frame extends ChannelOwner<FrameChannel, FrameInitializer> {
       if (event.remove)
         this._loadStates.delete(event.remove);
     });
+    this._channel.on('navigated', event => {
+      this._url = event.url;
+      this._name = event.name;
+      this._eventEmitter.emit('navigated', event);
+      if (!event.error && this._page)
+        this._page.emit(Events.Page.FrameNavigated, this);
+    });
   }
 
   private _apiName(method: string) {
@@ -87,9 +94,48 @@ export class Frame extends ChannelOwner<FrameChannel, FrameInitializer> {
     });
   }
 
+  private _setupNavigationWaiter(): Waiter {
+    const waiter = new Waiter();
+    waiter.rejectOnEvent(this._page!, Events.Page.Close, new Error('Navigation failed because page was closed!'));
+    waiter.rejectOnEvent(this._page!, Events.Page.Crash, new Error('Navigation failed because page crashed!'));
+    waiter.rejectOnEvent<Frame>(this._page!, Events.Page.FrameDetached, new Error('Navigating frame was detached!'), frame => frame === this);
+    return waiter;
+  }
+
   async waitForNavigation(options: types.WaitForNavigationOptions = {}): Promise<network.Response | null> {
     return this._wrapApiCall(this._apiName('waitForNavigation'), async () => {
-      return network.Response.fromNullable((await this._channel.waitForNavigation({ ...options })).response);
+      const waitUntil = verifyLoadState(options.waitUntil === undefined ? 'load' : options.waitUntil);
+      const timeout = this._page!._timeoutSettings.navigationTimeout(options);
+      const waiter = this._setupNavigationWaiter();
+      waiter.rejectOnTimeout(timeout, new TimeoutError(`Timeout ${timeout}ms exceeded.`));
+
+      const toUrl = typeof options.url === 'string' ? ` to "${options.url}"` : '';
+      waiter.log(`waiting for navigation${toUrl} until "${waitUntil}"`);
+
+      const navigatedEvent = await waiter.waitForEvent<FrameNavigatedEvent>(this._eventEmitter, 'navigated', event => {
+        // Any failed navigation results in a rejection.
+        if (event.error)
+          return true;
+        waiter.log(`  navigated to "${event.url}"`);
+        return helper.urlMatches(event.url, options.url);
+      });
+      if (navigatedEvent.error) {
+        const e = new Error(navigatedEvent.error);
+        e.stack = '';
+        await waiter.waitForPromise(Promise.reject(e));
+      }
+
+      if (!this._loadStates.has(waitUntil)) {
+        await waiter.waitForEvent<types.LifecycleEvent>(this._eventEmitter, 'loadstate', s => {
+          waiter.log(`  "${s}" event fired`);
+          return s === waitUntil;
+        });
+      }
+
+      const request = navigatedEvent.newDocument ? network.Request.fromNullable(navigatedEvent.newDocument.request || null) : null;
+      const response = request ? await waiter.waitForPromise(request._finalRequest().response()) : null;
+      waiter.dispose();
+      return response;
     });
   }
 
@@ -99,12 +145,12 @@ export class Frame extends ChannelOwner<FrameChannel, FrameInitializer> {
       return;
     return this._wrapApiCall(this._apiName('waitForLoadState'), async () => {
       const timeout = this._page!._timeoutSettings.navigationTimeout(options);
-      const waiter = new Waiter();
-      waiter.rejectOnEvent(this._page!, Events.Page.Close, new Error('Navigation failed because page was closed!'));
-      waiter.rejectOnEvent(this._page!, Events.Page.Crash, new Error('Navigation failed because page crashed!'));
-      waiter.rejectOnEvent<Frame>(this._page!, Events.Page.FrameDetached, new Error('Navigating frame was detached!'), frame => frame === this);
+      const waiter = this._setupNavigationWaiter();
       waiter.rejectOnTimeout(timeout, new TimeoutError(`Timeout ${timeout}ms exceeded.`));
-      await waiter.waitForEvent<types.LifecycleEvent>(this._eventEmitter, 'loadstate', s => s === state);
+      await waiter.waitForEvent<types.LifecycleEvent>(this._eventEmitter, 'loadstate', s => {
+        waiter.log(`  "${s}" event fired`);
+        return s === state;
+      });
       waiter.dispose();
     });
   }

src/rpc/client/network.ts

@@ -132,6 +132,10 @@ export class Request extends ChannelOwner<RequestChannel, RequestInitializer> {
       errorText: this._failureText
     };
   }
+
+  _finalRequest(): Request {
+    return this._redirectedTo ? this._redirectedTo._finalRequest() : this;
+  }
 }
 
 export class Route extends ChannelOwner<RouteChannel, RouteInitializer> {

src/rpc/client/page.ts

@@ -75,6 +75,7 @@ export class Page extends ChannelOwner<PageChannel, PageInitializer> {
 
   constructor(parent: ChannelOwner, type: string, guid: string, initializer: PageInitializer) {
     super(parent, type, guid, initializer);
+    this.setMaxListeners(0);
     this._browserContext = parent as BrowserContext;
     this._timeoutSettings = new TimeoutSettings(this._browserContext._timeoutSettings);
 
@@ -98,7 +99,6 @@ export class Page extends ChannelOwner<PageChannel, PageInitializer> {
     this._channel.on('fileChooser', ({ element, isMultiple }) => this.emit(Events.Page.FileChooser, new FileChooser(this, ElementHandle.from(element), isMultiple)));
     this._channel.on('frameAttached', ({ frame }) => this._onFrameAttached(Frame.from(frame)));
     this._channel.on('frameDetached', ({ frame }) => this._onFrameDetached(Frame.from(frame)));
-    this._channel.on('frameNavigated', ({ frame, url, name }) => this._onFrameNavigated(Frame.from(frame), url, name));
     this._channel.on('load', () => this.emit(Events.Page.Load));
     this._channel.on('pageError', ({ error }) => this.emit(Events.Page.PageError, parseError(error)));
     this._channel.on('popup', ({ page }) => this.emit(Events.Page.Popup, Page.from(page)));
@@ -136,12 +136,6 @@ export class Page extends ChannelOwner<PageChannel, PageInitializer> {
     this.emit(Events.Page.FrameDetached, frame);
   }
 
-  private _onFrameNavigated(frame: Frame, url: string, name: string) {
-    frame._url = url;
-    frame._name = name;
-    this.emit(Events.Page.FrameNavigated, frame);
-  }
-
   private _onRoute(route: Route, request: Request) {
     for (const {url, handler} of this._routes) {
       if (helper.urlMatches(request.url(), url)) {

src/rpc/client/waiter.ts

@@ -15,14 +15,17 @@
  */
 
 import { EventEmitter } from 'events';
+import { rewriteErrorMessage } from '../../utils/stackTrace';
 
 export class Waiter {
   private _dispose: (() => void)[] = [];
   private _failures: Promise<any>[] = [];
+  // TODO: can/should we move these logs into wrapApiCall?
+  private _logs: string[] = [];
 
   async waitForEvent<T = void>(emitter: EventEmitter, event: string, predicate?: (arg: T) => boolean): Promise<T> {
     const { promise, dispose } = waitForEvent(emitter, event, predicate);
-    return this._wait(promise, dispose);
+    return this.waitForPromise(promise, dispose);
   }
 
   rejectOnEvent<T = void>(emitter: EventEmitter, event: string, error: Error, predicate?: (arg: T) => boolean) {
@@ -42,7 +45,7 @@ export class Waiter {
       dispose();
   }
 
-  private async _wait<T>(promise: Promise<T>, dispose?: () => void): Promise<T> {
+  async waitForPromise<T>(promise: Promise<T>, dispose?: () => void): Promise<T> {
     try {
       const result = await Promise.race([promise, ...this._failures]);
       if (dispose)
@@ -52,10 +55,15 @@ export class Waiter {
       if (dispose)
         dispose();
       this.dispose();
+      rewriteErrorMessage(e, e.message + formatLogRecording(this._logs) + kLoggingNote);
       throw e;
     }
   }
 
+  log(s: string) {
+    this._logs.push(s);
+  }
+
   private _rejectOn(promise: Promise<any>, dispose?: () => void) {
     this._failures.push(promise);
     if (dispose)
@@ -89,3 +97,15 @@ function waitForTimeout(timeout: number): { promise: Promise<void>, dispose: ()
   const dispose = () => clearTimeout(timeoutId);
   return { promise, dispose };
 }
+
+const kLoggingNote = `\nNote: use DEBUG=pw:api environment variable and rerun to capture Playwright logs.`;
+
+function formatLogRecording(log: string[]): string {
+  if (!log.length)
+    return '';
+  const header = ` logs `;
+  const headerLength = 60;
+  const leftLength = (headerLength - header.length) / 2;
+  const rightLength = headerLength - header.length - leftLength;
+  return `\n${'='.repeat(leftLength)}${header}${'='.repeat(rightLength)}\n${log.join('\n')}\n${'='.repeat(headerLength)}`;
+}

src/rpc/server/electronDispatcher.ts

@@ -15,11 +15,10 @@
  */
 
 import { Dispatcher, DispatcherScope, lookupDispatcher } from './dispatcher';
-import { Electron, ElectronApplication, ElectronEvents } from '../../server/electron';
+import { Electron, ElectronApplication, ElectronEvents, ElectronPage } from '../../server/electron';
 import { ElectronApplicationChannel, ElectronApplicationInitializer, PageChannel, JSHandleChannel, ElectronInitializer, ElectronChannel, ElectronLaunchOptions } from '../channels';
 import { BrowserContextDispatcher } from './browserContextDispatcher';
 import { BrowserContextBase } from '../../browserContext';
-import { Page } from '../../page';
 import { PageDispatcher } from './pageDispatcher';
 import { parseArgument } from './jsHandleDispatcher';
 import { createHandle } from './elementHandlerDispatcher';
@@ -42,8 +41,11 @@ export class ElectronApplicationDispatcher extends Dispatcher<ElectronApplicatio
     });
 
     electronApplication.on(ElectronEvents.ElectronApplication.Close, () => this._dispatchEvent('close'));
-    electronApplication.on(ElectronEvents.ElectronApplication.Window, (page: Page) => {
-      this._dispatchEvent('window', { page: lookupDispatcher<PageDispatcher>(page) });
+    electronApplication.on(ElectronEvents.ElectronApplication.Window, (page: ElectronPage) => {
+      this._dispatchEvent('window', {
+        page: lookupDispatcher<PageDispatcher>(page),
+        browserWindow: createHandle(this._scope, page.browserWindow),
+      });
     });
   }