From ae8931bd4beae3ae65e681320eb6e36a8c65b4fa Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Fri, 16 Oct 2026 14:44:11 -0700 Subject: [PATCH 1/2] fix(frame): make check and subscribe one step in the navigation waits Load states are one TaskCompletionSource per WaitUntilState that the pump completes, so WaitForLoadStateAsync and WaitForNavigationAsync await the current slot instead of checking a list and then subscribing. WaitForURLAsync matches Url and subscribes under the dispatch lock that OnNavigated takes to update Url. Fixes: https://github.com/microsoft/playwright-dotnet/issues/3364 --- src/Playwright.Tests/PageWaitForUrlTests.cs | 23 +++++ src/Playwright/Core/Frame.cs | 108 ++++++++++++-------- 2 files changed, 90 insertions(+), 41 deletions(-) diff --git a/src/Playwright.Tests/PageWaitForUrlTests.cs b/src/Playwright.Tests/PageWaitForUrlTests.cs index 9782e51802..1c2991e8c4 100644 --- a/src/Playwright.Tests/PageWaitForUrlTests.cs +++ b/src/Playwright.Tests/PageWaitForUrlTests.cs @@ -153,4 +153,27 @@ 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(""); + }); + Server.SetRoute("/b.html", context => + { + context.Response.ContentType = "text/html"; + return context.Response.WriteAsync("

b

"); + }); + for (int i = 0; i < 10; i++) + { + await Page.GotoAsync(Server.Prefix + "/a.html"); + await Page.WaitForURLAsync("**/b.html", new() { Timeout = 5000 }); + } + } } diff --git a/src/Playwright/Core/Frame.cs b/src/Playwright/Core/Frame.cs index bdfbc3f60b..0d219d0e83 100644 --- a/src/Playwright/Core/Frame.cs +++ b/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,7 +43,11 @@ namespace Microsoft.Playwright.Core; internal class Frame : ChannelOwner, IFrame { - private readonly List _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[] _loadStateSlots = new TaskCompletionSource[Enum.GetValues(typeof(WaitUntilState)).Length]; internal readonly List _childFrames = new(); internal Frame(ChannelOwner parent, string guid, FrameInitializer initializer) : base(parent, guid) @@ -50,7 +55,14 @@ internal Frame(ChannelOwner parent, string guid, FrameInitializer initializer) : 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 +126,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 +144,24 @@ internal void OnLoadState(WaitUntilState? add, WaitUntilState? remove) } } + private static TaskCompletionSource NewLoadStateSlot() + => new(TaskCreationOptions.RunContinuationsAsynchronously); + + private TaskCompletionSource 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? navigated; + lock (_connection.DispatchLock) + { + // WaitForURLAsync matches Url and subscribes to Navigated under the same lock, so a + // waiter either sees the new Url or is in the snapshot taken here. + Url = e.Url; + Name = e.Name; + navigated = Navigated; + } + navigated?.Invoke(this, e); if (string.IsNullOrEmpty(e.Error)) { @@ -245,17 +270,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(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 +356,11 @@ await waiter.WaitForEventAsync(this, "LoadState", s => await waiter.WaitForPromiseAsync(Task.FromException(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( - 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 +1077,34 @@ await SendMessageToServerAsync( private Task WaitForURLAsync(string? urlString, Regex? urlRegex, Func? urlFunc, FrameWaitForURLOptions? options = default) { - if (new URLMatch() + // The same two steps as the JS client, under the dispatch lock: 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). OnNavigated + // updates Url and snapshots the Navigated handlers under the same lock, and both calls + // below subscribe before their first await. + lock (_connection.DispatchLock) { - glob = urlString, - re = urlRegex, - func = urlFunc, - baseURL = (Page as Page)?.Context.BaseURL, - }.Match(Url)) - { - return WaitForLoadStateAsync(ToLoadState(options?.WaitUntil), new() { Timeout = options?.Timeout }); - } - - return WaitForNavigationAsync( - new() + if (new URLMatch() { - UrlString = urlString, - UrlRegex = urlRegex, - UrlFunc = urlFunc, - Timeout = options?.Timeout, - WaitUntil = options?.WaitUntil, - }); + glob = urlString, + re = urlRegex, + func = urlFunc, + baseURL = (Page as Page)?.Context.BaseURL, + }.Match(Url)) + { + return WaitForLoadStateAsync(ToLoadState(options?.WaitUntil), new() { Timeout = options?.Timeout }); + } + + return WaitForNavigationAsync( + new() + { + UrlString = urlString, + UrlRegex = urlRegex, + UrlFunc = urlFunc, + Timeout = options?.Timeout, + WaitUntil = options?.WaitUntil, + }); + } } private LoadState? ToLoadState(WaitUntilState? waitUntilState) From fdfd53713b96298eaaeceff47185f53af2fa37ca Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Thu, 22 Oct 2026 16:42:46 -0700 Subject: [PATCH 2/2] fix(frame): run the URL matcher outside the dispatch lock The matcher can be a user predicate, and a predicate that blocks on a Playwright call deadlocked while the lock was held: the pump waits for the lock at the next frame, page or worker message, so the reply never arrives. Read Url and a navigation count under the lock, match outside it, and subscribe under the lock only if no navigation landed since, otherwise match again. --- src/Playwright.Tests/PageWaitForUrlTests.cs | 16 ++++++ src/Playwright/Core/Frame.cs | 64 ++++++++++++++------- 2 files changed, 58 insertions(+), 22 deletions(-) diff --git a/src/Playwright.Tests/PageWaitForUrlTests.cs b/src/Playwright.Tests/PageWaitForUrlTests.cs index 1c2991e8c4..f958fda934 100644 --- a/src/Playwright.Tests/PageWaitForUrlTests.cs +++ b/src/Playwright.Tests/PageWaitForUrlTests.cs @@ -176,4 +176,20 @@ public async Task ShouldNotMissNavigationCommittedRightAfterTheCall() 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); + } } diff --git a/src/Playwright/Core/Frame.cs b/src/Playwright/Core/Frame.cs index 0d219d0e83..09d87419e1 100644 --- a/src/Playwright/Core/Frame.cs +++ b/src/Playwright/Core/Frame.cs @@ -48,6 +48,9 @@ internal class Frame : ChannelOwner, IFrame // 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[] _loadStateSlots = new TaskCompletionSource[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 _childFrames = new(); internal Frame(ChannelOwner parent, string guid, FrameInitializer initializer) : base(parent, guid) @@ -155,10 +158,11 @@ internal void OnNavigated(FrameNavigatedEventArgs e) EventHandler? navigated; lock (_connection.DispatchLock) { - // WaitForURLAsync matches Url and subscribes to Navigated under the same lock, so a - // waiter either sees the new Url or is in the snapshot taken here. + // 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); @@ -1077,33 +1081,49 @@ await SendMessageToServerAsync( private Task WaitForURLAsync(string? urlString, Regex? urlRegex, Func? urlFunc, FrameWaitForURLOptions? options = default) { - // The same two steps as the JS client, under the dispatch lock: 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). OnNavigated - // updates Url and snapshots the Navigated handlers under the same lock, and both calls - // below subscribe before their first await. - lock (_connection.DispatchLock) + // 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, + }; + while (true) { - if (new URLMatch() + string url; + int navigationCount; + lock (_connection.DispatchLock) { - glob = urlString, - re = urlRegex, - func = urlFunc, - baseURL = (Page as Page)?.Context.BaseURL, - }.Match(Url)) + url = Url; + navigationCount = _navigationCount; + } + + if (matcher.Match(url)) { return WaitForLoadStateAsync(ToLoadState(options?.WaitUntil), new() { Timeout = options?.Timeout }); } - return WaitForNavigationAsync( - new() + lock (_connection.DispatchLock) + { + if (navigationCount == _navigationCount) { - UrlString = urlString, - UrlRegex = urlRegex, - UrlFunc = urlFunc, - Timeout = options?.Timeout, - WaitUntil = options?.WaitUntil, - }); + // Subscribes to Navigated before its first await. + return WaitForNavigationAsync( + new() + { + UrlString = urlString, + UrlRegex = urlRegex, + UrlFunc = urlFunc, + Timeout = options?.Timeout, + WaitUntil = options?.WaitUntil, + }); + } + } } }