fix: provide navigation and url in events (#2264)

Address 2 issues: * Lack of `navigation` in `browsingContext.navigationStarted` event. CDP does not always provide `loaderId` for same document navigation, while WebDriver BiDi spec requires a unique navigation ID for each navigation. To address the issue, I introduced A virtual navigation id, which is returned instead of loader ID in browsing context and network events. * Lack of `url` in `browsingContext.navigationStarted`. The information can be received from deprecated `Page.frameScheduledNavigation` CDP event. --------- Signed-off-by: Browser Automation Bot <[email protected]> Co-authored-by: Browser Automation Bot <[email protected]>
GoogleChromeLabs · Jun 24, 2024 · 9f058da · 9f058da
1 parent bfd31a6
commit 9f058da
Show file tree

Hide file tree

Showing 20 changed files with 205 additions and 172 deletions.
diff --git a/src/bidiMapper/BidiServer.ts b/src/bidiMapper/BidiServer.ts
@@ -94,6 +94,7 @@ export class BidiServer extends EventEmitter<BidiServerEvent> {
     this.#eventManager = new EventManager(this.#browsingContextStorage);
     const networkStorage = new NetworkStorage(
       this.#eventManager,
+      this.#browsingContextStorage,
       browserCdpClient,
       logger
     );

diff --git a/src/bidiMapper/modules/context/BrowsingContextImpl.ts b/src/bidiMapper/modules/context/BrowsingContextImpl.ts
@@ -33,6 +33,7 @@ import {assert} from '../../../utils/assert.js';
 import {Deferred} from '../../../utils/Deferred.js';
 import {type LoggerFn, LogType} from '../../../utils/log.js';
 import {inchesFromCm} from '../../../utils/unitConversions.js';
+import {uuidv4} from '../../../utils/uuid';
 import type {CdpTarget} from '../cdp/CdpTarget.js';
 import type {Realm} from '../script/Realm.js';
 import type {RealmStorage} from '../script/RealmStorage.js';
@@ -78,6 +79,12 @@ export class BrowsingContextImpl {
   readonly #logger?: LoggerFn;
   // Keeps track of the previously set viewport.
   #previousViewport: {width: number; height: number} = {width: 0, height: 0};
+  // The URL of the navigation that is currently in progress. A workaround of the CDP
+  // lacking URL for the pending navigation events, e.g. `Page.frameStartedLoading`.
+  // Set on `Page.navigate`, `Page.reload` commands and on deprecated CDP event
+  // `Page.frameScheduledNavigation`.
+  #pendingNavigationUrl: string | undefined;
+  #virtualNavigationId: string = uuidv4();
 
   #originalOpener?: string;
 
@@ -165,6 +172,15 @@ 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;
+  }
+
   dispose() {
     this.#deleteAllChildren();
 
@@ -333,6 +349,7 @@ export class BrowsingContextImpl {
         return;
       }
       this.#url = params.frame.url + (params.frame.urlFragment ?? '');
+      this.#pendingNavigationUrl = undefined;
 
       // At the point the page is initialized, all the nested iframes from the
       // previous page are detached and realms are destroyed.
@@ -344,6 +361,7 @@ export class BrowsingContextImpl {
       if (this.id !== params.frameId) {
         return;
       }
+      this.#pendingNavigationUrl = undefined;
       const timestamp = BrowsingContextImpl.getTimestamp();
       this.#url = params.url;
       this.#navigation.withinDocument.resolve();
@@ -354,7 +372,7 @@ export class BrowsingContextImpl {
           method: ChromiumBidi.BrowsingContext.EventNames.FragmentNavigated,
           params: {
             context: this.id,
-            navigation: null,
+            navigation: this.#virtualNavigationId,
             timestamp,
             url: this.#url,
           },
@@ -367,21 +385,35 @@ export class BrowsingContextImpl {
       if (this.id !== params.frameId) {
         return;
       }
+      // Generate a new virtual navigation id.
+      this.#virtualNavigationId = uuidv4();
       this.#eventManager.registerEvent(
         {
           type: 'event',
           method: ChromiumBidi.BrowsingContext.EventNames.NavigationStarted,
           params: {
             context: this.id,
-            navigation: null,
+            navigation: this.#virtualNavigationId,
             timestamp: BrowsingContextImpl.getTimestamp(),
-            url: '',
+            // 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
+            // provide the URL in case of BiDi-initiated navigations.
+            // TODO: provide proper URL in case of user-initiated navigations.
+            url: this.#pendingNavigationUrl ?? 'UNKNOWN',
           },
         },
         this.id
       );
     });
 
+    // TODO: don't use deprecated `Page.frameScheduledNavigation` event.
+    this.#cdpTarget.cdpClient.on('Page.frameScheduledNavigation', (params) => {
+      if (this.id !== params.frameId) {
+        return;
+      }
+      this.#pendingNavigationUrl = params.url;
+    });
+
     this.#cdpTarget.cdpClient.on('Page.lifecycleEvent', (params) => {
       if (this.id !== params.frameId) {
         return;
@@ -419,7 +451,7 @@ export class BrowsingContextImpl {
               method: ChromiumBidi.BrowsingContext.EventNames.DomContentLoaded,
               params: {
                 context: this.id,
-                navigation: this.#loaderId ?? null,
+                navigation: this.#virtualNavigationId,
                 timestamp,
                 url: this.#url,
               },
@@ -436,7 +468,7 @@ export class BrowsingContextImpl {
               method: ChromiumBidi.BrowsingContext.EventNames.Load,
               params: {
                 context: this.id,
-                navigation: this.#loaderId ?? null,
+                navigation: this.#virtualNavigationId,
                 timestamp,
                 url: this.#url,
               },
@@ -643,6 +675,12 @@ export class BrowsingContextImpl {
 
     await this.targetUnblockedOrThrow();
 
+    // Set the pending navigation URL to provide it in `browsingContext.navigationStarted`
+    // event.
+    // TODO: detect navigation start not from CDP. Check if
+    //  `Page.frameRequestedNavigation` can be used for this purpose.
+    this.#pendingNavigationUrl = url;
+
     // TODO: handle loading errors.
     const cdpNavigateResult = await this.#cdpTarget.cdpClient.sendCommand(
       'Page.navigate',
@@ -653,13 +691,15 @@ export class BrowsingContextImpl {
     );
 
     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: cdpNavigateResult.loaderId ?? null,
+            navigation: this.#virtualNavigationId,
             timestamp: BrowsingContextImpl.getTimestamp(),
             url,
           },
@@ -694,7 +734,7 @@ export class BrowsingContextImpl {
     }
 
     return {
-      navigation: cdpNavigateResult.loaderId ?? null,
+      navigation: this.#virtualNavigationId,
       // Url can change due to redirect get the latest one.
       url: wait === BrowsingContext.ReadinessState.None ? url : this.#url,
     };
@@ -724,10 +764,7 @@ export class BrowsingContextImpl {
     }
 
     return {
-      navigation:
-        wait === BrowsingContext.ReadinessState.None
-          ? null
-          : this.navigableId ?? null,
+      navigation: this.#virtualNavigationId,
       url: this.url,
     };
   }

diff --git a/src/bidiMapper/modules/network/NetworkRequest.ts b/src/bidiMapper/modules/network/NetworkRequest.ts
@@ -611,6 +611,7 @@ export class NetworkRequest {
   }
 
   #getNavigationId(): BrowsingContext.Navigation | null {
+    // Heuristic to determine if this is a navigation request, and if not return null.
     if (
       !this.#request.info ||
       !this.#request.info.loaderId ||
@@ -621,7 +622,11 @@ export class NetworkRequest {
     ) {
       return null;
     }
-    return this.#request.info.loaderId;
+
+    // Get virtual navigation ID from the browsing context.
+    return this.#networkStorage.getVirtualNavigationId(
+      this.#request?.info?.frameId
+    );
   }
 
   #getRequestData(): Network.RequestData {

diff --git a/src/bidiMapper/modules/network/NetworkStorage.spec.ts b/src/bidiMapper/modules/network/NetworkStorage.spec.ts
@@ -97,7 +97,7 @@ describe('NetworkStorage', () => {
     eventManager.on(EventManagerEvents.Event, ({message, event}) => {
       processingQueue.add(message, event);
     });
-    networkStorage = new NetworkStorage(eventManager, {
+    networkStorage = new NetworkStorage(eventManager, browsingContextStorage, {
       on(): void {
         // Used for clearing on target disconnect
       },

diff --git a/src/bidiMapper/modules/network/NetworkStorage.ts b/src/bidiMapper/modules/network/NetworkStorage.ts
@@ -25,6 +25,7 @@ import type {LoggerFn} from '../../../utils/log.js';
 import {uuidv4} from '../../../utils/uuid.js';
 import type {CdpClient} from '../../BidiMapper.js';
 import type {CdpTarget} from '../cdp/CdpTarget.js';
+import type {BrowsingContextStorage} from '../context/BrowsingContextStorage';
 import type {EventManager} from '../session/EventManager.js';
 
 import {NetworkRequest} from './NetworkRequest.js';
@@ -39,8 +40,9 @@ type NetworkInterception = Omit<
 
 /** Stores network and intercept maps. */
 export class NetworkStorage {
-  #eventManager: EventManager;
-  #logger?: LoggerFn;
+  readonly #browsingContextStorage: BrowsingContextStorage;
+  readonly #eventManager: EventManager;
+  readonly #logger?: LoggerFn;
 
   /**
    * A map from network request ID to Network Request objects.
@@ -53,9 +55,11 @@ export class NetworkStorage {
 
   constructor(
     eventManager: EventManager,
+    browsingContextStorage: BrowsingContextStorage,
     browserClient: CdpClient,
     logger?: LoggerFn
   ) {
+    this.#browsingContextStorage = browsingContextStorage;
     this.#eventManager = eventManager;
 
     browserClient.on('Target.detachedFromTarget', ({sessionId}) => {
@@ -312,4 +316,17 @@ export class NetworkStorage {
   deleteRequest(id: Network.Request) {
     this.#requests.delete(id);
   }
+
+  /**
+   * Gets the virtual navigation ID for the given navigable ID.
+   */
+  getVirtualNavigationId(contextId: string | undefined): string | null {
+    if (contextId === undefined) {
+      return null;
+    }
+    return (
+      this.#browsingContextStorage.findContext(contextId)
+        ?.virtualNavigationId ?? null
+    );
+  }
 }
diff --git a/tests/browsing_context/test_create.py b/tests/browsing_context/test_create.py
@@ -396,4 +396,7 @@ async def test_browsingContext_create_background(websocket, background,
         else:
             assert visible == "visible", "New tab should be visible when created in foreground"
 
+    if test_headless_mode != "false" and type == "window":
+        pytest.xfail("Window is not always selected in headless mode")
+
     assert has_focus is not background, "New context should not have focus" if background else "New context should have focus"
diff --git a/tests/browsing_context/test_fragment_navigated.py b/tests/browsing_context/test_fragment_navigated.py
@@ -13,7 +13,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import pytest
-from test_helpers import (ANY_TIMESTAMP, goto_url, read_JSON_message,
+from test_helpers import (ANY_TIMESTAMP, ANY_UUID, goto_url, read_JSON_message,
                           send_JSON_command, subscribe)
 
 
@@ -42,7 +42,7 @@ async def test_browsingContext_fragmentNavigated_event(websocket, context_id,
         "method": "browsingContext.fragmentNavigated",
         "params": {
             "context": context_id,
-            "navigation": None,
+            "navigation": ANY_UUID,
             "timestamp": ANY_TIMESTAMP,
             "url": base_url + "#test",
         }