fix(har): internal redirect in renderer-initiated navigations (#15000)

fix(har): internal redirect in renderer-initiated navigations

Author: dgozman

packages/playwright-core/src/client/harRouter.ts

@@ -56,7 +56,7 @@ export class HarRouter {
 
     if (response.action === 'redirect') {
       debugLogger.log('api', `HAR: ${route.request().url()} redirected to ${response.redirectURL}`);
-      await route._abort(undefined, response.redirectURL);
+      await route._redirectNavigationRequest(response.redirectURL!);
       return;
     }
 

packages/playwright-core/src/client/network.ts

@@ -282,12 +282,14 @@ export class Route extends ChannelOwner<channels.RouteChannel> implements api.Ro
   }
 
   async abort(errorCode?: string) {
-    await this._abort(errorCode);
+    this._checkNotHandled();
+    await this._raceWithPageClose(this._channel.abort({ errorCode }));
+    this._reportHandled(true);
   }
 
-  async _abort(errorCode?: string, redirectAbortedNavigationToUrl?: string) {
+  async _redirectNavigationRequest(url: string) {
     this._checkNotHandled();
-    await this._raceWithPageClose(this._channel.abort({ errorCode, redirectAbortedNavigationToUrl }));
+    await this._raceWithPageClose(this._channel.redirectNavigationRequest({ url }));
     this._reportHandled(true);
   }
 

packages/playwright-core/src/protocol/channels.ts

@@ -3158,17 +3158,23 @@ export interface RouteEventTarget {
 }
 export interface RouteChannel extends RouteEventTarget, Channel {
   _type_Route: boolean;
+  redirectNavigationRequest(params: RouteRedirectNavigationRequestParams, metadata?: Metadata): Promise<RouteRedirectNavigationRequestResult>;
   abort(params: RouteAbortParams, metadata?: Metadata): Promise<RouteAbortResult>;
   continue(params: RouteContinueParams, metadata?: Metadata): Promise<RouteContinueResult>;
   fulfill(params: RouteFulfillParams, metadata?: Metadata): Promise<RouteFulfillResult>;
 }
+export type RouteRedirectNavigationRequestParams = {
+  url: string,
+};
+export type RouteRedirectNavigationRequestOptions = {
+
+};
+export type RouteRedirectNavigationRequestResult = void;
 export type RouteAbortParams = {
   errorCode?: string,
-  redirectAbortedNavigationToUrl?: string,
 };
 export type RouteAbortOptions = {
   errorCode?: string,
-  redirectAbortedNavigationToUrl?: string,
 };
 export type RouteAbortResult = void;
 export type RouteContinueParams = {

packages/playwright-core/src/protocol/protocol.yml

@@ -2491,10 +2491,13 @@ Route:
 
   commands:
 
+    redirectNavigationRequest:
+      parameters:
+        url: string
+
     abort:
       parameters:
         errorCode: string?
-        redirectAbortedNavigationToUrl: string?
 
     continue:
       parameters:

packages/playwright-core/src/protocol/validator.ts

@@ -1181,9 +1181,11 @@ export function createScheme(tChannel: (name: string) => Validator): Scheme {
   });
   scheme.RequestResponseParams = tOptional(tObject({}));
   scheme.RequestRawRequestHeadersParams = tOptional(tObject({}));
+  scheme.RouteRedirectNavigationRequestParams = tObject({
+    url: tString,
+  });
   scheme.RouteAbortParams = tObject({
     errorCode: tOptional(tString),
-    redirectAbortedNavigationToUrl: tOptional(tString),
   });
   scheme.RouteContinueParams = tObject({
     url: tOptional(tString),

packages/playwright-core/src/server/dispatchers/networkDispatchers.ts

@@ -135,7 +135,11 @@ export class RouteDispatcher extends Dispatcher<Route, channels.RouteChannel> im
   }
 
   async abort(params: channels.RouteAbortParams): Promise<void> {
-    await this._object.abort(params.errorCode || 'failed', params.redirectAbortedNavigationToUrl);
+    await this._object.abort(params.errorCode || 'failed');
+  }
+
+  async redirectNavigationRequest(params: channels.RouteRedirectNavigationRequestParams): Promise<void> {
+    await this._object.redirectNavigationRequest(params.url);
   }
 }
 

packages/playwright-core/src/server/frames.ts

@@ -269,7 +269,7 @@ export class FrameManager {
       name: frame._name,
       newDocument: frame.pendingDocument(),
       error: new NavigationAbortedError(documentId, errorText),
-      isPublic: !frame._pendingNavigationRedirectAfterAbort
+      isPublic: !(documentId && frame._redirectedNavigations.has(documentId)),
     };
     frame.setPendingDocument(undefined);
     frame.emit(Frame.Events.InternalNavigation, navigationEvent);
@@ -467,7 +467,7 @@ export class Frame extends SdkObject {
   readonly _detachedPromise: Promise<void>;
   private _detachedCallback = () => {};
   private _raceAgainstEvaluationStallingEventsPromises = new Set<ManualPromise<any>>();
-  _pendingNavigationRedirectAfterAbort: { url: string, documentId: string } | undefined;
+  readonly _redirectedNavigations = new Map<string, { url: string, gotoPromise: Promise<network.Response | null> }>(); // documentId -> data
 
   constructor(page: Page, id: string, parentFrame: Frame | null) {
     super(page, 'frame');
@@ -604,21 +604,26 @@ export class Frame extends SdkObject {
       this._page._crashedPromise.then(() => { throw new Error('Navigation failed because page crashed!'); }),
       this._detachedPromise.then(() => { throw new Error('Navigating frame was detached!'); }),
       action().catch(e => {
-        if (this._pendingNavigationRedirectAfterAbort && e instanceof NavigationAbortedError) {
-          const { url, documentId } = this._pendingNavigationRedirectAfterAbort;
-          this._pendingNavigationRedirectAfterAbort = undefined;
-          if (e.documentId === documentId) {
-            progress.log(`redirecting navigation to "${url}"`);
-            return this._gotoAction(progress, url, options);
+        if (e instanceof NavigationAbortedError && e.documentId) {
+          const data = this._redirectedNavigations.get(e.documentId);
+          if (data) {
+            progress.log(`waiting for redirected navigation to "${data.url}"`);
+            return data.gotoPromise;
           }
         }
         throw e;
       }),
     ]);
   }
 
-  redirectNavigationAfterAbort(url: string, documentId: string) {
-    this._pendingNavigationRedirectAfterAbort = { url, documentId };
+  redirectNavigation(url: string, documentId: string, referer: string | undefined) {
+    const controller = new ProgressController(serverSideCallMetadata(), this);
+    const data = {
+      url,
+      gotoPromise: controller.run(progress => this._gotoAction(progress, url, { referer }), 0),
+    };
+    this._redirectedNavigations.set(documentId, data);
+    data.gotoPromise.finally(() => this._redirectedNavigations.delete(documentId));
   }
 
   async goto(metadata: CallMetadata, url: string, options: types.GotoOptions = {}): Promise<network.Response | null> {
@@ -659,7 +664,7 @@ export class Frame extends SdkObject {
       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');
+        throw new NavigationAbortedError(navigateResult.newDocumentId, 'Navigation interrupted by another one');
       }
       if (event.error)
         throw event.error;

packages/playwright-core/src/server/network.ts

@@ -244,13 +244,17 @@ export class Route extends SdkObject {
     return this._request;
   }
 
-  async abort(errorCode: string = 'failed', redirectAbortedNavigationToUrl?: string) {
+  async abort(errorCode: string = 'failed') {
     this._startHandling();
-    if (redirectAbortedNavigationToUrl)
-      this._request.frame().redirectNavigationAfterAbort(redirectAbortedNavigationToUrl, this._request._documentId!);
     await this._delegate.abort(errorCode);
   }
 
+  async redirectNavigationRequest(url: string) {
+    this._startHandling();
+    assert(this._request.isNavigationRequest());
+    this._request.frame().redirectNavigation(url, this._request._documentId!, this._request.headerValue('referer'));
+  }
+
   async fulfill(overrides: channels.RouteFulfillParams) {
     this._startHandling();
     let body = overrides.body;

tests/library/browsercontext-har.spec.ts

@@ -115,13 +115,31 @@ it('should change document URL after redirected navigation', async ({ contextFac
   const page = await context.newPage();
   const [response] = await Promise.all([
     page.waitForNavigation(),
+    page.waitForURL('https://www.theverge.com/'),
     page.goto('https://theverge.com/')
   ]);
   await expect(page).toHaveURL('https://www.theverge.com/');
   expect(response.request().url()).toBe('https://www.theverge.com/');
   expect(await page.evaluate(() => location.href)).toBe('https://www.theverge.com/');
 });
 
+it('should change document URL after redirected navigation on click', async ({ server, contextFactory, isAndroid, asset }) => {
+  it.fixme(isAndroid);
+
+  const path = asset('har-redirect.har');
+  const context = await contextFactory({ har: { path, urlFilter: /.*theverge.*/ } });
+  const page = await context.newPage();
+  await page.goto(server.EMPTY_PAGE);
+  await page.setContent(`<a href="https://theverge.com/">click me</a>`);
+  const [response] = await Promise.all([
+    page.waitForNavigation(),
+    page.click('text=click me'),
+  ]);
+  await expect(page).toHaveURL('https://www.theverge.com/');
+  expect(response.request().url()).toBe('https://www.theverge.com/');
+  expect(await page.evaluate(() => location.href)).toBe('https://www.theverge.com/');
+});
+
 it('should goBack to redirected navigation', async ({ contextFactory, isAndroid, asset, server }) => {
   it.fixme(isAndroid);