diff --git a/src/Playwright.Tests/PageWaitForUrlTests.cs b/src/Playwright.Tests/PageWaitForUrlTests.cs index 786a6bd9f..82c5db86b 100644 --- a/src/Playwright.Tests/PageWaitForUrlTests.cs +++ b/src/Playwright.Tests/PageWaitForUrlTests.cs @@ -80,6 +80,30 @@ public async Task UrlShouldWorkWithBothDomcontentloadedAndLoad() } } + // See https://github.com/microsoft/playwright-dotnet/issues/3364 + [PlaywrightTest] + public async Task ShouldNotMissANavigationThatCommitsWhileTheUrlIsChecked() + { + await Page.GotoAsync(Server.EmptyPage); + var target = Server.Prefix + "/grid.html"; + Task navigation = null; + await Page.WaitForURLAsync( + _ => + { + // The first check is of the page as it is: navigate away, and answer only once the navigation has + // committed, so that it lands after the URL was read and before a wait for the next one could start. + if (navigation == null) + { + navigation = Page.EvaluateAsync("url => window.location.href = url", target); + SpinWait.SpinUntil(() => Page.Url == target, 10_000); + } + + return _ == target; + }, + new() { Timeout = 5_000 }); + await navigation; + } + [PlaywrightTest("page-wait-for-url.spec.ts", "should work with clicking on anchor links")] public async Task ShouldWorkWithClickingOnAnchorLinks() { diff --git a/src/Playwright/Core/Frame.cs b/src/Playwright/Core/Frame.cs index bf7a34f20..44236a6ab 100644 --- a/src/Playwright/Core/Frame.cs +++ b/src/Playwright/Core/Frame.cs @@ -43,6 +43,11 @@ namespace Microsoft.Playwright.Core; internal class Frame : ChannelOwner, IFrame { private readonly List _loadStates = new(); + + // Guards Url and _loadStates. They are updated on the thread that reads the connection, and checked by waits on + // other threads: OnNavigated and OnLoadState update them and take the handlers to raise under this lock, and a + // wait checks and subscribes under it, so an update is either seen by the check or raised to the new handler. + private readonly object _stateLock = new(); internal readonly List _childFrames = new(); internal Frame(ChannelOwner parent, string guid, FrameInitializer initializer) : base(parent, guid) @@ -114,13 +119,22 @@ internal void OnLoadState(WaitUntilState? add, WaitUntilState? remove) { if (add.HasValue) { - _loadStates.Add(add.Value); - LoadState?.Invoke(this, add.Value); + EventHandler? loadStateHandlers; + lock (_stateLock) + { + _loadStates.Add(add.Value); + loadStateHandlers = LoadState; + } + + loadStateHandlers?.Invoke(this, add.Value); } if (remove.HasValue) { - _loadStates.Remove(remove.Value); + lock (_stateLock) + { + _loadStates.Remove(remove.Value); + } } if (this.ParentFrame == null && add == WaitUntilState.Load && this.Page != null) { @@ -134,9 +148,15 @@ internal void OnLoadState(WaitUntilState? add, WaitUntilState? remove) internal void OnNavigated(FrameNavigatedEventArgs e) { - Url = e.Url; - Name = e.Name; - Navigated?.Invoke(this, e); + EventHandler? navigatedHandlers; + lock (_stateLock) + { + Url = e.Url; + Name = e.Name; + navigatedHandlers = Navigated; + } + + navigatedHandlers?.Invoke(this, e); if (string.IsNullOrEmpty(e.Error)) { @@ -245,18 +265,10 @@ public async Task WaitForLoadStateAsync(LoadState? state = default, FrameWaitFor { waiter = SetupNavigationWaiter("frame.WaitForLoadStateAsync", options?.Timeout); - if (_loadStates.Contains(loadState)) + if (!await WaitForLoadStateEventAsync(waiter, loadState).ConfigureAwait(false)) { 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); - } } finally { @@ -297,55 +309,62 @@ await waiter.WaitForEventAsync(this, "LoadState", s => string? urlString, Func? urlFunc, Regex? urlRegex, - WaitUntilState? waitUntil) + WaitUntilState? waitUntil, + bool acceptCurrentUrl = false) { waitUntil ??= WaitUntilState.Load; string toUrl = !string.IsNullOrEmpty(urlString) ? $" to \"{urlString}\"" : string.Empty; + var urlMatch = new URLMatch() + { + glob = urlString, + re = urlRegex, + func = urlFunc, + baseURL = (Page as Page)?.Context.BaseURL, + }; waiter.Log($"waiting for navigation{toUrl} until \"{waitUntil}\""); - var navigatedEventTask = waiter.WaitForEventAsync( - this, - "Navigated", - e => + Func isNavigation = _ => + { + // Any failed navigation results in a rejection. + if (_.Error != null) { - // Any failed navigation results in a rejection. - if (e.Error != null) - { - return true; - } - - 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 true; + } - var navigatedEvent = await navigatedEventTask.ConfigureAwait(false); + waiter.Log($" navigated to \"{_.Url}\""); + return urlMatch.Match(_.Url); + }; - if (navigatedEvent.Error != null) + // The URL is read where the wait subscribes, so a navigation that lands after it is heard. It is matched + // after the lock is let go: a URL predicate is the caller's code, and must not hold up the connection. + string currentUrl; + (Task Task, Action Dispose) navigated; + lock (_stateLock) { - var ex = new PlaywrightException(navigatedEvent.Error); - await waiter.WaitForPromiseAsync(Task.FromException(ex)).ConfigureAwait(false); + currentUrl = Url; + navigated = waiter.GetWaitForEventTask(this, "Navigated", isNavigation); } - if (!_loadStates.Select(s => s.ToValueString()).Contains(waitUntil.Value.ToValueString())) + FrameNavigatedEventArgs? navigatedEvent = null; + if (acceptCurrentUrl && urlMatch.Match(currentUrl)) { - await waiter.WaitForEventAsync( - this, - "LoadState", - e => - { - waiter.Log($" \"{e}\" event fired"); - return e.ToValueString() == waitUntil.Value.ToValueString(); - }).ConfigureAwait(false); + navigated.Dispose(); } + else + { + navigatedEvent = await waiter.WaitForPromiseAsync(navigated.Task, navigated.Dispose).ConfigureAwait(false); + + if (navigatedEvent.Error != null) + { + var ex = new PlaywrightException(navigatedEvent.Error); + await waiter.WaitForPromiseAsync(Task.FromException(ex)).ConfigureAwait(false); + } + } + + await WaitForLoadStateEventAsync(waiter, waitUntil.Value).ConfigureAwait(false); - var request = navigatedEvent.NewDocument?.Request; + var request = navigatedEvent?.NewDocument?.Request; return request != null ? await waiter.WaitForPromiseAsync(request.FinalRequest.ResponseAsync()).ConfigureAwait(false) @@ -1058,49 +1077,47 @@ await SendMessageToServerAsync( } } - private Task WaitForURLAsync(string? urlString, Regex? urlRegex, Func? urlFunc, FrameWaitForURLOptions? options = default) + private async 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)) - { - if (options?.WaitUntil == WaitUntilState.Commit) + if (options?.WaitUntil == WaitUntilState.Commit && + new URLMatch() { - return Task.CompletedTask; - } - - return WaitForLoadStateAsync(ToLoadState(options?.WaitUntil), new() { Timeout = options?.Timeout }); + glob = urlString, + re = urlRegex, + func = urlFunc, + baseURL = (Page as Page)?.Context.BaseURL, + }.Match(Url)) + { + return; } - return WaitForNavigationAsync( - new() - { - UrlString = urlString, - UrlRegex = urlRegex, - UrlFunc = urlFunc, - Timeout = options?.Timeout, - WaitUntil = options?.WaitUntil, - }); + // A frame already at the URL has navigated there. Checking the URL here and then waiting for a navigation + // would miss one that lands in between, so the navigation wait checks it where it subscribes. + using var waiter = SetupNavigationWaiter("frame.WaitForNavigationAsync", options?.Timeout); + await WaitForNavigationInternalAsync(waiter, urlString, urlFunc, urlRegex, options?.WaitUntil, acceptCurrentUrl: true).ConfigureAwait(false); } - private LoadState? ToLoadState(WaitUntilState? waitUntilState) + // Waits for a load state unless the frame has it already, and says whether it waited. The check and the + // subscription are made under _stateLock, so a state that arrives between them is not recorded unheard. + private async Task WaitForLoadStateEventAsync(Waiter waiter, WaitUntilState loadState) { - if (waitUntilState == null) + (Task Task, Action Dispose) loadStateEvent; + lock (_stateLock) { - return null; + if (_loadStates.Contains(loadState)) + { + return false; + } + + loadStateEvent = waiter.GetWaitForEventTask(this, "LoadState", _ => + { + waiter.Log($" \"{_}\" event fired"); + return _ == loadState; + }); } - return waitUntilState switch - { - WaitUntilState.Load => Microsoft.Playwright.LoadState.Load, - WaitUntilState.DOMContentLoaded => Microsoft.Playwright.LoadState.DOMContentLoaded, - WaitUntilState.NetworkIdle => Microsoft.Playwright.LoadState.NetworkIdle, - _ => null, - }; + await waiter.WaitForPromiseAsync(loadStateEvent.Task, loadStateEvent.Dispose).ConfigureAwait(false); + return true; } private Waiter SetupNavigationWaiter(string @event, float? timeout)