fix(webdriver): request redirect chain (#12168)

puppeteer · Apr 2, 2024 · d345055 · d345055
1 parent 6dfb560
commit d345055
Show file tree

Hide file tree

Showing 5 changed files with 40 additions and 43 deletions.
diff --git a/packages/puppeteer-core/src/bidi/Frame.ts b/packages/puppeteer-core/src/bidi/Frame.ts
@@ -402,11 +402,9 @@ export class BidiFrame extends Frame {
           if (!request) {
             return null;
           }
-          const httpRequest = requests.get(request)!;
-          const lastRedirect = httpRequest.redirectChain().at(-1);
-          return (
-            lastRedirect !== undefined ? lastRedirect : httpRequest
-          ).response();
+          const lastRequest = request.lastRedirect ?? request;
+          const httpRequest = requests.get(lastRequest)!;
+          return httpRequest.response();
         }),
         raceWith(
           timeout(ms),

diff --git a/packages/puppeteer-core/src/bidi/HTTPRequest.ts b/packages/puppeteer-core/src/bidi/HTTPRequest.ts
@@ -31,27 +31,32 @@ export const requests = new WeakMap<Request, BidiHTTPRequest>();
 export class BidiHTTPRequest extends HTTPRequest {
   static from(
     bidiRequest: Request,
-    frame: BidiFrame | undefined
+    frame: BidiFrame | undefined,
+    redirect?: BidiHTTPRequest
   ): BidiHTTPRequest {
-    const request = new BidiHTTPRequest(bidiRequest, frame);
+    const request = new BidiHTTPRequest(bidiRequest, frame, redirect);
     request.#initialize();
     return request;
   }
-
-  #redirect: BidiHTTPRequest | undefined;
+  #redirectBy: BidiHTTPRequest | undefined;
   #response: BidiHTTPResponse | null = null;
   override readonly id: string;
   readonly #frame: BidiFrame | undefined;
   readonly #request: Request;
 
-  private constructor(request: Request, frame: BidiFrame | undefined) {
+  private constructor(
+    request: Request,
+    frame: BidiFrame | undefined,
+    redirectBy?: BidiHTTPRequest
+  ) {
     super();
     requests.set(request, this);
 
     this.interception.enabled = request.isBlocked;
 
     this.#request = request;
     this.#frame = frame;
+    this.#redirectBy = redirectBy;
     this.id = request.id;
   }
 
@@ -61,8 +66,8 @@ export class BidiHTTPRequest extends HTTPRequest {
 
   #initialize() {
     this.#request.on('redirect', request => {
-      this.#redirect = BidiHTTPRequest.from(request, this.#frame);
-      void this.#redirect.finalizeInterceptions();
+      const httpRequest = BidiHTTPRequest.from(request, this.#frame, this);
+      void httpRequest.finalizeInterceptions();
     });
     this.#request.once('success', data => {
       this.#response = BidiHTTPResponse.from(data, this);
@@ -123,13 +128,13 @@ export class BidiHTTPRequest extends HTTPRequest {
   }
 
   override redirectChain(): BidiHTTPRequest[] {
-    if (this.#redirect === undefined) {
+    if (this.#redirectBy === undefined) {
       return [];
     }
-    const redirects = [this.#redirect];
+    const redirects = [this.#redirectBy];
     for (const redirect of redirects) {
-      if (redirect.#redirect !== undefined) {
-        redirects.push(redirect.#redirect);
+      if (redirect.#redirectBy !== undefined) {
+        redirects.push(redirect.#redirectBy);
       }
     }
     return redirects;

diff --git a/packages/puppeteer-core/src/bidi/core/Navigation.ts b/packages/puppeteer-core/src/bidi/core/Navigation.ts
@@ -6,7 +6,6 @@
 
 import {EventEmitter} from '../../common/EventEmitter.js';
 import {inertIfDisposed} from '../../util/decorators.js';
-import {Deferred} from '../../util/Deferred.js';
 import {DisposableStack, disposeSymbol} from '../../util/disposable.js';
 
 import type {BrowsingContext} from './BrowsingContext.js';
@@ -44,7 +43,7 @@ export class Navigation extends EventEmitter<{
   #navigation: Navigation | undefined;
   readonly #browsingContext: BrowsingContext;
   readonly #disposables = new DisposableStack();
-  readonly #id = new Deferred<string | null>();
+  #id?: string | null;
   // keep-sorted end
 
   private constructor(context: BrowsingContext) {
@@ -69,7 +68,6 @@ export class Navigation extends EventEmitter<{
     browsingContextEmitter.on('request', ({request}) => {
       if (
         request.navigation === undefined ||
-        this.#request !== undefined ||
         // If a request with a navigation ID comes in, then the navigation ID is
         // for this navigation.
         !this.#matches(request.navigation)
@@ -79,6 +77,13 @@ export class Navigation extends EventEmitter<{
 
       this.#request = request;
       this.emit('request', request);
+      const requestEmitter = this.#disposables.use(
+        new EventEmitter(this.#request)
+      );
+
+      requestEmitter.on('redirect', request => {
+        this.#request = request;
+      });
     });
 
     const sessionEmitter = this.#disposables.use(
@@ -139,11 +144,11 @@ export class Navigation extends EventEmitter<{
     if (this.#navigation !== undefined && !this.#navigation.disposed) {
       return false;
     }
-    if (!this.#id.resolved()) {
-      this.#id.resolve(navigation);
+    if (this.#id === undefined) {
+      this.#id = navigation;
       return true;
     }
-    return this.#id.value() === navigation;
+    return this.#id === navigation;
   }
 
   // keep-sorted start block=yes

diff --git a/packages/puppeteer-core/src/bidi/core/Request.ts b/packages/puppeteer-core/src/bidi/core/Request.ts
@@ -135,6 +135,16 @@ export class Request extends EventEmitter<{
   get redirect(): Request | undefined {
     return this.#redirect;
   }
+  get lastRedirect(): Request | undefined {
+    let redirect = this.#redirect;
+    while (redirect) {
+      if (redirect && !redirect.#redirect) {
+        return redirect;
+      }
+      redirect = redirect.#redirect;
+    }
+    return redirect;
+  }
   get response(): Bidi.Network.ResponseData | undefined {
     return this.#response;
   }

diff --git a/test/TestExpectations.json b/test/TestExpectations.json
@@ -833,13 +833,6 @@
     "expectations": ["SKIP"],
     "comment": "TODO: Needs full support for continueResponse in Firefox https://bugzilla.mozilla.org/show_bug.cgi?id=1853887"
   },
-  {
-    "testIdPattern": "[requestinterception-experimental.spec] cooperative request interception Request.respond should redirect",
-    "platforms": ["darwin", "linux", "win32"],
-    "parameters": ["webDriverBiDi"],
-    "expectations": ["FAIL"],
-    "comment": "Puppeteer request.redirectChain issue"
-  },
   {
     "testIdPattern": "[requestinterception.spec] *",
     "platforms": ["darwin", "linux", "win32"],
@@ -3557,13 +3550,6 @@
     "parameters": ["firefox", "webDriverBiDi"],
     "expectations": ["PASS"]
   },
-  {
-    "testIdPattern": "[requestinterception-experimental.spec] cooperative request interception Request.respond should redirect",
-    "platforms": ["darwin", "linux", "win32"],
-    "parameters": ["firefox", "webDriverBiDi"],
-    "expectations": ["SKIP"],
-    "comment": "Firefox does not support headers in provideResponse"
-  },
   {
     "testIdPattern": "[requestinterception-experimental.spec] request interception \"after each\" hook in \"request interception\"",
     "platforms": ["win32"],
@@ -3745,13 +3731,6 @@
     "parameters": ["firefox", "webDriverBiDi"],
     "expectations": ["PASS"]
   },
-  {
-    "testIdPattern": "[requestinterception.spec] request interception Request.respond should redirect",
-    "platforms": ["darwin", "linux", "win32"],
-    "parameters": ["chrome", "webDriverBiDi"],
-    "expectations": ["FAIL"],
-    "comment": "Puppeteer request.redirectChain issue"
-  },
   {
     "testIdPattern": "[screenshot.spec] Screenshots Cdp should use scale for clip",
     "platforms": ["darwin", "linux", "win32"],