1918672 - MessageHandler should automatically retry commands if the browsing context is initial or loading

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, task, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

It turns out that we emit browsingContext.contextCreated too early triggering clients to assume the tab is ready and to start interacting with it, while we are still in the process of loading about:blank. This causes various issues for other commands that might run in parallel. I'm going to add those known issues as blockers for this bug.

We probably have to wait for a solution of the discussion on https://github.com/w3c/webdriver-bidi/issues/766.

But given the importance of this issue (especially for Playwright) we should prioritize this bug and start working on it in M12.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Julian will check if it's more stable when we emit the browsingContext.contextCreated event later when the initial about:blank was loaded. At this point we might also have to not send out any of the navigation events.

Comment 2

[Julian Descottes [:jdescottes]]

Delaying the contextCreated using waitForInitialNavigationCompleted seems to fix issues for playwright (eg frequent failures when running npm run biditest -- --project='bidi-firefox-beta*' page-click.spec.ts:24 -x --repeat-each=30).

However we still have the issue that in some cases there will be no navigation, and the context will stay on the initial about:blank document.
From quick local tests it seems to happen with iframes and with popups created using window.open().

To test things out, I simply skipped iframes and popups for now, and added a new flag on the navigation helper resolveWhenNotInitial in order to resolve as soon as the document is no longer the initial document. This seems to consistently happen on a NOTIFY_LOCATION notification, which we already listen to.

However, when we start delaying this contextCreated event, we have the issue that some other events occur earlier. For now I spotted browsingContext.navigationStarted and network.* events, but there could be more?

So on top of what I prototyped here, I imagine we would also need to queue some events until we emit contextCreated, otherwise we would notify about events for a context which has not been sent to the client.

diff --git a/remote/webdriver-bidi/modules/root/browsingContext.sys.mjs b/remote/webdriver-bidi/modules/root/browsingContext.sys.mjs
--- a/remote/webdriver-bidi/modules/root/browsingContext.sys.mjs
+++ b/remote/webdriver-bidi/modules/root/browsingContext.sys.mjs
@@ -1734,12 +1734,15 @@ class BrowsingContextModule extends Root
         }
       );
 
-      await lazy.waitForInitialNavigationCompleted(
-        browsingContext.webProgress,
-        {
-          unloadTimeout: 5000,
-        }
-      );
+      if (!browsingContext.parent && !browsingContext.opener) {
+        await lazy.waitForInitialNavigationCompleted(
+          browsingContext.webProgress,
+          {
+            resolveWhenNotInitial: true,
+            unloadTimeout: 5000,
+          }
+        );
+      }
+
       this._emitEventForBrowsingContext(
         browsingContext.id,
         "browsingContext.contextCreated",

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1918672 - [bidi] Wait for initial naviation before emitting contextCreated

contextCreated is emitted as soon the initialDocument is created for a new context.
However the corresponding browsing context is soon to be replaced with another one, delay the event
until the final document is loaded.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1918672 - [bidi] Retry all commands if the browsing context is loading or is initial

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37dd306f6061
[bidi] Retry all commands if the browsing context is loading or is initial r=webdriver-reviewers,whimboo

Comment 6

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/37dd306f6061

Comment 7

[Julian Descottes [:jdescottes]]

Renaming the bug to match what we actually landed here.