diff --git a/src/Playwright.Tests/BrowserContextBasicTests.cs b/src/Playwright.Tests/BrowserContextBasicTests.cs index c0f352b7e..7df78b389 100644 --- a/src/Playwright.Tests/BrowserContextBasicTests.cs +++ b/src/Playwright.Tests/BrowserContextBasicTests.cs @@ -312,4 +312,18 @@ public async Task ShouldEmulateNavigatorOnLine() await context.SetOfflineAsync(false); Assert.True(await page.EvaluateAsync("() => 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); + } } diff --git a/src/Playwright.Tests/FrameHierarchyTests.cs b/src/Playwright.Tests/FrameHierarchyTests.cs index eeb6e8dcb..8b4f0b365 100644 --- a/src/Playwright.Tests/FrameHierarchyTests.cs +++ b/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); + } } diff --git a/src/Playwright/Core/Browser.cs b/src/Playwright/Core/Browser.cs index e5e2b874d..384785c58 100644 --- a/src/Playwright/Core/Browser.cs +++ b/src/Playwright/Core/Browser.cs @@ -57,7 +57,7 @@ internal Browser(ChannelOwner parent, string guid, BrowserInitializer initialize public event EventHandler? Context; - public IReadOnlyList Contexts => _contexts.ToArray(); + public IReadOnlyList 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) diff --git a/src/Playwright/Core/BrowserContext.cs b/src/Playwright/Core/BrowserContext.cs index 6f41d2d44..a67bbff77 100644 --- a/src/Playwright/Core/BrowserContext.cs +++ b/src/Playwright/Core/BrowserContext.cs @@ -159,7 +159,7 @@ public event EventHandler RequestFailed public IBrowser? Browser => _browser; - public IReadOnlyList Pages => _pages; + public IReadOnlyList Pages => Snapshot(_pages); internal Page? OwnerPage { get; set; } @@ -173,7 +173,7 @@ public event EventHandler RequestFailed public IAPIRequestContext APIRequest => _request; - public IReadOnlyList ServiceWorkers => _serviceWorkers; + public IReadOnlyList ServiceWorkers => Snapshot(_serviceWorkers); public IReadOnlyList BackgroundPages => new List(); @@ -245,7 +245,10 @@ internal override void OnMessage(string method, JsonElement serverParams) { var serviceWorker = serverParams.GetProperty("worker").ToObject(_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 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,15 +831,18 @@ private async Task UnrouteAsync(string? globMatch, Regex? reMatch, Func(); var remaining = new List(); - 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); @@ -833,7 +850,10 @@ private async Task UnrouteAsync(string? globMatch, Regex? reMatch, Func removed, List 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 removed, List> patterns; + lock (_connection.DispatchLock) + { + patterns = RouteHandler.PrepareInterceptionPatterns(_routes); + } await SendMessageToServerAsync( "setNetworkInterceptionPatterns", new Dictionary @@ -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 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 url, Action private Task RouteWebSocketAsync(string? globMatch, Regex? reMatch, Func? 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> patterns; + lock (_connection.DispatchLock) + { + patterns = WebSocketRouteHandler.PrepareInterceptionPatterns(_webSocketRoutes); + } await SendMessageToServerAsync("setWebSocketInterceptionPatterns", new Dictionary { ["patterns"] = patterns, diff --git a/src/Playwright/Core/BrowserType.cs b/src/Playwright/Core/BrowserType.cs index 18a81c90e..cdb4d6011 100644 --- a/src/Playwright/Core/BrowserType.cs +++ b/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()) + foreach (BrowserContext context in browser?.Snapshot(browser._contexts) ?? Array.Empty()) { - foreach (Page page in context._pages.ToArray()) + foreach (Page page in context.Snapshot(context._pages)) { page.OnClose(); } diff --git a/src/Playwright/Core/CDPSession.cs b/src/Playwright/Core/CDPSession.cs index 9f508fb5e..82f0dd64d 100644 --- a/src/Playwright/Core/CDPSession.cs +++ b/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; } } diff --git a/src/Playwright/Core/Frame.cs b/src/Playwright/Core/Frame.cs index ee9551cf4..bdfbc3f60 100644 --- a/src/Playwright/Core/Frame.cs +++ b/src/Playwright/Core/Frame.cs @@ -64,7 +64,7 @@ internal Frame(ChannelOwner parent, string guid, FrameInitializer initializer) : /// public event EventHandler? LoadState; - public IReadOnlyList