From bfcd65d2f7b6008cd00b8577de9fa2d3f18a2653 Mon Sep 17 00:00:00 2001
From: jrandolf <101637635+jrandolf@users.noreply.github.com>
Date: Thu, 8 Feb 2024 15:22:17 +0100
Subject: [PATCH] chore: fix `bidi/core` navigation (#11878)

---
 .../src/bidi/core/BrowsingContext.ts          | 27 ++++---
 .../src/bidi/core/Navigation.ts               | 75 +++++++++++++------
 2 files changed, 65 insertions(+), 37 deletions(-)

diff --git a/packages/puppeteer-core/src/bidi/core/BrowsingContext.ts b/packages/puppeteer-core/src/bidi/core/BrowsingContext.ts
index dbf4eee94ec..e8bd93433be 100644
--- a/packages/puppeteer-core/src/bidi/core/BrowsingContext.ts
+++ b/packages/puppeteer-core/src/bidi/core/BrowsingContext.ts
@@ -210,7 +210,16 @@ export class BrowsingContext extends EventEmitter<{
       }
       this.#url = info.url;
 
-      this.#requests.clear();
+      for (const [id, request] of this.#requests) {
+        if (request.disposed) {
+          this.#requests.delete(id);
+        }
+      }
+      // If the navigation hasn't finished, then this is nested navigation. The
+      // current navigation will handle this.
+      if (this.#navigation !== undefined && !this.#navigation.disposed) {
+        return;
+      }
 
       // Note the navigation ID is null for this event.
       this.#navigation = Navigation.from(this);
@@ -232,10 +241,10 @@ export class BrowsingContext extends EventEmitter<{
       if (event.context !== this.id) {
         return;
       }
+      // Means the request is a redirect. This is handled in Request.
       if (this.#requests.has(event.request.request)) {
         return;
       }
-
       const request = Request.from(this, event);
       this.#requests.set(request.id, request);
       this.emit('request', {request});
@@ -353,33 +362,23 @@ export class BrowsingContext extends EventEmitter<{
   async navigate(
     url: string,
     wait?: Bidi.BrowsingContext.ReadinessState
-  ): Promise<Navigation> {
+  ): Promise<void> {
     await this.#session.send('browsingContext.navigate', {
       context: this.id,
       url,
       wait,
     });
-    return await new Promise(resolve => {
-      this.once('navigation', ({navigation}) => {
-        resolve(navigation);
-      });
-    });
   }
 
   @throwIfDisposed<BrowsingContext>(context => {
     // SAFETY: Disposal implies this exists.
     return context.#reason!;
   })
-  async reload(options: ReloadOptions = {}): Promise<Navigation> {
+  async reload(options: ReloadOptions = {}): Promise<void> {
     await this.#session.send('browsingContext.reload', {
       context: this.id,
       ...options,
     });
-    return await new Promise(resolve => {
-      this.once('navigation', ({navigation}) => {
-        resolve(navigation);
-      });
-    });
   }
 
   @throwIfDisposed<BrowsingContext>(context => {
diff --git a/packages/puppeteer-core/src/bidi/core/Navigation.ts b/packages/puppeteer-core/src/bidi/core/Navigation.ts
index a7efbfeb2c0..50040164a5d 100644
--- a/packages/puppeteer-core/src/bidi/core/Navigation.ts
+++ b/packages/puppeteer-core/src/bidi/core/Navigation.ts
@@ -41,9 +41,10 @@ export class Navigation extends EventEmitter<{
 
   // keep-sorted start
   #request: Request | undefined;
+  #navigation: Navigation | undefined;
   readonly #browsingContext: BrowsingContext;
   readonly #disposables = new DisposableStack();
-  readonly #id = new Deferred<string>();
+  readonly #id = new Deferred<string | null>();
   // keep-sorted end
 
   private constructor(context: BrowsingContext) {
@@ -65,31 +66,48 @@ export class Navigation extends EventEmitter<{
       this.dispose();
     });
 
-    this.#browsingContext.on('request', ({request}) => {
-      if (request.navigation === this.#id.value()) {
-        this.#request = request;
-        this.emit('request', request);
+    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)
+      ) {
+        return;
       }
+
+      this.#request = request;
+      this.emit('request', request);
     });
 
     const sessionEmitter = this.#disposables.use(
       new EventEmitter(this.#session)
     );
-    // To get the navigation ID if any.
+    sessionEmitter.on('browsingContext.navigationStarted', info => {
+      if (
+        info.context !== this.#browsingContext.id ||
+        this.#navigation !== undefined
+      ) {
+        return;
+      }
+      this.#navigation = Navigation.from(this.#browsingContext);
+    });
+
     for (const eventName of [
       'browsingContext.domContentLoaded',
       'browsingContext.load',
     ] as const) {
       sessionEmitter.on(eventName, info => {
-        if (info.context !== this.#browsingContext.id) {
+        if (
+          info.context !== this.#browsingContext.id ||
+          info.navigation === null ||
+          !this.#matches(info.navigation)
+        ) {
           return;
         }
-        if (!info.navigation) {
-          return;
-        }
-        if (!this.#id.resolved()) {
-          this.#id.resolve(info.navigation);
-        }
+
+        this.dispose();
       });
     }
 
@@ -99,18 +117,15 @@ export class Navigation extends EventEmitter<{
       ['browsingContext.navigationAborted', 'aborted'],
     ] as const) {
       sessionEmitter.on(eventName, info => {
-        if (info.context !== this.#browsingContext.id) {
-          return;
-        }
-        if (!info.navigation) {
-          return;
-        }
-        if (!this.#id.resolved()) {
-          this.#id.resolve(info.navigation);
-        }
-        if (this.#id.value() !== info.navigation) {
+        if (
+          info.context !== this.#browsingContext.id ||
+          // Note we don't check if `navigation` is null since `null` means the
+          // fragment navigated.
+          !this.#matches(info.navigation)
+        ) {
           return;
         }
+
         this.emit(event, {
           url: info.url,
           timestamp: new Date(info.timestamp),
@@ -120,6 +135,17 @@ export class Navigation extends EventEmitter<{
     }
   }
 
+  #matches(navigation: string | null): boolean {
+    if (this.#navigation !== undefined && !this.#navigation.disposed) {
+      return false;
+    }
+    if (!this.#id.resolved()) {
+      this.#id.resolve(navigation);
+      return true;
+    }
+    return this.#id.value() === navigation;
+  }
+
   // keep-sorted start block=yes
   get #session() {
     return this.#browsingContext.userContext.browser.session;
@@ -130,6 +156,9 @@ export class Navigation extends EventEmitter<{
   get request(): Request | undefined {
     return this.#request;
   }
+  get navigation(): Navigation | undefined {
+    return this.#navigation;
+  }
   // keep-sorted end
 
   @inertIfDisposed