Skip to content

Commit eca1640

Browse files
kblokjnyrup
kblok
and
jnyrup
authored
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 <[email protected]> * 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 <[email protected]>
1 parent 5e9f4a5 commit eca1640

File tree

12 files changed

+134
-12
lines changed

12 files changed

+134
-12
lines changed

‎lib/PuppeteerSharp.Nunit/TestExpectations/TestExpectations.upstream.json‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2943,6 +2943,20 @@
29432943
"expectations": ["FAIL"],
29442944
"comment": "TODO: add a comment explaining why this expectation is required (include links to issues)"
29452945
},
2946+
{
2947+
"testIdPattern": "[oopif.spec] OOPIF should recover cross-origin frames on reconnect",
2948+
"platforms": ["darwin", "linux", "win32"],
2949+
"parameters": ["chrome", "webDriverBiDi"],
2950+
"expectations": ["FAIL"],
2951+
"comment": "See https://github.com/GoogleChromeLabs/chromium-bidi/issues/2362"
2952+
},
2953+
{
2954+
"testIdPattern": "[oopif.spec] OOPIF should recover cross-origin frames on reconnect",
2955+
"platforms": ["darwin", "linux", "win32"],
2956+
"parameters": ["firefox", "webDriverBiDi"],
2957+
"expectations": ["FAIL"],
2958+
"comment": "Firefox does not support multiple sessions in BiDi."
2959+
},
29462960
{
29472961
"testIdPattern": "[oopif.spec] OOPIF should support lazy OOP frames",
29482962
"platforms": ["darwin", "linux", "win32"],

‎lib/PuppeteerSharp.Tests/OOPIFTests/OOPIFTests.cs‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,34 @@ await FrameUtils.AttachFrameAsync(
104104
StringAssert.Contains("frame.html", await frame2.EvaluateExpressionAsync<string>("document.location.href"));
105105
}
106106

107+
[Test, Retry(2), PuppeteerTest("oopif.spec", "OOPIF", "should recover cross-origin frames on reconnect")]
108+
public async Task ShouldRecoverCrossOriginFramesOnReconnect()
109+
{
110+
await Page.GoToAsync(TestConstants.EmptyPage);
111+
var frame1Task = Page.WaitForFrameAsync((frame) => frame != Page.MainFrame && frame.ParentFrame == Page.MainFrame);
112+
var frame2Task = Page.WaitForFrameAsync((frame) => frame != Page.MainFrame && frame.ParentFrame != Page.MainFrame);
113+
114+
await FrameUtils.AttachFrameAsync(
115+
Page,
116+
"frame1",
117+
TestConstants.CrossProcessHttpPrefix + "/frames/one-frame.html"
118+
);
119+
120+
await Task.WhenAll(frame1Task, frame2Task);
121+
var dump1 = await FrameUtils.DumpFramesAsync(Page.MainFrame);
122+
123+
await using var browserTwo = await Puppeteer.ConnectAsync(new ConnectOptions()
124+
{
125+
BrowserWSEndpoint = Browser.WebSocketEndpoint,
126+
});
127+
128+
var pages = await browserTwo.PagesAsync();
129+
var emptyPages = pages.Where(page => page.Url == TestConstants.EmptyPage).ToArray();
130+
Assert.AreEqual(1, emptyPages.Length);
131+
var dump2 = await FrameUtils.DumpFramesAsync(emptyPages[0].MainFrame);
132+
Assert.AreEqual(dump1, dump2);
133+
}
134+
107135
[Test, Retry(2), PuppeteerTest("oopif.spec", "OOPIF", "should support OOP iframes getting detached")]
108136
public async Task ShouldSupportOopIframesGettingDetached()
109137
{

‎lib/PuppeteerSharp/Browser.cs‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,16 @@ public void Dispose()
174174
/// <returns>ValueTask.</returns>
175175
public async ValueTask DisposeAsync()
176176
{
177-
await CloseAsync().ConfigureAwait(false);
177+
// On disposal, the browser doesn't get closed. It gets disconnected.
178+
if (Launcher == null)
179+
{
180+
Disconnect();
181+
}
182+
else
183+
{
184+
await CloseAsync().ConfigureAwait(false);
185+
}
186+
178187
await ScreenshotTaskQueue.DisposeAsync().ConfigureAwait(false);
179188
GC.SuppressFinalize(this);
180189
}

‎lib/PuppeteerSharp/Cdp/CdpBrowser.cs‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,6 @@ public class CdpBrowser : Browser
4040

4141
private readonly ConcurrentDictionary<string, CdpBrowserContext> _contexts;
4242
private readonly ILogger<Browser> _logger;
43-
private readonly Func<Target, bool> _targetFilterCallback;
4443
private Task _closeTask;
4544

4645
internal CdpBrowser(
@@ -58,7 +57,7 @@ internal CdpBrowser(
5857
DefaultViewport = defaultViewport;
5958
Launcher = launcher;
6059
Connection = connection;
61-
_targetFilterCallback = targetFilter ?? (_ => true);
60+
var targetFilterCallback = targetFilter ?? (_ => true);
6261
_logger = Connection.LoggerFactory.CreateLogger<Browser>();
6362
IsPageTargetFunc =
6463
isPageTargetFunc ??
@@ -74,14 +73,14 @@ internal CdpBrowser(
7473
TargetManager = new FirefoxTargetManager(
7574
connection,
7675
CreateTarget,
77-
_targetFilterCallback);
76+
targetFilterCallback);
7877
}
7978
else
8079
{
8180
TargetManager = new ChromeTargetManager(
8281
connection,
8382
CreateTarget,
84-
_targetFilterCallback,
83+
targetFilterCallback,
8584
this,
8685
launcher?.Options?.Timeout ?? Puppeteer.DefaultTimeout);
8786
}

‎lib/PuppeteerSharp/Cdp/CdpPage.cs‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,7 @@ private CdpPage(
113113
TaskScheduler.Default);
114114

115115
SetupPrimaryTargetListeners();
116+
AttachExistingTargets();
116117
}
117118

118119
/// <inheritdoc cref="CDPSession"/>
@@ -1309,4 +1310,23 @@ private Task ResetBackgroundColorAndViewportAsync(ScreenshotOptions options)
13091310
: Task.CompletedTask;
13101311
return Task.WhenAll(omitBackgroundTask, setViewPortTask);
13111312
}
1313+
1314+
private void AttachExistingTargets()
1315+
{
1316+
List<ITarget> queue = [];
1317+
queue.AddRange(_targetManager.GetChildTargets(PrimaryTarget));
1318+
1319+
for (var idx = 0; idx < queue.Count; idx++)
1320+
{
1321+
var next = (CdpTarget)queue[idx];
1322+
var session = next.Session;
1323+
1324+
if (session != null)
1325+
{
1326+
OnAttachedToTarget(this, new SessionEventArgs(session));
1327+
}
1328+
1329+
queue.AddRange(_targetManager.GetChildTargets(next));
1330+
}
1331+
}
13121332
}

‎lib/PuppeteerSharp/Cdp/ChromeTargetManager.cs‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,8 @@ await _connection.SendAsync(
8989
await _initializeCompletionSource.Task.ConfigureAwait(false);
9090
}
9191

92+
public IEnumerable<ITarget> GetChildTargets(ITarget target) => target.ChildTargets;
93+
9294
private void StoreExistingTargetsForInit()
9395
{
9496
foreach (var kv in _discoveredTargetsByTargetId)
@@ -101,8 +103,11 @@ private void StoreExistingTargetsForInit()
101103
null,
102104
_browser.ScreenshotTaskQueue);
103105

104-
if ((_targetFilterFunc == null || _targetFilterFunc(targetForFilter)) &&
105-
kv.Value.Type != TargetType.Browser)
106+
// Targets from extensions and the browser that will not be
107+
// auto-attached. Therefore, we should not add them to
108+
// #targetsIdsForInit.
109+
var skipTarget = kv.Value.Type == TargetType.Browser || kv.Value.Url.StartsWith("chrome-extension://", StringComparison.OrdinalIgnoreCase);
110+
if (!skipTarget && (_targetFilterFunc == null || _targetFilterFunc(targetForFilter)))
106111
{
107112
_targetsIdsForInit.Add(kv.Key);
108113
}
@@ -292,6 +297,8 @@ private async Task OnAttachedToTargetAsync(object sender, TargetAttachedToTarget
292297
_attachedTargetsBySessionId.TryAdd(session.Id, target);
293298
}
294299

300+
var parentTarget = parentSession?.Target;
301+
parentTarget?.AddChildTarget(target);
295302
(parentSession ?? parentConnection as CDPSession)?.OnSessionReady(session);
296303

297304
await EnsureTargetsIdsForInitAsync().ConfigureAwait(false);
@@ -380,6 +387,7 @@ private void OnDetachedFromTarget(object sender, TargetDetachedFromTargetRespons
380387
return;
381388
}
382389

390+
(sender as CdpCDPSession)?.Target.RemoveChildTarget(target);
383391
_attachedTargetsByTargetId.TryRemove(target.TargetId, out _);
384392
TargetGone?.Invoke(this, new TargetChangedArgs { Target = target });
385393
}

‎lib/PuppeteerSharp/Cdp/FirefoxTargetManager.cs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,8 @@ public async Task InitializeAsync()
6060

6161
public AsyncDictionaryHelper<string, CdpTarget> GetAvailableTargets() => _availableTargetsByTargetId;
6262

63+
public IEnumerable<ITarget> GetChildTargets(ITarget target) => [];
64+
6365
private void OnMessageReceived(object sender, MessageEventArgs e)
6466
{
6567
try

‎lib/PuppeteerSharp/Cdp/ITargetManager.cs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
using System;
2+
using System.Collections;
3+
using System.Collections.Generic;
24
using System.Threading.Tasks;
35
using PuppeteerSharp.Cdp;
46
using PuppeteerSharp.Helpers;
@@ -41,5 +43,12 @@ internal interface ITargetManager
4143
/// </summary>
4244
/// <returns>A task that resolves when all the tasks are completed.</returns>
4345
Task InitializeAsync();
46+
47+
/// <summary>
48+
/// Returns the target's child.
49+
/// </summary>
50+
/// <param name="target">Target to evaluate.</param>
51+
/// <returns>A list of targets.</returns>
52+
IEnumerable<ITarget> GetChildTargets(ITarget target);
4453
}
4554
}

‎lib/PuppeteerSharp/ITarget.cs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
using System.Collections.Generic;
12
using System.Threading.Tasks;
23

34
namespace PuppeteerSharp
@@ -44,6 +45,11 @@ public interface ITarget
4445
/// <value>The URL.</value>
4546
string Url { get; }
4647

48+
/// <summary>
49+
/// Gets the target's child targets.
50+
/// </summary>
51+
IEnumerable<ITarget> ChildTargets { get; }
52+
4753
/// <summary>
4854
/// Creates a Chrome Devtools Protocol session attached to the target.
4955
/// </summary>

‎lib/PuppeteerSharp/Page.cs‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -750,7 +750,22 @@ public void Dispose()
750750
/// <inheritdoc/>
751751
public async ValueTask DisposeAsync()
752752
{
753-
await CloseAsync().ConfigureAwait(false);
753+
try
754+
{
755+
// We don't want to close the page if we're connected to the browser using `Connect`.
756+
if (Browser.Launcher == null)
757+
{
758+
return;
759+
}
760+
761+
await CloseAsync().ConfigureAwait(false);
762+
}
763+
catch
764+
{
765+
// Closing on dispose might not be bulletproof.
766+
// If the user didn't close the page explicitly, we won't fail.
767+
}
768+
754769
GC.SuppressFinalize(this);
755770
}
756771

0 commit comments

Comments
 (0)