Repository navigation
fix(frame): subscribe before checking state in navigation waits - #3384
Closed
Yury Semikhatsky (yury-s) wants to merge 2 commits into
Closed
Yury Semikhatsky (yury-s) wants to merge 2 commits into
Yury Semikhatsky (yury-s) wants to merge 2 commits into
Conversation
Yury Semikhatsky (yury-s)
marked this pull request as ready for review
October 15, 2026 17:21
| // 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<WaitUntilState>(this, "LoadState", s => |
There was a problem hiding this comment.
This is different from what we do in JS
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: #3364
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.
Yury Semikhatsky (yury-s)
force-pushed
the
fix-3364
branch
from
October 15, 2026 18:26
51ed3ab to
6629ab4
Compare
| { | ||
| waiter = SetupNavigationWaiter("frame.WaitForLoadStateAsync", options?.Timeout); | ||
|
|
||
| // Unlike the JS client, load states are updated on the transport thread, so subscribe |
There was a problem hiding this comment.
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.
Collaborator
|
Closing in favor of #3453. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Frameevents are raised on the transport thread, whileWaitForURLAsync,WaitForLoadStateAsyncandWaitForNavigationAsyncchecked the recorded state first and subscribed afterwards. Anavigatedorloadstateevent delivered in between was never seen again, and the wait ran to its timeout. The JS client does the same check-then-subscribe, but there nothing can be dispatched between the two.WaitForURLAsyncre-checks the URL inside the navigation wait once the subscription exists.WaitForURLAsync("**/b.html")right afterGotoAsync); it fails onmainin a fresh test host, where the check-to-subscribe window is widest.Fixes: #3364