From 1493756ae4ad276d1b7e955b5376daab005bb5e1 Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Wed, 14 Oct 2026 20:03:08 -0700 Subject: [PATCH 1/2] fix(frame): subscribe before checking state in navigation waits Frame events are raised on the transport thread, but WaitForURLAsync, WaitForLoadStateAsync and WaitForNavigationAsync checked the recorded state first and subscribed afterwards. A navigation or load state delivered in between was never seen again and the wait ran to its timeout. Subscribe first in all three places, guard the load-state list that both threads touch, and wait for load states by WaitUntilState so that Commit is honored when the URL already matches. Fixes: https://github.com/microsoft/playwright-dotnet/issues/3364 --- src/Playwright.Tests/PageWaitForUrlTests.cs | 23 +++ src/Playwright/Core/Frame.cs | 156 +++++++++----------- 2 files changed, 96 insertions(+), 83 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 b28b7e84b9..ab2674a026 100644 --- a/src/Playwright/Core/Frame.cs +++ b/src/Playwright/Core/Frame.cs @@ -114,13 +114,19 @@ internal void OnLoadState(WaitUntilState? add, WaitUntilState? remove) { if (add.HasValue) { - _loadStates.Add(add.Value); + lock (_loadStates) + { + _loadStates.Add(add.Value); + } LoadState?.Invoke(this, add.Value); } if (remove.HasValue) { - _loadStates.Remove(remove.Value); + lock (_loadStates) + { + _loadStates.Remove(remove.Value); + } } if (this.ParentFrame == null && add == WaitUntilState.Load && this.Page != null) { @@ -227,7 +233,6 @@ internal async Task> SelectOptionAsync(string selector, IE [MethodImpl(MethodImplOptions.NoInlining)] public async Task WaitForLoadStateAsync(LoadState? state = default, FrameWaitForLoadStateOptions? options = default) { - Waiter? waiter = null; WaitUntilState loadState = Microsoft.Playwright.WaitUntilState.Load; switch (state) { @@ -241,26 +246,34 @@ public async Task WaitForLoadStateAsync(LoadState? state = default, FrameWaitFor loadState = Microsoft.Playwright.WaitUntilState.NetworkIdle; break; } - try - { - waiter = SetupNavigationWaiter("frame.WaitForLoadStateAsync", options?.Timeout); + using var waiter = SetupNavigationWaiter("frame.WaitForLoadStateAsync", options?.Timeout); + await WaitForLoadStateInternalAsync(waiter, loadState).ConfigureAwait(false); + } - if (_loadStates.Contains(loadState)) - { - 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); - } + private async Task WaitForLoadStateInternalAsync(Waiter waiter, WaitUntilState loadState) + { + // Subscribe before checking: load states are recorded and the event is raised on the + // transport thread, and an event raised between the check and the subscription would + // never be seen again (https://github.com/microsoft/playwright-dotnet/issues/3364). + var (loadStateTask, unsubscribe) = waiter.GetWaitForEventTask(this, "LoadState", s => + { + waiter.Log($" \"{s}\" event fired"); + return s == loadState; + }); + if (HasLoadState(loadState)) + { + unsubscribe(); + waiter.Log($" not waiting, \"{loadState}\" event already fired"); + return; } - finally + await waiter.WaitForPromiseAsync(loadStateTask, unsubscribe).ConfigureAwait(false); + } + + private bool HasLoadState(WaitUntilState loadState) + { + lock (_loadStates) { - waiter?.Dispose(); + return _loadStates.Contains(loadState); } } @@ -282,7 +295,10 @@ await waiter.WaitForEventAsync(this, "LoadState", s => public async Task RunAndWaitForNavigationAsync(Func? action, FrameRunAndWaitForNavigationOptions? options = default) { using var waiter = SetupNavigationWaiter("frame.WaitForNavigationAsync", options?.Timeout); - var result = WaitForNavigationInternalAsync(waiter, options?.Url ?? options?.UrlString, options?.UrlFunc, options?.UrlRegex, options?.WaitUntil); + var urlString = options?.Url ?? options?.UrlString; + var waitUntil = options?.WaitUntil ?? WaitUntilState.Load; + var navigation = SubscribeToNavigation(waiter, urlString, UrlMatch(urlString, options?.UrlRegex, options?.UrlFunc), waitUntil); + var result = WaitForNavigationInternalAsync(waiter, navigation, waitUntil); if (action != null) { @@ -292,19 +308,21 @@ await waiter.WaitForEventAsync(this, "LoadState", s => return await result.ConfigureAwait(false); } - private async Task WaitForNavigationInternalAsync( - Waiter waiter, - string? urlString, - Func? urlFunc, - Regex? urlRegex, - WaitUntilState? waitUntil) + private URLMatch UrlMatch(string? urlString, Regex? urlRegex, Func? urlFunc) + => new() + { + glob = urlString, + re = urlRegex, + func = urlFunc, + baseURL = (Page as Page)?.Context.BaseURL, + }; + + private (Task Task, Action Unsubscribe) SubscribeToNavigation(Waiter waiter, string? urlString, URLMatch urlMatch, WaitUntilState waitUntil) { - waitUntil ??= WaitUntilState.Load; string toUrl = !string.IsNullOrEmpty(urlString) ? $" to \"{urlString}\"" : string.Empty; - waiter.Log($"waiting for navigation{toUrl} until \"{waitUntil}\""); - var navigatedEventTask = waiter.WaitForEventAsync( + return waiter.GetWaitForEventTask( this, "Navigated", e => @@ -316,16 +334,16 @@ await waiter.WaitForEventAsync(this, "LoadState", s => } waiter.Log($" navigated to \"{e.Url}\""); - return new URLMatch() - { - glob = urlString, - re = urlRegex, - func = urlFunc, - baseURL = (Page as Page)?.Context.BaseURL, - }.Match(e.Url); + return urlMatch.Match(e.Url); }); + } - var navigatedEvent = await navigatedEventTask.ConfigureAwait(false); + private async Task WaitForNavigationInternalAsync( + Waiter waiter, + (Task Task, Action Unsubscribe) navigation, + WaitUntilState waitUntil) + { + var navigatedEvent = await waiter.WaitForPromiseAsync(navigation.Task, navigation.Unsubscribe).ConfigureAwait(false); if (navigatedEvent.Error != null) { @@ -333,17 +351,7 @@ await waiter.WaitForEventAsync(this, "LoadState", s => await waiter.WaitForPromiseAsync(Task.FromException(ex)).ConfigureAwait(false); } - if (!_loadStates.Select(s => s.ToValueString()).Contains(waitUntil.Value.ToValueString())) - { - await waiter.WaitForEventAsync( - this, - "LoadState", - e => - { - waiter.Log($" \"{e}\" event fired"); - return e.ToValueString() == waitUntil.Value.ToValueString(); - }).ConfigureAwait(false); - } + await WaitForLoadStateInternalAsync(waiter, waitUntil).ConfigureAwait(false); var request = navigatedEvent.NewDocument?.Request; @@ -1049,44 +1057,26 @@ await SendMessageToServerAsync( } } - private Task WaitForURLAsync(string? urlString, Regex? urlRegex, Func? urlFunc, FrameWaitForURLOptions? options = default) - { - if (new URLMatch() - { - 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) + private async Task WaitForURLAsync(string? urlString, Regex? urlRegex, Func? urlFunc, FrameWaitForURLOptions? options = default) { - if (waitUntilState == null) + var urlMatch = UrlMatch(urlString, urlRegex, urlFunc); + var waitUntil = options?.WaitUntil ?? WaitUntilState.Load; + using var waiter = SetupNavigationWaiter("frame.WaitForURLAsync", options?.Timeout); + + // Subscribe to navigations before checking the current URL: the frame navigates on the + // transport thread, and a navigation committed between the check and the subscription + // would never be seen again (https://github.com/microsoft/playwright-dotnet/issues/3364). + var navigation = SubscribeToNavigation(waiter, urlString, urlMatch, waitUntil); + if (urlMatch.Match(Url)) { - return null; + // Already there, only the load state can still be pending. Checking the recorded + // states also honors "commit", which is recorded as soon as the URL changes. + navigation.Unsubscribe(); + await WaitForLoadStateInternalAsync(waiter, waitUntil).ConfigureAwait(false); + return; } - return waitUntilState switch - { - WaitUntilState.Load => Microsoft.Playwright.LoadState.Load, - WaitUntilState.DOMContentLoaded => Microsoft.Playwright.LoadState.DOMContentLoaded, - WaitUntilState.NetworkIdle => Microsoft.Playwright.LoadState.NetworkIdle, - _ => null, - }; + await WaitForNavigationInternalAsync(waiter, navigation, waitUntil).ConfigureAwait(false); } private Waiter SetupNavigationWaiter(string @event, float? timeout) From 6629ab4f84fe520c57d0bfff3656f5dcb123075b Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Thu, 15 Oct 2026 11:26:18 -0700 Subject: [PATCH 2/2] fix(frame): drop the locks and keep the JS method shape Review follow-up: no lock around _loadStates, the three wait methods keep their original structure, and the only deviation from the JS client is subscribing before the state check, with a comment at each spot. WaitForURLAsync re-checks the URL inside the navigation wait once the subscription exists instead of restructuring the method. --- src/Playwright/Core/Frame.cs | 176 ++++++++++++++++++++--------------- 1 file changed, 103 insertions(+), 73 deletions(-) diff --git a/src/Playwright/Core/Frame.cs b/src/Playwright/Core/Frame.cs index ab2674a026..b419286547 100644 --- a/src/Playwright/Core/Frame.cs +++ b/src/Playwright/Core/Frame.cs @@ -114,19 +114,13 @@ internal void OnLoadState(WaitUntilState? add, WaitUntilState? remove) { if (add.HasValue) { - lock (_loadStates) - { - _loadStates.Add(add.Value); - } + _loadStates.Add(add.Value); LoadState?.Invoke(this, add.Value); } if (remove.HasValue) { - lock (_loadStates) - { - _loadStates.Remove(remove.Value); - } + _loadStates.Remove(remove.Value); } if (this.ParentFrame == null && add == WaitUntilState.Load && this.Page != null) { @@ -233,6 +227,7 @@ internal async Task> SelectOptionAsync(string selector, IE [MethodImpl(MethodImplOptions.NoInlining)] public async Task WaitForLoadStateAsync(LoadState? state = default, FrameWaitForLoadStateOptions? options = default) { + Waiter? waiter = null; WaitUntilState loadState = Microsoft.Playwright.WaitUntilState.Load; switch (state) { @@ -246,34 +241,31 @@ public async Task WaitForLoadStateAsync(LoadState? state = default, FrameWaitFor loadState = Microsoft.Playwright.WaitUntilState.NetworkIdle; break; } - using var waiter = SetupNavigationWaiter("frame.WaitForLoadStateAsync", options?.Timeout); - await WaitForLoadStateInternalAsync(waiter, loadState).ConfigureAwait(false); - } - - private async Task WaitForLoadStateInternalAsync(Waiter waiter, WaitUntilState loadState) - { - // Subscribe before checking: load states are recorded and the event is raised on the - // transport thread, and an event raised between the check and the subscription would - // never be seen again (https://github.com/microsoft/playwright-dotnet/issues/3364). - var (loadStateTask, unsubscribe) = waiter.GetWaitForEventTask(this, "LoadState", s => - { - waiter.Log($" \"{s}\" event fired"); - return s == loadState; - }); - if (HasLoadState(loadState)) + try { - unsubscribe(); - waiter.Log($" not waiting, \"{loadState}\" event already fired"); - return; - } - await waiter.WaitForPromiseAsync(loadStateTask, unsubscribe).ConfigureAwait(false); - } + waiter = SetupNavigationWaiter("frame.WaitForLoadStateAsync", options?.Timeout); - private bool HasLoadState(WaitUntilState loadState) - { - lock (_loadStates) + // Unlike the JS client, load states are updated on the transport thread, so subscribe + // before checking: an event raised between the check and the subscription would be lost + // (https://github.com/microsoft/playwright-dotnet/issues/3364). + var (loadStateTask, unsubscribe) = waiter.GetWaitForEventTask(this, "LoadState", s => + { + waiter.Log($" \"{s}\" event fired"); + return s == loadState; + }); + if (_loadStates.Contains(loadState)) + { + unsubscribe(); + waiter.Log($" not waiting, \"{state}\" event already fired"); + } + else + { + await waiter.WaitForPromiseAsync(loadStateTask, unsubscribe).ConfigureAwait(false); + } + } + finally { - return _loadStates.Contains(loadState); + waiter?.Dispose(); } } @@ -295,10 +287,7 @@ private bool HasLoadState(WaitUntilState loadState) public async Task RunAndWaitForNavigationAsync(Func? action, FrameRunAndWaitForNavigationOptions? options = default) { using var waiter = SetupNavigationWaiter("frame.WaitForNavigationAsync", options?.Timeout); - var urlString = options?.Url ?? options?.UrlString; - var waitUntil = options?.WaitUntil ?? WaitUntilState.Load; - var navigation = SubscribeToNavigation(waiter, urlString, UrlMatch(urlString, options?.UrlRegex, options?.UrlFunc), waitUntil); - var result = WaitForNavigationInternalAsync(waiter, navigation, waitUntil); + var result = WaitForNavigationInternalAsync(waiter, options?.Url ?? options?.UrlString, options?.UrlFunc, options?.UrlRegex, options?.WaitUntil); if (action != null) { @@ -308,21 +297,27 @@ private bool HasLoadState(WaitUntilState loadState) return await result.ConfigureAwait(false); } - private URLMatch UrlMatch(string? urlString, Regex? urlRegex, Func? urlFunc) - => new() + private async Task WaitForNavigationInternalAsync( + Waiter waiter, + string? urlString, + Func? urlFunc, + Regex? urlRegex, + WaitUntilState? waitUntil, + bool recheckUrl = false) + { + waitUntil ??= WaitUntilState.Load; + string toUrl = !string.IsNullOrEmpty(urlString) ? $" to \"{urlString}\"" : string.Empty; + + waiter.Log($"waiting for navigation{toUrl} until \"{waitUntil}\""); + + var urlMatch = new URLMatch() { glob = urlString, re = urlRegex, func = urlFunc, baseURL = (Page as Page)?.Context.BaseURL, }; - - private (Task Task, Action Unsubscribe) SubscribeToNavigation(Waiter waiter, string? urlString, URLMatch urlMatch, WaitUntilState waitUntil) - { - string toUrl = !string.IsNullOrEmpty(urlString) ? $" to \"{urlString}\"" : string.Empty; - waiter.Log($"waiting for navigation{toUrl} until \"{waitUntil}\""); - - return waiter.GetWaitForEventTask( + var (navigatedEventTask, unsubscribe) = waiter.GetWaitForEventTask( this, "Navigated", e => @@ -336,24 +331,44 @@ private URLMatch UrlMatch(string? urlString, Regex? urlRegex, Func waiter.Log($" navigated to \"{e.Url}\""); return urlMatch.Match(e.Url); }); - } - private async Task WaitForNavigationInternalAsync( - Waiter waiter, - (Task Task, Action Unsubscribe) navigation, - WaitUntilState waitUntil) - { - var navigatedEvent = await waiter.WaitForPromiseAsync(navigation.Task, navigation.Unsubscribe).ConfigureAwait(false); - - if (navigatedEvent.Error != null) + FrameNavigatedEventArgs? navigatedEvent = null; + // Unlike the JS client, the frame navigates on the transport thread. WaitForURLAsync checked + // the URL before the subscription above existed, so check again now that a navigation can no + // longer go unnoticed (https://github.com/microsoft/playwright-dotnet/issues/3364). + if (recheckUrl && urlMatch.Match(Url)) { - var ex = new PlaywrightException(navigatedEvent.Error); - await waiter.WaitForPromiseAsync(Task.FromException(ex)).ConfigureAwait(false); + unsubscribe(); + } + else + { + navigatedEvent = await waiter.WaitForPromiseAsync(navigatedEventTask, unsubscribe).ConfigureAwait(false); + if (navigatedEvent.Error != null) + { + var ex = new PlaywrightException(navigatedEvent.Error); + await waiter.WaitForPromiseAsync(Task.FromException(ex)).ConfigureAwait(false); + } } - await WaitForLoadStateInternalAsync(waiter, waitUntil).ConfigureAwait(false); + // Same as in WaitForLoadStateAsync: subscribe before checking the load state. + var (loadStateTask, unsubscribeLoadState) = waiter.GetWaitForEventTask( + this, + "LoadState", + e => + { + waiter.Log($" \"{e}\" event fired"); + return e.ToValueString() == waitUntil.Value.ToValueString(); + }); + if (_loadStates.Select(s => s.ToValueString()).Contains(waitUntil.Value.ToValueString())) + { + unsubscribeLoadState(); + } + else + { + await waiter.WaitForPromiseAsync(loadStateTask, unsubscribeLoadState).ConfigureAwait(false); + } - var request = navigatedEvent.NewDocument?.Request; + var request = navigatedEvent?.NewDocument?.Request; return request != null ? await waiter.WaitForPromiseAsync(request.FinalRequest.ResponseAsync()).ConfigureAwait(false) @@ -1059,24 +1074,39 @@ await SendMessageToServerAsync( private async Task WaitForURLAsync(string? urlString, Regex? urlRegex, Func? urlFunc, FrameWaitForURLOptions? options = default) { - var urlMatch = UrlMatch(urlString, urlRegex, urlFunc); - var waitUntil = options?.WaitUntil ?? WaitUntilState.Load; - using var waiter = SetupNavigationWaiter("frame.WaitForURLAsync", options?.Timeout); - - // Subscribe to navigations before checking the current URL: the frame navigates on the - // transport thread, and a navigation committed between the check and the subscription - // would never be seen again (https://github.com/microsoft/playwright-dotnet/issues/3364). - var navigation = SubscribeToNavigation(waiter, urlString, urlMatch, waitUntil); - if (urlMatch.Match(Url)) + if (new URLMatch() + { + glob = urlString, + re = urlRegex, + func = urlFunc, + baseURL = (Page as Page)?.Context.BaseURL, + }.Match(Url)) { - // Already there, only the load state can still be pending. Checking the recorded - // states also honors "commit", which is recorded as soon as the URL changes. - navigation.Unsubscribe(); - await WaitForLoadStateInternalAsync(waiter, waitUntil).ConfigureAwait(false); + await WaitForLoadStateAsync(ToLoadState(options?.WaitUntil), new() { Timeout = options?.Timeout }).ConfigureAwait(false); return; } - await WaitForNavigationInternalAsync(waiter, navigation, waitUntil).ConfigureAwait(false); + // Unlike the JS client, the frame can navigate between the check above and the subscription + // inside WaitForNavigationInternalAsync, so have it check the URL again once subscribed + // (https://github.com/microsoft/playwright-dotnet/issues/3364). + using var waiter = SetupNavigationWaiter("frame.WaitForNavigationAsync", options?.Timeout); + await WaitForNavigationInternalAsync(waiter, urlString, urlFunc, urlRegex, options?.WaitUntil, recheckUrl: true).ConfigureAwait(false); + } + + private LoadState? ToLoadState(WaitUntilState? waitUntilState) + { + if (waitUntilState == null) + { + return null; + } + + return waitUntilState switch + { + WaitUntilState.Load => Microsoft.Playwright.LoadState.Load, + WaitUntilState.DOMContentLoaded => Microsoft.Playwright.LoadState.DOMContentLoaded, + WaitUntilState.NetworkIdle => Microsoft.Playwright.LoadState.NetworkIdle, + _ => null, + }; } private Waiter SetupNavigationWaiter(string @event, float? timeout)