fix(threading): run message dispatch and library code on one dispatcher thread

CLAUDE.md

@@ -62,6 +62,13 @@ The tests are NUnit tests in `src/Playwright.Tests/`, one class per upstream spe
 - New channel object types require: enum entry in `ChannelOwnerType.cs`, case in `Connection.cs`, initializer in `Transport/Protocol/Generated/`, and a `Core/` class.
 - Public APIs should use .NET standard types (e.g. `IAsyncDisposable`) not custom Playwright types. Internal helpers (e.g. `Disposable` class in `Core/`) stay internal.
 
+### Threading model
+The client mirrors the JS event loop: each connection has one dispatcher thread (`Transport/Dispatcher.cs`) that handles driver messages and runs all library code, one item at a time. Read [docs/threading.md](docs/threading.md) before changing code in `Core/` or `Transport/`; it has the model and the rules of thumb. The short version:
+- A public async method is a single `SendMessageToServerAsync` call, or wraps its whole body in `WrapApiCallAsync(async () => ...)`.
+- State is touched only on the dispatcher thread. A `TaskCompletionSource` is completed with `Dispatcher.Complete`/`Fail`, and code that awaited something else (a user callback, a timeout, I/O) calls `await _connection.Dispatcher.SwitchTo()` before it touches state.
+- Lists that getters hand out are copy-on-write (`Helpers/CopyOnWrite.cs`), and nothing blocks on the dispatcher thread.
+- `ThreadingModelTests.cs` and `DispatcherTests.cs` pin the rules.
+
 ## Commit Convention
 
 Before committing, run `dotnet format ./src/ -v:diag` and fix style issues.

docs/threading.md

@@ -0,0 +1,56 @@
+# Threading model
+
+The JS client runs on one event loop: a driver message is handled synchronously, and the promise continuations it resolves run as microtasks before the next message. Library code between two awaits can therefore check a state and subscribe to an event without anything happening in between. The .NET client keeps the JS method bodies, so it restores that property with one **dispatcher thread** per connection (`src/Playwright/Transport/Dispatcher.cs`).
+
+## How it works
+
+- **One thread, one item at a time.** The dispatcher thread runs driver messages (`Connection.Dispatch`) and all library code. The transport reads and parses on the thread pool and posts each message to the dispatcher. A connection that `BrowserType.ConnectAsync` creates shares the dispatcher of the connection that owns its pipe.
+- **API calls hop on and off.** `Connection.WrapApiCallAsync` posts the body of a public call to the dispatcher, and returns the result on the thread pool. The caller's continuation never runs on the dispatcher thread, and a `SynchronizationContext` that the caller captured is honored as with any other `Task`.
+- **Microtasks.** A `TaskCompletionSource` that library code awaits is completed with `Dispatcher.Complete` or `Dispatcher.Fail`: after the current item, before the next one. The awaiting code continues inline on the dispatcher thread, because every library `await` uses `ConfigureAwait(false)` and the thread has no `SynchronizationContext`. So a call resumes once the message that completed it has been handled in full: `Page.Load` has fired when `WaitForLoadStateAsync` returns.
+- **Event accessors update a snapshot.** `+=` and `-=` run on the caller's thread and replace the handler list atomically (`ChannelOwner.UpdateEventHandler`), so the dispatcher thread raises either the old list or the new one. For the events that the server sends only while somebody listens, the dispatcher thread alone tells the server, from the handlers as they are when it runs.
+- **Getters read snapshots.** Synchronous getters run on the caller's thread. Scalars (`Url`, `IsClosed`) are single reads. Lists (`Pages`, `Frames`, `ChildFrames`, `Workers`, `Contexts`) are copy-on-write through `Helpers/CopyOnWrite.cs`: a published list is never mutated.
+
+Where user code runs:
+
+| User code | Thread |
+|---|---|
+| Event handler, route handler, binding, locator handler: the synchronous part | dispatcher |
+| The same handler after it awaited a Playwright call or anything else | thread pool |
+| Predicate of a wait (`WaitForRequestAsync(r => ...)`, `WaitForURLAsync(url => ...)`) | dispatcher |
+| Action of `RunAndWaitFor*Async` | thread pool |
+| Code after `await` of any Playwright call | thread pool, or the caller's `SynchronizationContext` |
+
+## Rules of thumb
+
+1. **Wrap public async methods.** A public async method is either a single `SendMessageToServerAsync` call plus parsing of its result, or its whole body is inside `WrapApiCallAsync(async () => ...)`. Private helpers that a public method delegates to are wrapped as well; a nested wrap is inline and free. Put the wrap on the signature line and keep the body at the indentation of a method body, so that wrapping a method does not re-indent it:
+
+   ```csharp
+   public Task CloseAsync(PageCloseOptions? options = default) => WrapApiCallAsync(async () =>
+   {
+       ...
+   });
+   ```
+2. **Touch state only on the dispatcher thread.** Fields such as `_routes`, `_loadStates`, `_pages` and `Bindings` are read and written there. Put `_connection.Dispatcher.AssertCurrent()` next to new state access; it fails in Debug builds, which is what the tests run.
+3. **Complete through the dispatcher.** Use `Dispatcher.Complete` and `Dispatcher.Fail`, never `SetResult` or `SetException`, on a source that library code awaits. Do not create such a source with `RunContinuationsAsynchronously`.
+4. **Switch back after a foreign await.** `await _connection.Dispatcher.SwitchTo()` before touching state after awaiting something that the dispatcher did not complete:
+   - a user callback (put the switch in a `finally`, the callback may throw),
+   - a timer or a timeout (`Task.Delay`, `WithTimeout`), file or pipe I/O,
+   - a `Task` that another call started and cached,
+   - any library call in a flow that a message started (`OnRouteAsync`, `BindingCall.CallAsync`). Such a flow is not inside an API call, so the calls it makes are outermost and return on the thread pool.
+5. **Synchronous API never waits for the dispatcher.** It may be called from a handler that runs on it. Read a snapshot, or hand work to the dispatcher with `Dispatcher.RunOrPost`: it is ordered with the caller's other calls.
+6. **Hand out copy-on-write lists.** A collection that a getter returns, or that synchronous API enumerates, is replaced with `CopyOnWrite.Add` and `CopyOnWrite.Remove`, never mutated in place.
+7. **Never block the dispatcher thread.** No `.Result`, `.Wait()` or `GetAwaiter().GetResult()` on a Playwright call in library code that may run there.
+8. **Keep `ConfigureAwait(false)` everywhere**, and pass `TaskScheduler.Default` to `ContinueWith` and `StartNew`.
+9. **Clear the API zone around user code.** A call is nested, and stays on the dispatcher thread, when `Connection.ApiZone` is set. User code that runs inside a wrapped body must not inherit it; see `Waiter.CancelWaitOnExceptionAsync`.
+
+## What users can rely on
+
+- Events that the driver sent before a reply have been handled when the call returns.
+- Blocking is safe everywhere except in the synchronous part of a handler: a synchronous wait on a Playwright call there stalls the connection until it times out.
+- A handler that throws closes the connection, as before.
+- `+=` and `-=` on an event take effect at once, as with any C# event. After `-=` has returned, the handler is not called for an event that is raised later; only an event that is being raised on the dispatcher thread at that very moment may still reach it.
+- Arguments are read when the call runs on the dispatcher thread, shortly after it was made. Do not mutate an options object or a buffer that a call in flight still uses.
+
+## Tests
+
+`DispatcherTests.cs` pins the dispatcher itself. `ThreadingModelTests.cs` pins the rules above end to end; add a case there when a rule changes.

src/Playwright.Tests/BrowserContextBasicTests.cs

@@ -312,4 +312,18 @@ public async Task ShouldEmulateNavigatorOnLine()
         await context.SetOfflineAsync(false);
         Assert.True(await page.EvaluateAsync<bool>("() => window.navigator.onLine"));
     }
+
+    [PlaywrightTest]
+    public async Task ShouldAllowOpeningPagesWhileEnumeratingPages()
+    {
+        // Pages used to hand out the live list, so a page opened from inside the loop made the
+        // next MoveNext throw "Collection was modified".
+        await using var context = await Browser.NewContextAsync();
+        await context.NewPageAsync();
+        foreach (var page in context.Pages)
+        {
+            await context.NewPageAsync();
+        }
+        Assert.AreEqual(2, context.Pages.Count);
+    }
 }

src/Playwright.Tests/DispatcherTests.cs

@@ -0,0 +1,247 @@
+/*
+ * MIT License
+ *
+ * Copyright (c) Microsoft Corporation.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and / or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+using Microsoft.Playwright.Transport;
+
+namespace Microsoft.Playwright.Tests;
+
+/// <summary>
+/// The dispatcher is the thread on which a connection handles driver messages and runs library
+/// code; see <see cref="Dispatcher"/>. These tests pin the three rules the rest of the client
+/// depends on: items run on that thread in order, a completion source completed through the
+/// dispatcher resumes its awaiter after the current item and on the dispatcher thread, and the
+/// two hops (onto the thread and off it) land where they say.
+/// </summary>
+public class DispatcherTests
+{
+    private Dispatcher _dispatcher = null!;
+
+    private static bool OnDispatcherThread => Thread.CurrentThread.Name == Dispatcher.ThreadName;
+
+    [SetUp]
+    public void SetUp() => _dispatcher = new Dispatcher();
+
+    [TearDown]
+    public void TearDown() => _dispatcher.Dispose();
+
+    [Test]
+    public async Task PostRunsItemsOnTheDispatcherThreadInOrder()
+    {
+        var log = new List<string>();
+        var done = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);
+        for (int i = 0; i < 100; i++)
+        {
+            int n = i;
+            _dispatcher.Post(() =>
+            {
+                Assert.True(_dispatcher.IsCurrent);
+                Assert.True(OnDispatcherThread);
+                log.Add($"item{n}");
+            });
+        }
+        _dispatcher.Post(() => done.SetResult(true));
+        await done.Task;
+        Assert.AreEqual(Enumerable.Range(0, 100).Select(i => $"item{i}").ToList(), log);
+    }
+
+    [Test]
+    public async Task SwitchToLandsOnTheDispatcherThreadAndIsSynchronousThere()
+    {
+        Assert.False(_dispatcher.IsCurrent);
+        Assert.False(_dispatcher.SwitchTo().IsCompleted);
+        await _dispatcher.SwitchTo();
+        Assert.True(_dispatcher.IsCurrent);
+        Assert.True(_dispatcher.SwitchTo().IsCompleted);
+        // A second hop from the dispatcher thread is a no-op.
+        await _dispatcher.SwitchTo();
+        Assert.True(_dispatcher.IsCurrent);
+        await Dispatcher.ExitToThreadPool();
+        Assert.False(_dispatcher.IsCurrent);
+    }
+
+    [Test]
+    public async Task SwitchToFlowsTheExecutionContext()
+    {
+        var local = new AsyncLocal<string>();
+        local.Value = "from the caller";
+        await _dispatcher.SwitchTo();
+        Assert.AreEqual("from the caller", local.Value);
+        await Dispatcher.ExitToThreadPool();
+        Assert.AreEqual("from the caller", local.Value);
+    }
+
+    [Test]
+    public async Task CompleteRunsTheContinuationAfterTheCurrentItemOnTheDispatcherThread()
+    {
+        // The library's await: ConfigureAwait(false) on a source that the dispatcher completes.
+        var tcs = new TaskCompletionSource<int>();
+        var itemFinished = false;
+        var continuation = Task.Run(async () =>
+        {
+            var value = await tcs.Task.ConfigureAwait(false);
+            Assert.True(itemFinished, "the continuation must run after the item that completed the source");
+            Assert.True(_dispatcher.IsCurrent, "the continuation must run inline on the dispatcher thread");
+            return value;
+        });
+        // Give the awaiter time to subscribe before the source is completed.
+        await Task.Delay(50);
+        _dispatcher.Post(() =>
+        {
+            _dispatcher.Complete(tcs, 42);
+            Assert.False(tcs.Task.IsCompleted, "completion is deferred until the item has finished");
+            itemFinished = true;
+        });
+        Assert.AreEqual(42, await continuation);
+    }
+
+    [Test]
+    public async Task MicrotasksRunBeforeTheNextItem()
+    {
+        var log = new List<string>();
+        var tcs1 = new TaskCompletionSource<bool>();
+        var tcs2 = new TaskCompletionSource<bool>();
+        var done = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);
+        // Subscribe from the dispatcher thread, so that the continuations run inline there.
+        await _dispatcher.SwitchTo();
+        _ = tcs1.Task.ContinueWith(
+            _ =>
+            {
+                log.Add("continuation1");
+                // A microtask queued by a microtask still runs before the next item.
+                _dispatcher.Complete(tcs2, true);
+            },
+            TaskContinuationOptions.ExecuteSynchronously);
+        _ = tcs2.Task.ContinueWith(_ => log.Add("continuation2"), TaskContinuationOptions.ExecuteSynchronously);
+        _dispatcher.Post(() =>
+        {
+            log.Add("item1");
+            _dispatcher.Complete(tcs1, true);
+            log.Add("item1 end");
+        });
+        _dispatcher.Post(() => log.Add("item2"));
+        _dispatcher.Post(() => done.SetResult(true));
+        await Dispatcher.ExitToThreadPool();
+        await done.Task;
+        Assert.AreEqual(new[] { "item1", "item1 end", "continuation1", "continuation2", "item2" }, log);
+    }
+
+    [Test]
+    public async Task CompleteFromAnotherThreadResumesTheAwaiterOnTheDispatcherThread()
+    {
+        var tcs = new TaskCompletionSource<bool>();
+        var continuation = Task.Run(async () =>
+        {
+            await tcs.Task.ConfigureAwait(false);
+            return _dispatcher.IsCurrent;
+        });
+        await Task.Delay(50);
+        Assert.False(_dispatcher.IsCurrent);
+        _dispatcher.Complete(tcs, true);
+        Assert.True(await continuation);
+    }
+
+    [Test]
+    public async Task FailFaultsTheSourceAfterTheCurrentItem()
+    {
+        var tcs = new TaskCompletionSource<bool>();
+        var continuation = Task.Run(async () =>
+        {
+            try
+            {
+                await tcs.Task.ConfigureAwait(false);
+                return null;
+            }
+            catch (InvalidOperationException ex)
+            {
+                Assert.True(_dispatcher.IsCurrent);
+                return ex.Message;
+            }
+        });
+        await Task.Delay(50);
+        _dispatcher.Post(() => _dispatcher.Fail(tcs, new InvalidOperationException("boom")));
+        Assert.AreEqual("boom", await continuation);
+    }
+
+    [Test]
+    public async Task InlineContinuationChainStaysOnTheDispatcherThread()
+    {
+        // Nested library methods awaiting each other with ConfigureAwait(false): every level
+        // resumes on the dispatcher thread when the innermost source is completed there.
+        var tcs = new TaskCompletionSource<bool>();
+        var threads = new List<bool>();
+
+        async Task Level(int depth)
+        {
+            if (depth == 0)
+            {
+                await tcs.Task.ConfigureAwait(false);
+            }
+            else
+            {
+                await Level(depth - 1).ConfigureAwait(false);
+            }
+            threads.Add(_dispatcher.IsCurrent);
+        }
+
+        var chain = Task.Run(() => Level(20));
+        await Task.Delay(50);
+        _dispatcher.Post(() => _dispatcher.Complete(tcs, true));
+        await chain;
+        Assert.AreEqual(21, threads.Count);
+        Assert.True(threads.All(onDispatcher => onDispatcher));
+    }
+
+    [Test]
+    public async Task ExitToThreadPoolNeverCompletesSynchronously()
+    {
+        Assert.False(Dispatcher.ExitToThreadPool().IsCompleted);
+        var thread = Thread.CurrentThread;
+        await Dispatcher.ExitToThreadPool();
+        Assert.True(Thread.CurrentThread.IsThreadPoolThread);
+        Assert.False(_dispatcher.IsCurrent);
+    }
+
+    [Test]
+    public async Task PostAfterDisposeStillRunsTheAction()
+    {
+        _dispatcher.Dispose();
+        var ran = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);
+        _dispatcher.Post(() => ran.SetResult(true));
+        Assert.True(await ran.Task.WaitAsync(TimeSpan.FromSeconds(5)));
+        // The hop still resolves, on some thread, so that a call made after close can throw.
+        await _dispatcher.SwitchTo();
+    }
+
+    [Test]
+    public async Task UnhandledExceptionInAnItemIsReportedAndTheThreadSurvives()
+    {
+        Exception reported = null;
+        _dispatcher.UnhandledException += (_, ex) => reported = ex;
+        var done = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);
+        _dispatcher.Post(() => throw new InvalidOperationException("boom"));
+        _dispatcher.Post(() => done.SetResult(true));
+        await done.Task;
+        Assert.AreEqual("boom", reported?.Message);
+    }
+}

src/Playwright.Tests/FrameHierarchyTests.cs

@@ -209,4 +209,24 @@ await Page.EvaluateAsync(@"() => {
         Assert.False(frame2.IsDetached);
         Assert.That(frame1, Is.Not.EqualTo(frame2));
     }
+
+    [PlaywrightTest]
+    public async Task ShouldAllowAttachingFramesWhileEnumeratingChildFrames()
+    {
+        // ChildFrames used to hand out the live list, so a frame attached from inside the loop
+        // made the next MoveNext throw "Collection was modified".
+        await Page.GotoAsync(Server.EmptyPage);
+        await FrameUtils.AttachFrameAsync(Page, "frame1", Server.EmptyPage);
+        foreach (var frame in Page.MainFrame.ChildFrames)
+        {
+            await FrameUtils.AttachFrameAsync(Page, "frame2", Server.EmptyPage);
+        }
+        Assert.AreEqual(2, Page.MainFrame.ChildFrames.Count);
+        int next = 3;
+        foreach (var frame in Page.Frames)
+        {
+            await FrameUtils.AttachFrameAsync(Page, $"frame{next++}", Server.EmptyPage);
+        }
+        Assert.AreEqual(6, Page.Frames.Count);
+    }
 }

src/Playwright.Tests/PageRunAndWaitForResponseTests.cs

@@ -109,4 +109,18 @@ public async Task ShouldWorkWithUrlMatch()
 
         Assert.AreEqual(Server.Prefix + "/digits/1.png", response.Url);
     }
+
+    // See https://github.com/microsoft/playwright-dotnet/pull/3256
+    [PlaywrightTest]
+    public async Task ShouldNotDeadlockWhenBlockingAfterTheWait()
+    {
+        // The code after the wait used to run inside the handler of the `response` event, on the
+        // thread that delivers messages, so a synchronous wait there never got its reply.
+        await Page.GotoAsync(Server.EmptyPage);
+        var response = await Page.RunAndWaitForResponseAsync(() => Page.EvaluateAsync<string>(@"() => {
+                    fetch('/digits/1.png');
+                }"), Server.Prefix + "/digits/1.png");
+        Assert.AreEqual(Server.Prefix + "/digits/1.png", response.Url);
+        Assert.True(Task.Run(() => Page.GotoAsync(Server.EmptyPage)).Wait(10_000));
+    }
 }