fix: navigation with wait None (#2557)

Do not wait for the CDP command in case `BrowsingContext.navigate` with
`wait: None`. Provide `pendingNavigationId` to consistent navigation ID.

---------

Co-authored-by: browser-automation-bot <133232582+browser-automation-bot@users.noreply.github.com>

src/bidiMapper/modules/context/BrowsingContextImpl.ts

@@ -87,7 +87,14 @@ export class BrowsingContextImpl {
   // on a deprecated `Page.frameScheduledNavigation` event. The latest is required as the
   // `Page.frameRequestedNavigation` event is not emitted for same-document navigations.
   #pendingNavigationUrl: string | undefined;
-  #virtualNavigationId: string = uuidv4();
+  // Navigation ID is required, as CDP `loaderId` cannot be mapped 1:1 to all the
+  // navigations (e.g. same document navigations). Updated after each navigation,
+  // including same-document ones.
+  #navigationId: string = uuidv4();
+  // When a new navigation is started via `BrowsingContext.navigate` with `wait` set to
+  // `None`, the command result should have `navigation` value, but mapper does not have
+  // it yet. This value will be set to `navigationId` after next .
+  #pendingNavigationId: string | undefined;
 
   #originalOpener?: string;
 
@@ -200,13 +207,8 @@ export class BrowsingContextImpl {
     return this.#loaderId;
   }
 
-  /**
-   * Virtual navigation ID. Required, as CDP `loaderId` cannot be mapped 1:1 to all the
-   * navigations (e.g. same document navigations). Updated after each navigation,
-   * including same-document ones.
-   */
-  get virtualNavigationId(): string {
-    return this.#virtualNavigationId;
+  get navigationId(): string {
+    return this.#navigationId;
   }
 
   dispose() {
@@ -416,7 +418,7 @@ export class BrowsingContextImpl {
           method: ChromiumBidi.BrowsingContext.EventNames.FragmentNavigated,
           params: {
             context: this.id,
-            navigation: this.#virtualNavigationId,
+            navigation: this.#navigationId,
             timestamp,
             url: this.#url,
           },
@@ -429,15 +431,17 @@ export class BrowsingContextImpl {
       if (this.id !== params.frameId) {
         return;
       }
-      // Generate a new virtual navigation id.
-      this.#virtualNavigationId = uuidv4();
+      // Use `pendingNavigationId` if navigation initiated by BiDi
+      // `BrowsingContext.navigate` or generate a new navigation id.
+      this.#navigationId = this.#pendingNavigationId ?? uuidv4();
+      this.#pendingNavigationId = undefined;
       this.#eventManager.registerEvent(
         {
           type: 'event',
           method: ChromiumBidi.BrowsingContext.EventNames.NavigationStarted,
           params: {
             context: this.id,
-            navigation: this.#virtualNavigationId,
+            navigation: this.#navigationId,
             timestamp: BrowsingContextImpl.getTimestamp(),
             // The URL of the navigation that is currently in progress. Although the URL
             // is not yet known in case of user-initiated navigations, it is possible to
@@ -502,7 +506,7 @@ export class BrowsingContextImpl {
               method: ChromiumBidi.BrowsingContext.EventNames.DomContentLoaded,
               params: {
                 context: this.id,
-                navigation: this.#virtualNavigationId,
+                navigation: this.#navigationId,
                 timestamp,
                 url: this.#url,
               },
@@ -519,7 +523,7 @@ export class BrowsingContextImpl {
               method: ChromiumBidi.BrowsingContext.EventNames.Load,
               params: {
                 context: this.id,
-                navigation: this.#virtualNavigationId,
+                navigation: this.#navigationId,
                 timestamp,
                 url: this.#url,
               },
@@ -815,40 +819,55 @@ export class BrowsingContextImpl {
     //  `Page.frameRequestedNavigation` can be used for this purpose.
     this.#pendingNavigationUrl = url;
 
-    // TODO: handle loading errors.
-    const cdpNavigateResult = await this.#cdpTarget.cdpClient.sendCommand(
-      'Page.navigate',
-      {
-        url,
-        frameId: this.id,
-      }
-    );
+    const navigationId = uuidv4();
+    this.#pendingNavigationId = navigationId;
 
-    if (cdpNavigateResult.errorText) {
-      // If navigation failed, no pending navigation is left.
-      this.#pendingNavigationUrl = undefined;
-      this.#eventManager.registerEvent(
+    // Navigate and wait for the result. If the navigation fails, the error event is
+    // emitted and the promise is rejected.
+    const cdpNavigatePromise = (async () => {
+      const cdpNavigateResult = await this.#cdpTarget.cdpClient.sendCommand(
+        'Page.navigate',
         {
-          type: 'event',
-          method: ChromiumBidi.BrowsingContext.EventNames.NavigationFailed,
-          params: {
-            context: this.id,
-            navigation: this.#virtualNavigationId,
-            timestamp: BrowsingContextImpl.getTimestamp(),
-            url,
-          },
-        },
-        this.id
+          url,
+          frameId: this.id,
+        }
       );
 
-      throw new UnknownErrorException(cdpNavigateResult.errorText);
+      if (cdpNavigateResult.errorText) {
+        // If navigation failed, no pending navigation is left.
+        this.#pendingNavigationUrl = undefined;
+        this.#eventManager.registerEvent(
+          {
+            type: 'event',
+            method: ChromiumBidi.BrowsingContext.EventNames.NavigationFailed,
+            params: {
+              context: this.id,
+              navigation: navigationId,
+              timestamp: BrowsingContextImpl.getTimestamp(),
+              url,
+            },
+          },
+          this.id
+        );
+
+        throw new UnknownErrorException(cdpNavigateResult.errorText);
+      }
+
+      this.#documentChanged(cdpNavigateResult.loaderId);
+      return cdpNavigateResult;
+    })();
+
+    if (wait === BrowsingContext.ReadinessState.None) {
+      // Do not wait for the result of the navigation promise.
+      return {
+        navigation: navigationId,
+        url,
+      };
     }
 
-    this.#documentChanged(cdpNavigateResult.loaderId);
+    const cdpNavigateResult = await cdpNavigatePromise;
 
     switch (wait) {
-      case BrowsingContext.ReadinessState.None:
-        break;
       case BrowsingContext.ReadinessState.Interactive:
         // No `loaderId` means same-document navigation.
         if (cdpNavigateResult.loaderId === undefined) {
@@ -868,9 +887,9 @@ export class BrowsingContextImpl {
     }
 
     return {
-      navigation: this.#virtualNavigationId,
+      navigation: navigationId,
       // Url can change due to redirect get the latest one.
-      url: wait === BrowsingContext.ReadinessState.None ? url : this.#url,
+      url: this.#url,
     };
   }
 
@@ -898,7 +917,7 @@ export class BrowsingContextImpl {
     }
 
     return {
-      navigation: this.#virtualNavigationId,
+      navigation: this.#navigationId,
       url: this.url,
     };
   }

src/bidiMapper/modules/network/NetworkRequest.ts

@@ -202,9 +202,7 @@ export class NetworkRequest {
     }
 
     // Get virtual navigation ID from the browsing context.
-    return this.#networkStorage.getVirtualNavigationId(
-      this.#context ?? undefined
-    );
+    return this.#networkStorage.getNavigationId(this.#context ?? undefined);
   }
 
   get #cookies() {

src/bidiMapper/modules/network/NetworkStorage.ts

@@ -320,13 +320,12 @@ export class NetworkStorage {
   /**
    * Gets the virtual navigation ID for the given navigable ID.
    */
-  getVirtualNavigationId(contextId: string | undefined): string | null {
+  getNavigationId(contextId: string | undefined): string | null {
     if (contextId === undefined) {
       return null;
     }
     return (
-      this.#browsingContextStorage.findContext(contextId)
-        ?.virtualNavigationId ?? null
+      this.#browsingContextStorage.findContext(contextId)?.navigationId ?? null
     );
   }
 }

tests/browsing_context/test_navigate.py

@@ -542,30 +542,31 @@ async def test_browsingContext_navigationStarted_sameDocumentNavigation(
                 "wait": "none"
             }
         })
-    # Assert that the navigation started event was received.
+
+    # Assert that the navigation command was finished.
+    response = await read_JSON_message(websocket)
+    assert response == AnyExtending({
+        'id': command_id,
+        'result': {
+            'navigation': ANY_UUID,
+            'url': url_base
+        }
+    })
+    navigation_id = response["result"]["navigation"]
+
+    # Assert that the navigation started event was received with the correct
+    # navigation id.
     response = await read_JSON_message(websocket)
     assert response == {
         'type': 'event',
         "method": "browsingContext.navigationStarted",
         "params": {
             "context": context_id,
-            "navigation": ANY_UUID,
+            "navigation": navigation_id,
             "timestamp": ANY_TIMESTAMP,
             "url": url_base,
         }
     }
-    navigation_id = response["params"]["navigation"]
-
-    # Assert that the navigation command was finished with the correct
-    # navigation id.
-    response = await read_JSON_message(websocket)
-    assert response == AnyExtending({
-        'id': command_id,
-        'result': {
-            'navigation': navigation_id,
-            'url': url_base
-        }
-    })
 
     # Assert that the page is loaded.
     response = await read_JSON_message(websocket)

wpt-metadata/chromedriver/headful/webdriver/tests/bidi/browsing_context/navigation_failed/navigation_failed.py.ini

@@ -14,9 +14,6 @@
   [test_with_x_frame_options_header[DENY\]]
     expected: FAIL
 
-  [test_with_new_navigation]
-    expected: FAIL
-
   [test_with_new_navigation_inside_page]
     expected: FAIL
 

wpt-metadata/chromedriver/headful/webdriver/tests/bidi/browsing_context/navigation_started/navigation_started.py.ini

@@ -1,11 +1,4 @@
 [navigation_started.py]
-  expected: TIMEOUT
-  [test_iframe]
-    expected: FAIL
-
-  [test_nested_iframes]
-    expected: FAIL
-
   [test_new_context[tab\]]
     expected: FAIL
 

wpt-metadata/chromedriver/headless/webdriver/tests/bidi/browsing_context/navigation_failed/navigation_failed.py.ini

@@ -14,9 +14,6 @@
   [test_with_x_frame_options_header[DENY\]]
     expected: FAIL
 
-  [test_with_new_navigation]
-    expected: FAIL
-
   [test_with_new_navigation_inside_page]
     expected: FAIL
 

wpt-metadata/chromedriver/headless/webdriver/tests/bidi/browsing_context/navigation_started/navigation_started.py.ini

@@ -1,11 +1,4 @@
 [navigation_started.py]
-  expected: TIMEOUT
-  [test_iframe]
-    expected: FAIL
-
-  [test_nested_iframes]
-    expected: FAIL
-
   [test_new_context[tab\]]
     expected: FAIL
 

wpt-metadata/mapper/headless/webdriver/tests/bidi/browsing_context/navigation_failed/navigation_failed.py.ini

@@ -14,9 +14,6 @@
   [test_with_x_frame_options_header[DENY\]]
     expected: FAIL
 
-  [test_with_new_navigation]
-    expected: FAIL
-
   [test_with_new_navigation_inside_page]
     expected: FAIL
 

wpt-metadata/mapper/headless/webdriver/tests/bidi/browsing_context/navigation_started/navigation_started.py.ini

@@ -1,11 +1,4 @@
 [navigation_started.py]
-  expected: TIMEOUT
-  [test_iframe]
-    expected: FAIL
-
-  [test_nested_iframes]
-    expected: FAIL
-
   [test_new_context[tab\]]
     expected: FAIL