fix(threading): guard client-side state with a dispatch lock

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/FrameHierarchyTests.cs

@@ -209,4 +209,18 @@ 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);
+    }
 }

src/Playwright/Core/Browser.cs

@@ -57,7 +57,7 @@ internal Browser(ChannelOwner parent, string guid, BrowserInitializer initialize
 
     public event EventHandler<IBrowserContext>? Context;
 
-    public IReadOnlyList<IBrowserContext> Contexts => _contexts.ToArray();
+    public IReadOnlyList<IBrowserContext> Contexts => Snapshot(_contexts);
 
     public bool IsConnected { get; private set; }
 
@@ -281,7 +281,7 @@ internal void ConnectToBrowserType(BrowserType browserType, string? tracesDir)
         // This is why browser type is not wired up in the constructor, and instead this separate method is called later on.
         _browserType = browserType;
         _tracesDir = tracesDir;
-        foreach (var context in _contexts)
+        foreach (var context in Snapshot(_contexts))
         {
             context._tracing._tracesDir = this._tracesDir;
             context._request._tracing._tracesDir = this._tracesDir;
@@ -292,7 +292,10 @@ internal void ConnectToBrowserType(BrowserType browserType, string? tracesDir)
     private void DidCreateContext(BrowserContext context)
     {
         context._browser = this;
-        _contexts.Add(context);
+        lock (_connection.DispatchLock)
+        {
+            _contexts.Add(context);
+        }
         // Note: when connecting to a browser, initial contexts arrive before `_browserType` is set,
         // and will be configured later in `ConnectToBrowserType`.
         if (_browserType != null)

src/Playwright/Core/BrowserContext.cs

@@ -159,7 +159,7 @@ public event EventHandler<IRequest> RequestFailed
 
     public IBrowser? Browser => _browser;
 
-    public IReadOnlyList<IPage> Pages => _pages;
+    public IReadOnlyList<IPage> Pages => Snapshot(_pages);
 
     internal Page? OwnerPage { get; set; }
 
@@ -173,7 +173,7 @@ public event EventHandler<IRequest> RequestFailed
 
     public IAPIRequestContext APIRequest => _request;
 
-    public IReadOnlyList<IWorker> ServiceWorkers => _serviceWorkers;
+    public IReadOnlyList<IWorker> ServiceWorkers => Snapshot(_serviceWorkers);
 
     public IReadOnlyList<IPage> BackgroundPages => new List<IPage>();
 
@@ -245,7 +245,10 @@ internal override void OnMessage(string method, JsonElement serverParams)
                 {
                     var serviceWorker = serverParams.GetProperty("worker").ToObject<Worker>(_connection.DefaultJsonSerializerOptions)!;
                     ((Worker)serviceWorker).Context = this;
-                    _serviceWorkers.Add(serviceWorker);
+                    lock (_connection.DispatchLock)
+                    {
+                        _serviceWorkers.Add(serviceWorker);
+                    }
                     ServiceWorker?.Invoke(this, serviceWorker);
                     break;
                 }
@@ -729,7 +732,7 @@ internal async Task OnRouteAsync(Route route)
     {
         route._context = this;
         var page = route._request.SafePage;
-        var routeHandlers = _routes.ToArray();
+        var routeHandlers = Snapshot(_routes);
         foreach (var routeHandler in routeHandlers)
         {
             // If the page or the context was closed we stall all requests right away.
@@ -741,16 +744,24 @@ internal async Task OnRouteAsync(Route route)
             {
                 continue;
             }
-            if (!_routes.Contains(routeHandler))
-            {
-                continue;
-            }
-            if (routeHandler.WillExpire())
+            bool noRoutesLeft;
+            lock (_connection.DispatchLock)
             {
-                _routes.Remove(routeHandler);
+                if (!_routes.Contains(routeHandler))
+                {
+                    continue;
+                }
+                if (routeHandler.WillExpire())
+                {
+                    _routes.Remove(routeHandler);
+                }
+                // Counted here, under the lock, rather than in HandleAsync: two matching requests
+                // handled concurrently must not both see the count before the other's increment.
+                routeHandler.HandledCount++;
+                noRoutesLeft = _routes.Count == 0;
             }
             var handled = await routeHandler.HandleAsync(route).ConfigureAwait(false);
-            if (_routes.Count == 0)
+            if (noRoutesLeft)
             {
                 UpdateInterceptionAsync().IgnoreException();
             }
@@ -773,7 +784,7 @@ internal async Task OnRouteAsync(Route route)
 
     internal async Task OnWebSocketRouteAsync(WebSocketRoute webSocketRoute)
     {
-        var routeHandler = _webSocketRoutes.Find(route => route.Matches(webSocketRoute.Url));
+        var routeHandler = Array.Find(Snapshot(_webSocketRoutes), route => route.Matches(webSocketRoute.Url));
         if (routeHandler != null)
         {
             await routeHandler.HandleAsync(webSocketRoute).ConfigureAwait(false);
@@ -805,7 +816,10 @@ private async Task<IAsyncDisposable> RouteAsync(string? globMatch, Regex? reMatc
             Handler = handler,
             Times = options?.Times,
         };
-        _routes.Insert(0, setting);
+        lock (_connection.DispatchLock)
+        {
+            _routes.Insert(0, setting);
+        }
         await UpdateInterceptionAsync().ConfigureAwait(false);
         return new DisposableStub(async () =>
         {
@@ -817,23 +831,29 @@ private async Task UnrouteAsync(string? globMatch, Regex? reMatch, Func<string,
     {
         var removed = new List<RouteHandler>();
         var remaining = new List<RouteHandler>();
-        foreach (var routeHandler in _routes)
+        lock (_connection.DispatchLock)
         {
-            if (routeHandler.urlMatcher.Equals(globMatch, reMatch, funcMatch, BaseURL, false) && (handler == null || routeHandler.Handler == handler))
+            foreach (var routeHandler in _routes)
             {
-                removed.Add(routeHandler);
-            }
-            else
-            {
-                remaining.Add(routeHandler);
+                if (routeHandler.urlMatcher.Equals(globMatch, reMatch, funcMatch, BaseURL, false) && (handler == null || routeHandler.Handler == handler))
+                {
+                    removed.Add(routeHandler);
+                }
+                else
+                {
+                    remaining.Add(routeHandler);
+                }
             }
         }
         await UnrouteInternalAsync(removed, remaining, UnrouteBehavior.Default).ConfigureAwait(false);
     }
 
     private async Task UnrouteInternalAsync(List<RouteHandler> removed, List<RouteHandler> remaining, UnrouteBehavior? behavior)
     {
-        _routes = remaining;
+        lock (_connection.DispatchLock)
+        {
+            _routes = remaining;
+        }
         if (behavior != null && behavior != UnrouteBehavior.Default)
         {
             var tasks = removed.Select(routeHandler => routeHandler.StopAsync(behavior.Value));
@@ -844,7 +864,11 @@ private async Task UnrouteInternalAsync(List<RouteHandler> removed, List<RouteHa
 
     private async Task UpdateInterceptionAsync()
     {
-        var patterns = RouteHandler.PrepareInterceptionPatterns(_routes);
+        List<Dictionary<string, object>> patterns;
+        lock (_connection.DispatchLock)
+        {
+            patterns = RouteHandler.PrepareInterceptionPatterns(_routes);
+        }
         await SendMessageToServerAsync(
             "setNetworkInterceptionPatterns",
             new Dictionary<string, object?>
@@ -858,7 +882,10 @@ internal void OnClose()
         ClosingOrClosed = true;
         if (Browser != null)
         {
-            ((Browser)Browser)._contexts.Remove(this);
+            lock (_connection.DispatchLock)
+            {
+                ((Browser)Browser)._contexts.Remove(this);
+            }
             ((Browser)Browser)._browserType.Playwright._selectors._contextsForSelectors.Remove(this);
         }
 
@@ -884,7 +911,10 @@ internal void OnClose()
     private void Channel_OnPage(object sender, Page page)
     {
         page.Context = this;
-        _pages.Add(page);
+        lock (_connection.DispatchLock)
+        {
+            _pages.Add(page);
+        }
         Page?.Invoke(this, page);
 
         if (page.Opener?.IsClosed == false)
@@ -895,7 +925,12 @@ private void Channel_OnPage(object sender, Page page)
 
     private void Channel_BindingCall(BindingCall bindingCall)
     {
-        if (_bindings.TryGetValue(bindingCall.Name, out var binding))
+        Delegate? binding;
+        lock (_connection.DispatchLock)
+        {
+            _bindings.TryGetValue(bindingCall.Name, out binding);
+        }
+        if (binding != null)
         {
             _ = bindingCall.CallAsync(binding);
         }
@@ -905,20 +940,23 @@ private void Channel_BindingCall(BindingCall bindingCall)
 
     private async Task<IAsyncDisposable> ExposeBindingAsync(string name, Delegate callback)
     {
-        foreach (var page in _pages)
+        lock (_connection.DispatchLock)
         {
-            if (page.Bindings.ContainsKey(name))
+            foreach (var page in _pages)
             {
-                throw new PlaywrightException($"Function \"{name}\" has been already registered in one of the pages");
+                if (page.Bindings.ContainsKey(name))
+                {
+                    throw new PlaywrightException($"Function \"{name}\" has been already registered in one of the pages");
+                }
             }
-        }
 
-        if (_bindings.ContainsKey(name))
-        {
-            throw new PlaywrightException($"Function \"{name}\" has been already registered");
-        }
+            if (_bindings.ContainsKey(name))
+            {
+                throw new PlaywrightException($"Function \"{name}\" has been already registered");
+            }
 
-        _bindings.Add(name, callback);
+            _bindings.Add(name, callback);
+        }
 
         var result = await SendMessageToServerAsync(
             "exposeBinding",
@@ -959,17 +997,25 @@ public async Task RouteFromHARAsync(string har, BrowserContextRouteFromHAROption
             UrlRegex = options?.UrlRegex,
             UrlString = options?.UrlString,
         }).ConfigureAwait(false);
-        _harRouters.Add(harRouter);
+        lock (_connection.DispatchLock)
+        {
+            _harRouters.Add(harRouter);
+        }
         await harRouter.AddContextRouteAsync(this).ConfigureAwait(false);
     }
 
     private void DisposeHarRouters()
     {
-        foreach (var router in _harRouters)
+        HarRouter[] routers;
+        lock (_connection.DispatchLock)
+        {
+            routers = _harRouters.ToArray();
+            _harRouters.Clear();
+        }
+        foreach (var router in routers)
         {
             router.Dispose();
         }
-        _harRouters.Clear();
     }
 
     [MethodImpl(MethodImplOptions.NoInlining)]
@@ -986,7 +1032,7 @@ public Task RouteWebSocketAsync(Func<string, bool> url, Action<IWebSocketRoute>
 
     private Task RouteWebSocketAsync(string? globMatch, Regex? reMatch, Func<string, bool>? funcMatch, Delegate handler)
     {
-        _webSocketRoutes.Insert(0, new WebSocketRouteHandler()
+        var routeHandler = new WebSocketRouteHandler()
         {
             urlMatcher = new URLMatch()
             {
@@ -997,13 +1043,21 @@ private Task RouteWebSocketAsync(string? globMatch, Regex? reMatch, Func<string,
                 isWebSocketUrl = true,
             },
             Handler = handler,
-        });
+        };
+        lock (_connection.DispatchLock)
+        {
+            _webSocketRoutes.Insert(0, routeHandler);
+        }
         return UpdateWebSocketInterceptionAsync();
     }
 
     private async Task UpdateWebSocketInterceptionAsync()
     {
-        var patterns = WebSocketRouteHandler.PrepareInterceptionPatterns(_webSocketRoutes);
+        List<Dictionary<string, object>> patterns;
+        lock (_connection.DispatchLock)
+        {
+            patterns = WebSocketRouteHandler.PrepareInterceptionPatterns(_webSocketRoutes);
+        }
         await SendMessageToServerAsync("setWebSocketInterceptionPatterns", new Dictionary<string, object?>
         {
             ["patterns"] = patterns,

src/Playwright/Core/BrowserType.cs

@@ -187,9 +187,9 @@ void ClosePipe()
         void OnPipeClosed(string? reason = null)
         {
             // Emulate all pages, contexts and the browser closing upon disconnect.
-            foreach (BrowserContext context in browser?._contexts.ToArray() ?? Array.Empty<BrowserContext>())
+            foreach (BrowserContext context in browser?.Snapshot(browser._contexts) ?? Array.Empty<BrowserContext>())
             {
-                foreach (Page page in context._pages.ToArray())
+                foreach (Page page in context.Snapshot(context._pages))
                 {
                     page.OnClose();
                 }

src/Playwright/Core/CDPSession.cs

@@ -73,23 +73,24 @@ internal override void OnMessage(string method, JsonElement serverParams)
 
     private void OnCDPEvent(string name, JsonElement? @params)
     {
-        if (_cdpSessionEvents.TryGetValue(name, out var cdpNamedEvent))
+        CDPSessionEvent? cdpNamedEvent;
+        lock (_connection.DispatchLock)
         {
-            cdpNamedEvent.RaiseEvent(@params);
+            _cdpSessionEvents.TryGetValue(name, out cdpNamedEvent);
         }
+        cdpNamedEvent?.RaiseEvent(@params);
     }
 
     [MethodImpl(MethodImplOptions.NoInlining)]
     public ICDPSessionEvent Event(string eventName)
     {
-        if (_cdpSessionEvents.TryGetValue(eventName, out var cdpNamedEvent))
+        lock (_connection.DispatchLock)
         {
-            return cdpNamedEvent;
-        }
-        else
-        {
-            cdpNamedEvent = new(eventName);
-            _cdpSessionEvents.Add(eventName, cdpNamedEvent);
+            if (!_cdpSessionEvents.TryGetValue(eventName, out var cdpNamedEvent))
+            {
+                cdpNamedEvent = new(eventName);
+                _cdpSessionEvents.Add(eventName, cdpNamedEvent);
+            }
             return cdpNamedEvent;
         }
     }

src/Playwright/Core/Frame.cs

@@ -64,7 +64,7 @@ internal Frame(ChannelOwner parent, string guid, FrameInitializer initializer) :
     /// </summary>
     public event EventHandler<WaitUntilState>? LoadState;
 
-    public IReadOnlyList<IFrame> ChildFrames => _childFrames;
+    public IReadOnlyList<IFrame> ChildFrames => Snapshot(_childFrames);
 
     public string Name { get; internal set; }