fix(frame): make check and subscribe one step in the navigation waits

src/Playwright.Tests/PageWaitForUrlTests.cs

@@ -153,4 +153,43 @@ await TaskUtils.WhenAll(
             frame.EvaluateAsync("url => window.location.href = url", Server.Prefix + "/grid.html"));
         ;
     }
+
+    [PlaywrightTest]
+    public async Task ShouldNotMissNavigationCommittedRightAfterTheCall()
+    {
+        // https://github.com/microsoft/playwright-dotnet/issues/3364
+        // Page A navigates to B a few milliseconds after it loads. The wait must see that
+        // navigation even when it commits right after WaitForURLAsync was called.
+        Server.SetRoute("/a.html", context =>
+        {
+            context.Response.ContentType = "text/html";
+            return context.Response.WriteAsync("<script>addEventListener('load', () => setTimeout(() => location.href = '/b.html', 5))</script>");
+        });
+        Server.SetRoute("/b.html", context =>
+        {
+            context.Response.ContentType = "text/html";
+            return context.Response.WriteAsync("<h1>b</h1>");
+        });
+        for (int i = 0; i < 10; i++)
+        {
+            await Page.GotoAsync(Server.Prefix + "/a.html");
+            await Page.WaitForURLAsync("**/b.html", new() { Timeout = 5000 });
+        }
+    }
+
+    [PlaywrightTest]
+    public async Task ShouldNotHoldTheDispatchLockWhileMatchingTheUrl()
+    {
+        // The predicate is user code, so it must run with no client lock held. This one blocks
+        // on a call whose reply follows a frameAttached message, which the message pump
+        // handles under the dispatch lock.
+        await Page.GotoAsync(Server.EmptyPage);
+        var completed = false;
+        await Page.WaitForURLAsync(url =>
+        {
+            completed = Page.EvaluateAsync("() => { document.body.appendChild(document.createElement('iframe')); }").Wait(5000);
+            return true;
+        });
+        Assert.True(completed);
+    }
 }

src/Playwright/Core/Frame.cs

@@ -33,6 +33,7 @@
 using System.Text.Json;
 using System.Text.Json.Serialization;
 using System.Text.RegularExpressions;
+using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.Playwright.Helpers;
 using Microsoft.Playwright.Transport;
@@ -42,15 +43,29 @@ namespace Microsoft.Playwright.Core;
 
 internal class Frame : ChannelOwner, IFrame
 {
-    private readonly List<WaitUntilState> _loadStates = new();
+    // One slot per WaitUntilState. A completed task means the state is present, a pending one
+    // that it is not, and waiters await the current slot, so there is no check to interleave
+    // with a subscription. Only a completed slot is replaced on remove, so a waiter started
+    // before a navigation resolves when the new document reaches the state.
+    private readonly TaskCompletionSource<bool>[] _loadStateSlots = new TaskCompletionSource<bool>[Enum.GetValues(typeof(WaitUntilState)).Length];
+    // Incremented with every Url update, under the dispatch lock. WaitForURLAsync uses it to
+    // tell whether Url changed while it ran the matcher outside the lock.
+    private int _navigationCount;
     internal readonly List<Frame> _childFrames = new();
 
     internal Frame(ChannelOwner parent, string guid, FrameInitializer initializer) : base(parent, guid)
     {
         Url = initializer.Url;
         Name = initializer.Name;
         ParentFrame = initializer.ParentFrame;
-        _loadStates = initializer.LoadStates;
+        for (var i = 0; i < _loadStateSlots.Length; i++)
+        {
+            _loadStateSlots[i] = NewLoadStateSlot();
+        }
+        foreach (var state in initializer.LoadStates)
+        {
+            _loadStateSlots[(int)state].TrySetResult(true);
+        }
         Webmcp = new WebMCP(this);
     }
 
@@ -114,13 +129,13 @@ internal void OnLoadState(WaitUntilState? add, WaitUntilState? remove)
     {
         if (add.HasValue)
         {
-            _loadStates.Add(add.Value);
+            _loadStateSlots[(int)add.Value].TrySetResult(true);
             LoadState?.Invoke(this, add.Value);
         }
 
-        if (remove.HasValue)
+        if (remove.HasValue && _loadStateSlots[(int)remove.Value].Task.IsCompleted)
         {
-            _loadStates.Remove(remove.Value);
+            Volatile.Write(ref _loadStateSlots[(int)remove.Value], NewLoadStateSlot());
         }
         if (this.ParentFrame == null && add == WaitUntilState.Load && this.Page != null)
         {
@@ -132,11 +147,25 @@ internal void OnLoadState(WaitUntilState? add, WaitUntilState? remove)
         }
     }
 
+    private static TaskCompletionSource<bool> NewLoadStateSlot()
+        => new(TaskCreationOptions.RunContinuationsAsynchronously);
+
+    private TaskCompletionSource<bool> LoadStateSlot(WaitUntilState state)
+        => Volatile.Read(ref _loadStateSlots[(int)state]);
+
     internal void OnNavigated(FrameNavigatedEventArgs e)
     {
-        Url = e.Url;
-        Name = e.Name;
-        Navigated?.Invoke(this, e);
+        EventHandler<FrameNavigatedEventArgs>? navigated;
+        lock (_connection.DispatchLock)
+        {
+            // WaitForURLAsync subscribes to Navigated under the same lock, after checking that
+            // the count has not moved, so a waiter either matched this Url or is in the snapshot.
+            Url = e.Url;
+            Name = e.Name;
+            _navigationCount++;
+            navigated = Navigated;
+        }
+        navigated?.Invoke(this, e);
 
         if (string.IsNullOrEmpty(e.Error))
         {
@@ -245,17 +274,15 @@ public async Task WaitForLoadStateAsync(LoadState? state = default, FrameWaitFor
         {
             waiter = SetupNavigationWaiter("frame.WaitForLoadStateAsync", options?.Timeout);
 
-            if (_loadStates.Contains(loadState))
+            var loadStateTask = LoadStateSlot(loadState).Task;
+            if (loadStateTask.IsCompleted)
             {
                 waiter.Log($"  not waiting, \"{state}\" event already fired");
             }
             else
             {
-                await waiter.WaitForEventAsync<WaitUntilState>(this, "LoadState", s =>
-                {
-                    waiter.Log($"  \"{s}\" event fired");
-                    return s == loadState;
-                }).ConfigureAwait(false);
+                await waiter.WaitForPromiseAsync(loadStateTask).ConfigureAwait(false);
+                waiter.Log($"  \"{state}\" event fired");
             }
         }
         finally
@@ -333,16 +360,11 @@ await waiter.WaitForEventAsync<WaitUntilState>(this, "LoadState", s =>
             await waiter.WaitForPromiseAsync(Task.FromException<object>(ex)).ConfigureAwait(false);
         }
 
-        if (!_loadStates.Select(s => s.ToValueString()).Contains(waitUntil.Value.ToValueString()))
+        var loadStateTask = LoadStateSlot(waitUntil.Value).Task;
+        if (!loadStateTask.IsCompleted)
         {
-            await waiter.WaitForEventAsync<WaitUntilState>(
-                this,
-                "LoadState",
-                e =>
-                {
-                    waiter.Log($"  \"{e}\" event fired");
-                    return e.ToValueString() == waitUntil.Value.ToValueString();
-                }).ConfigureAwait(false);
+            await waiter.WaitForPromiseAsync(loadStateTask).ConfigureAwait(false);
+            waiter.Log($"  \"{waitUntil}\" event fired");
         }
 
         var request = navigatedEvent.NewDocument?.Request;
@@ -1059,26 +1081,50 @@ await SendMessageToServerAsync(
 
     private Task WaitForURLAsync(string? urlString, Regex? urlRegex, Func<string, bool>? urlFunc, FrameWaitForURLOptions? options = default)
     {
-        if (new URLMatch()
+        // The same two steps as the JS client. Unlike there, the pump can navigate the frame
+        // between matching Url and subscribing to Navigated, and the navigation that just
+        // happened would then be waited for again (#3364). The matcher can run user code, so it
+        // runs outside the dispatch lock; the subscription happens under it, only if no
+        // navigation landed since Url was read, and otherwise the match is retried.
+        var matcher = new URLMatch()
         {
             glob = urlString,
             re = urlRegex,
             func = urlFunc,
             baseURL = (Page as Page)?.Context.BaseURL,
-        }.Match(Url))
+        };
+        while (true)
         {
-            return WaitForLoadStateAsync(ToLoadState(options?.WaitUntil), new() { Timeout = options?.Timeout });
-        }
+            string url;
+            int navigationCount;
+            lock (_connection.DispatchLock)
+            {
+                url = Url;
+                navigationCount = _navigationCount;
+            }
 
-        return WaitForNavigationAsync(
-            new()
+            if (matcher.Match(url))
             {
-                UrlString = urlString,
-                UrlRegex = urlRegex,
-                UrlFunc = urlFunc,
-                Timeout = options?.Timeout,
-                WaitUntil = options?.WaitUntil,
-            });
+                return WaitForLoadStateAsync(ToLoadState(options?.WaitUntil), new() { Timeout = options?.Timeout });
+            }
+
+            lock (_connection.DispatchLock)
+            {
+                if (navigationCount == _navigationCount)
+                {
+                    // Subscribes to Navigated before its first await.
+                    return WaitForNavigationAsync(
+                        new()
+                        {
+                            UrlString = urlString,
+                            UrlRegex = urlRegex,
+                            UrlFunc = urlFunc,
+                            Timeout = options?.Timeout,
+                            WaitUntil = options?.WaitUntil,
+                        });
+                }
+            }
+        }
     }
 
     private LoadState? ToLoadState(WaitUntilState? waitUntilState)