Ensure existing targets are attached to pages (#2670)

* Ensure existing targets are attached to pages * cr * Update lib/PuppeteerSharp/Cdp/CdpPage.cs Co-authored-by: Jonas Nyrup <jnyrup@users.noreply.github.com> * Don't fail close on dispose * Don't close pages from a connection on Dispose * change some browser disconnection stuff * Update lib/PuppeteerSharp/Browser.cs * Update lib/PuppeteerSharp/Cdp/CdpBrowser.cs * Update lib/PuppeteerSharp/Cdp/CdpBrowser.cs * Update lib/PuppeteerSharp/Cdp/CdpBrowser.cs * Add empty line --------- Co-authored-by: Jonas Nyrup <jnyrup@users.noreply.github.com>
hardkoded · Jul 3, 2024 · eca1640 · danbopes · Aug 7, 2024 · kblok
1 parent 5e9f4a5
commit eca1640
Show file tree

Hide file tree

Showing 12 changed files with 134 additions and 12 deletions.
diff --git a/lib/PuppeteerSharp.Nunit/TestExpectations/TestExpectations.upstream.json b/lib/PuppeteerSharp.Nunit/TestExpectations/TestExpectations.upstream.json
@@ -2943,6 +2943,20 @@
     "expectations": ["FAIL"],
     "comment": "TODO: add a comment explaining why this expectation is required (include links to issues)"
   },
+  {
+    "testIdPattern": "[oopif.spec] OOPIF should recover cross-origin frames on reconnect",
+    "platforms": ["darwin", "linux", "win32"],
+    "parameters": ["chrome", "webDriverBiDi"],
+    "expectations": ["FAIL"],
+    "comment": "See https://github.com/GoogleChromeLabs/chromium-bidi/issues/2362"
+  },
+  {
+    "testIdPattern": "[oopif.spec] OOPIF should recover cross-origin frames on reconnect",
+    "platforms": ["darwin", "linux", "win32"],
+    "parameters": ["firefox", "webDriverBiDi"],
+    "expectations": ["FAIL"],
+    "comment": "Firefox does not support multiple sessions in BiDi."
+  },
   {
     "testIdPattern": "[oopif.spec] OOPIF should support lazy OOP frames",
     "platforms": ["darwin", "linux", "win32"],

diff --git a/lib/PuppeteerSharp.Tests/OOPIFTests/OOPIFTests.cs b/lib/PuppeteerSharp.Tests/OOPIFTests/OOPIFTests.cs
@@ -104,6 +104,34 @@ await FrameUtils.AttachFrameAsync(
             StringAssert.Contains("frame.html", await frame2.EvaluateExpressionAsync<string>("document.location.href"));
         }
 
+        [Test, Retry(2), PuppeteerTest("oopif.spec", "OOPIF", "should recover cross-origin frames on reconnect")]
+        public async Task ShouldRecoverCrossOriginFramesOnReconnect()
+        {
+            await Page.GoToAsync(TestConstants.EmptyPage);
+            var frame1Task = Page.WaitForFrameAsync((frame) => frame != Page.MainFrame && frame.ParentFrame == Page.MainFrame);
+            var frame2Task = Page.WaitForFrameAsync((frame) => frame != Page.MainFrame && frame.ParentFrame != Page.MainFrame);
+
+            await FrameUtils.AttachFrameAsync(
+                Page,
+                "frame1",
+                TestConstants.CrossProcessHttpPrefix + "/frames/one-frame.html"
+            );
+
+            await Task.WhenAll(frame1Task, frame2Task);
+            var dump1 = await FrameUtils.DumpFramesAsync(Page.MainFrame);
+
+            await using var browserTwo = await Puppeteer.ConnectAsync(new ConnectOptions()
+            {
+                BrowserWSEndpoint = Browser.WebSocketEndpoint,
+            });
+
+            var pages = await browserTwo.PagesAsync();
+            var emptyPages = pages.Where(page => page.Url == TestConstants.EmptyPage).ToArray();
+            Assert.AreEqual(1, emptyPages.Length);
+            var dump2 = await FrameUtils.DumpFramesAsync(emptyPages[0].MainFrame);
+            Assert.AreEqual(dump1, dump2);
+        }
+
         [Test, Retry(2), PuppeteerTest("oopif.spec", "OOPIF", "should support OOP iframes getting detached")]
         public async Task ShouldSupportOopIframesGettingDetached()
         {

diff --git a/lib/PuppeteerSharp/Browser.cs b/lib/PuppeteerSharp/Browser.cs
@@ -174,7 +174,16 @@ public void Dispose()
         /// <returns>ValueTask.</returns>
         public async ValueTask DisposeAsync()
         {
-            await CloseAsync().ConfigureAwait(false);
+            // On disposal, the browser doesn't get closed. It gets disconnected.
+            if (Launcher == null)
+            {
+                Disconnect();
+            }
+            else
+            {
+                await CloseAsync().ConfigureAwait(false);
+            }
+
             await ScreenshotTaskQueue.DisposeAsync().ConfigureAwait(false);
             GC.SuppressFinalize(this);
         }

diff --git a/lib/PuppeteerSharp/Cdp/CdpBrowser.cs b/lib/PuppeteerSharp/Cdp/CdpBrowser.cs
@@ -40,7 +40,6 @@ public class CdpBrowser : Browser
 
     private readonly ConcurrentDictionary<string, CdpBrowserContext> _contexts;
     private readonly ILogger<Browser> _logger;
-    private readonly Func<Target, bool> _targetFilterCallback;
     private Task _closeTask;
 
     internal CdpBrowser(
@@ -58,7 +57,7 @@ internal CdpBrowser(
         DefaultViewport = defaultViewport;
         Launcher = launcher;
         Connection = connection;
-        _targetFilterCallback = targetFilter ?? (_ => true);
+        var targetFilterCallback = targetFilter ?? (_ => true);
         _logger = Connection.LoggerFactory.CreateLogger<Browser>();
         IsPageTargetFunc =
             isPageTargetFunc ??
@@ -74,14 +73,14 @@ internal CdpBrowser(
             TargetManager = new FirefoxTargetManager(
                     connection,
                     CreateTarget,
-                    _targetFilterCallback);
+                    targetFilterCallback);
         }
         else
         {
             TargetManager = new ChromeTargetManager(
                 connection,
                 CreateTarget,
-                _targetFilterCallback,
+                targetFilterCallback,
                 this,
                 launcher?.Options?.Timeout ?? Puppeteer.DefaultTimeout);
         }

diff --git a/lib/PuppeteerSharp/Cdp/CdpPage.cs b/lib/PuppeteerSharp/Cdp/CdpPage.cs
@@ -113,6 +113,7 @@ private CdpPage(
             TaskScheduler.Default);
 
         SetupPrimaryTargetListeners();
+        AttachExistingTargets();
     }
 
     /// <inheritdoc cref="CDPSession"/>
@@ -1309,4 +1310,23 @@ private Task ResetBackgroundColorAndViewportAsync(ScreenshotOptions options)
             : Task.CompletedTask;
         return Task.WhenAll(omitBackgroundTask, setViewPortTask);
     }
+
+    private void AttachExistingTargets()
+    {
+        List<ITarget> queue = [];
+        queue.AddRange(_targetManager.GetChildTargets(PrimaryTarget));
+
+        for (var idx = 0; idx < queue.Count; idx++)
+        {
+            var next = (CdpTarget)queue[idx];
+            var session = next.Session;
+
+            if (session != null)
+            {
+                OnAttachedToTarget(this, new SessionEventArgs(session));
+            }
+
+            queue.AddRange(_targetManager.GetChildTargets(next));
+        }
+    }
 }
diff --git a/lib/PuppeteerSharp/Cdp/ChromeTargetManager.cs b/lib/PuppeteerSharp/Cdp/ChromeTargetManager.cs
@@ -89,6 +89,8 @@ await _connection.SendAsync(
             await _initializeCompletionSource.Task.ConfigureAwait(false);
         }
 
+        public IEnumerable<ITarget> GetChildTargets(ITarget target) => target.ChildTargets;
+
         private void StoreExistingTargetsForInit()
         {
             foreach (var kv in _discoveredTargetsByTargetId)
@@ -101,8 +103,11 @@ private void StoreExistingTargetsForInit()
                     null,
                     _browser.ScreenshotTaskQueue);
 
-                if ((_targetFilterFunc == null || _targetFilterFunc(targetForFilter)) &&
-                    kv.Value.Type != TargetType.Browser)
+                // Targets from extensions and the browser that will not be
+                // auto-attached. Therefore, we should not add them to
+                // #targetsIdsForInit.
+                var skipTarget = kv.Value.Type == TargetType.Browser || kv.Value.Url.StartsWith("chrome-extension://", StringComparison.OrdinalIgnoreCase);
+                if (!skipTarget && (_targetFilterFunc == null || _targetFilterFunc(targetForFilter)))
                 {
                     _targetsIdsForInit.Add(kv.Key);
                 }
@@ -292,6 +297,8 @@ private async Task OnAttachedToTargetAsync(object sender, TargetAttachedToTarget
                 _attachedTargetsBySessionId.TryAdd(session.Id, target);
             }
 
+            var parentTarget = parentSession?.Target;
+            parentTarget?.AddChildTarget(target);
             (parentSession ?? parentConnection as CDPSession)?.OnSessionReady(session);
 
             await EnsureTargetsIdsForInitAsync().ConfigureAwait(false);
@@ -380,6 +387,7 @@ private void OnDetachedFromTarget(object sender, TargetDetachedFromTargetRespons
                 return;
             }
 
+            (sender as CdpCDPSession)?.Target.RemoveChildTarget(target);
             _attachedTargetsByTargetId.TryRemove(target.TargetId, out _);
             TargetGone?.Invoke(this, new TargetChangedArgs { Target = target });
         }

diff --git a/lib/PuppeteerSharp/Cdp/FirefoxTargetManager.cs b/lib/PuppeteerSharp/Cdp/FirefoxTargetManager.cs
@@ -60,6 +60,8 @@ public async Task InitializeAsync()
 
         public AsyncDictionaryHelper<string, CdpTarget> GetAvailableTargets() => _availableTargetsByTargetId;
 
+        public IEnumerable<ITarget> GetChildTargets(ITarget target) => [];
+
         private void OnMessageReceived(object sender, MessageEventArgs e)
         {
             try

diff --git a/lib/PuppeteerSharp/Cdp/ITargetManager.cs b/lib/PuppeteerSharp/Cdp/ITargetManager.cs
@@ -1,4 +1,6 @@
 using System;
+using System.Collections;
+using System.Collections.Generic;
 using System.Threading.Tasks;
 using PuppeteerSharp.Cdp;
 using PuppeteerSharp.Helpers;
@@ -41,5 +43,12 @@ internal interface ITargetManager
         /// </summary>
         /// <returns>A task that resolves when all the tasks are completed.</returns>
         Task InitializeAsync();
+
+        /// <summary>
+        /// Returns the target's child.
+        /// </summary>
+        /// <param name="target">Target to evaluate.</param>
+        /// <returns>A list of targets.</returns>
+        IEnumerable<ITarget> GetChildTargets(ITarget target);
     }
 }
diff --git a/lib/PuppeteerSharp/ITarget.cs b/lib/PuppeteerSharp/ITarget.cs
@@ -1,3 +1,4 @@
+using System.Collections.Generic;
 using System.Threading.Tasks;
 
 namespace PuppeteerSharp
@@ -44,6 +45,11 @@ public interface ITarget
         /// <value>The URL.</value>
         string Url { get; }
 
+        /// <summary>
+        /// Gets the target's child targets.
+        /// </summary>
+        IEnumerable<ITarget> ChildTargets { get; }
+
         /// <summary>
         /// Creates a Chrome Devtools Protocol session attached to the target.
         /// </summary>

diff --git a/lib/PuppeteerSharp/Page.cs b/lib/PuppeteerSharp/Page.cs
@@ -750,7 +750,22 @@ public void Dispose()
         /// <inheritdoc/>
         public async ValueTask DisposeAsync()
         {
-            await CloseAsync().ConfigureAwait(false);
+            try
+            {
+                // We don't want to close the page if we're connected to the browser using `Connect`.
+                if (Browser.Launcher == null)
+                {
+                    return;
+                }
+
+                await CloseAsync().ConfigureAwait(false);
+            }
+            catch
+            {
+                // Closing on dispose might not be bulletproof.
+                // If the user didn't close the page explicitly, we won't fail.
+            }
+
             GC.SuppressFinalize(this);
         }
 

diff --git a/lib/PuppeteerSharp/PuppeteerSharp.csproj b/lib/PuppeteerSharp/PuppeteerSharp.csproj
@@ -12,10 +12,10 @@
     <Description>Headless Browser .NET API</Description>
     <PackageId>PuppeteerSharp</PackageId>
     <PackageReleaseNotes></PackageReleaseNotes>
-    <PackageVersion>18.0.2</PackageVersion>
-    <ReleaseVersion>18.0.2</ReleaseVersion>
-    <AssemblyVersion>18.0.2</AssemblyVersion>
-    <FileVersion>18.0.2</FileVersion>
+    <PackageVersion>18.0.3</PackageVersion>
+    <ReleaseVersion>18.0.3</ReleaseVersion>
+    <AssemblyVersion>18.0.3</AssemblyVersion>
+    <FileVersion>18.0.3</FileVersion>
     <SynchReleaseVersion>false</SynchReleaseVersion>
     <StyleCopTreatErrorsAsWarnings>false</StyleCopTreatErrorsAsWarnings>
     <DebugType>embedded</DebugType>

diff --git a/lib/PuppeteerSharp/Target.cs b/lib/PuppeteerSharp/Target.cs
@@ -1,6 +1,9 @@
 using System;
+using System.Collections.Generic;
 using System.Diagnostics;
 using System.Threading.Tasks;
+using PuppeteerSharp.Cdp;
+using PuppeteerSharp.Helpers;
 
 namespace PuppeteerSharp
 {
@@ -10,6 +13,8 @@ namespace PuppeteerSharp
     [DebuggerDisplay("Target {Type} - {Url}")]
     public abstract class Target : ITarget
     {
+        private readonly ConcurrentSet<ITarget> _childTargets = [];
+
         internal Target(
             TargetInfo targetInfo,
             CDPSession session,
@@ -47,6 +52,9 @@ internal Target(
         /// <inheritdoc/>
         IBrowserContext ITarget.BrowserContext => BrowserContext;
 
+        /// <inheritdoc/>
+        IEnumerable<ITarget> ITarget.ChildTargets => _childTargets;
+
         internal BrowserContext BrowserContext { get; }
 
         internal Browser Browser => BrowserContext.Browser;
@@ -80,5 +88,9 @@ internal Target(
 
         /// <inheritdoc/>
         public abstract Task<ICDPSession> CreateCDPSessionAsync();
+
+        internal void AddChildTarget(CdpTarget target) => _childTargets.Add(target);
+
+        internal void RemoveChildTarget(CdpTarget target) => _childTargets.Remove(target);
     }
 }