Skip to content

fix(frame): subscribe before checking state in navigation waits #3384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Yury Semikhatsky (yury-s) wants to merge 2 commits into from
Closed

fix(frame): subscribe before checking state in navigation waits #3384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1493756
fix(frame): subscribe before checking state in navigation waits
yury-s Oct 15, 2026
6629ab4
fix(frame): drop the locks and keep the JS method shape
yury-s Oct 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/Playwright.Tests/PageWaitForUrlTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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("<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 });
}
}
}
100 changes: 60 additions & 40 deletions src/Playwright/Core/Frame.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,17 +245,22 @@ public async Task WaitForLoadStateAsync(LoadState? state = default, FrameWaitFor
{
waiter = SetupNavigationWaiter("frame.WaitForLoadStateAsync", options?.Timeout);

// Unlike the JS client, load states are updated on the transport thread, so subscribe

@pavelfeldman Pavel Feldman (pavelfeldman) Oct 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't afford this - everything else will fall apart. We should make sure events come back as a flow serialized with commands on the api thread. Otherwise all of our assumptions are invalid.

// 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<WaitUntilState>(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.WaitForEventAsync<WaitUntilState>(this, "LoadState", s =>
{
waiter.Log($" \"{s}\" event fired");
return s == loadState;
}).ConfigureAwait(false);
await waiter.WaitForPromiseAsync(loadStateTask, unsubscribe).ConfigureAwait(false);
}
}
finally
Expand Down Expand Up @@ -297,14 +302,22 @@ await waiter.WaitForEventAsync<WaitUntilState>(this, "LoadState", s =>
string? urlString,
Func<string, bool>? urlFunc,
Regex? urlRegex,
WaitUntilState? waitUntil)
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 navigatedEventTask = waiter.WaitForEventAsync<FrameNavigatedEventArgs>(
var urlMatch = new URLMatch()
{
glob = urlString,
re = urlRegex,
func = urlFunc,
baseURL = (Page as Page)?.Context.BaseURL,
};
var (navigatedEventTask, unsubscribe) = waiter.GetWaitForEventTask<FrameNavigatedEventArgs>(
this,
"Navigated",
e =>
Expand All @@ -316,36 +329,46 @@ await waiter.WaitForEventAsync<WaitUntilState>(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);

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<object>(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<object>(ex)).ConfigureAwait(false);
}
}

if (!_loadStates.Select(s => s.ToValueString()).Contains(waitUntil.Value.ToValueString()))
// Same as in WaitForLoadStateAsync: subscribe before checking the load state.
var (loadStateTask, unsubscribeLoadState) = waiter.GetWaitForEventTask<WaitUntilState>(
this,
"LoadState",
e =>
{
waiter.Log($" \"{e}\" event fired");
return e.ToValueString() == waitUntil.Value.ToValueString();
});
if (_loadStates.Select(s => s.ToValueString()).Contains(waitUntil.Value.ToValueString()))
{
await waiter.WaitForEventAsync<WaitUntilState>(
this,
"LoadState",
e =>
{
waiter.Log($" \"{e}\" event fired");
return e.ToValueString() == waitUntil.Value.ToValueString();
}).ConfigureAwait(false);
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)
Expand Down Expand Up @@ -1049,7 +1072,7 @@ await SendMessageToServerAsync(
}
}

private Task WaitForURLAsync(string? urlString, Regex? urlRegex, Func<string, bool>? urlFunc, FrameWaitForURLOptions? options = default)
private async Task WaitForURLAsync(string? urlString, Regex? urlRegex, Func<string, bool>? urlFunc, FrameWaitForURLOptions? options = default)
{
if (new URLMatch()
{
Expand All @@ -1059,18 +1082,15 @@ private Task WaitForURLAsync(string? urlString, Regex? urlRegex, Func<string, bo
baseURL = (Page as Page)?.Context.BaseURL,
}.Match(Url))
{
return WaitForLoadStateAsync(ToLoadState(options?.WaitUntil), new() { Timeout = options?.Timeout });
await WaitForLoadStateAsync(ToLoadState(options?.WaitUntil), new() { Timeout = options?.Timeout }).ConfigureAwait(false);
return;
}

return WaitForNavigationAsync(
new()
{
UrlString = urlString,
UrlRegex = urlRegex,
UrlFunc = urlFunc,
Timeout = options?.Timeout,
WaitUntil = options?.WaitUntil,
});
// 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)
Expand Down
Loading