Skip to content

Commit

Permalink
Prevent leaks of chromium processes and temp user directories
Browse files Browse the repository at this point in the history
  • Loading branch information
@ggeurts
ggeurts committed Sep 4, 2018
1 parent 0a61436 commit b30afbb
Show file tree
Hide file tree
Showing 11 changed files with 1,037 additions and 548 deletions.
2 changes: 1 addition & 1 deletion lib/PuppeteerSharp.Tests/Issues/Issue0128.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ public class Issue0128
[Fact]
public async Task LauncherShouldFailGracefully()
{
await Assert.ThrowsAsync<ChromeProcessException>(async () =>
await Assert.ThrowsAsync<ChromiumProcessException>(async () =>
{
var options = TestConstants.DefaultBrowserOptions();
options.Args = new[] { "-remote-debugging-port=-2" };
Expand Down
162 changes: 83 additions & 79 deletions lib/PuppeteerSharp.Tests/PuppeteerTests/PuppeteerLaunchTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Net;
using System.Runtime.InteropServices;
using System.Threading.Tasks;
using PuppeteerSharp.Helpers;
using Xunit;
using Xunit.Abstractions;

Expand Down Expand Up @@ -119,116 +120,118 @@ public async Task ShouldRejectIfExecutablePathIsInvalid()
[Fact]
public async Task UserDataDirOption()
{
var launcher = new Launcher(TestConstants.LoggerFactory);
var userDataDir = Launcher.GetTemporaryDirectory();
var options = TestConstants.DefaultBrowserOptions();
options.UserDataDir = userDataDir;

using (var browser = await launcher.LaunchAsync(options))
using (var userDataDir = new TempDirectory())
{
Assert.True(Directory.GetFiles(userDataDir).Length > 0);
await browser.CloseAsync();
Assert.True(Directory.GetFiles(userDataDir).Length > 0);
await launcher.TryDeleteUserDataDir();
var options = TestConstants.DefaultBrowserOptions();
options.UserDataDir = userDataDir.Path;

var launcher = new Launcher(TestConstants.LoggerFactory);
using (var browser = await launcher.LaunchAsync(options))
{
Assert.True(Directory.GetFiles(userDataDir.Path).Length > 0);
await browser.CloseAsync();
Assert.True(Directory.GetFiles(userDataDir.Path).Length > 0);
}
}
}

[Fact]
public async Task UserDataDirArgument()
{
var launcher = new Launcher(TestConstants.LoggerFactory);
var userDataDir = Launcher.GetTemporaryDirectory();
var options = TestConstants.DefaultBrowserOptions();
options.Args = options.Args.Concat(new[] { $"--user-data-dir=\"{userDataDir}\"" }).ToArray();

using (var browser = await launcher.LaunchAsync(options))
using (var userDataDir = new TempDirectory())
{
// Open a page to make sure its functional.
await browser.NewPageAsync();
Assert.True(Directory.GetFiles(userDataDir).Length > 0);
await browser.CloseAsync();
Assert.True(Directory.GetFiles(userDataDir).Length > 0);
await launcher.TryDeleteUserDataDir();
var launcher = new Launcher(TestConstants.LoggerFactory);
var options = TestConstants.DefaultBrowserOptions();
options.Args = options.Args.Concat(new[] {$"--user-data-dir=\"{userDataDir}\""}).ToArray();

using (var browser = await launcher.LaunchAsync(options))
{
// Open a page to make sure its functional.
await browser.NewPageAsync();
Assert.True(Directory.GetFiles(userDataDir.Path).Length > 0);
await browser.CloseAsync();
Assert.True(Directory.GetFiles(userDataDir.Path).Length > 0);
}
}
}

[Fact]
public async Task UserDataDirOptionShouldRestoreState()
{
var launcher = new Launcher(TestConstants.LoggerFactory);
var userDataDir = Launcher.GetTemporaryDirectory();
var options = TestConstants.DefaultBrowserOptions();
options.Args = options.Args.Concat(new[] { $"--user-data-dir=\"{userDataDir}\"" }).ToArray();

using (var browser = await launcher.LaunchAsync(options))
using (var userDataDir = new TempDirectory())
{
var page = await browser.NewPageAsync();
await page.GoToAsync(TestConstants.EmptyPage);
await page.EvaluateExpressionAsync("localStorage.hey = 'hello'");
}
var launcher = new Launcher(TestConstants.LoggerFactory);
var options = TestConstants.DefaultBrowserOptions();
options.Args = options.Args.Concat(new[] {$"--user-data-dir=\"{userDataDir}\""}).ToArray();

using (var browser2 = await Puppeteer.LaunchAsync(options, TestConstants.LoggerFactory))
{
var page2 = await browser2.NewPageAsync();
await page2.GoToAsync(TestConstants.EmptyPage);
Assert.Equal("hello", await page2.EvaluateExpressionAsync("localStorage.hey"));
}
using (var browser = await launcher.LaunchAsync(options))
{
var page = await browser.NewPageAsync();
await page.GoToAsync(TestConstants.EmptyPage);
await page.EvaluateExpressionAsync("localStorage.hey = 'hello'");
}

await launcher.TryDeleteUserDataDir();
using (var browser2 = await Puppeteer.LaunchAsync(options, TestConstants.LoggerFactory))
{
var page2 = await browser2.NewPageAsync();
await page2.GoToAsync(TestConstants.EmptyPage);
Assert.Equal("hello", await page2.EvaluateExpressionAsync("localStorage.hey"));
}
}
}

[Fact]
public async Task UserDataDirOptionShouldRestoreCookies()
{
var launcher = new Launcher(TestConstants.LoggerFactory);
var userDataDir = Launcher.GetTemporaryDirectory();
var options = TestConstants.DefaultBrowserOptions();
options.Args = options.Args.Concat(new[] { $"--user-data-dir=\"{userDataDir}\"" }).ToArray();

using (var browser = await launcher.LaunchAsync(options))
using (var userDataDir = new TempDirectory())
{
var page = await browser.NewPageAsync();
await page.GoToAsync(TestConstants.EmptyPage);
await page.EvaluateExpressionAsync(
"document.cookie = 'doSomethingOnlyOnce=true; expires=Fri, 31 Dec 9999 23:59:59 GMT'");
}
var launcher = new Launcher(TestConstants.LoggerFactory);
var options = TestConstants.DefaultBrowserOptions();
options.Args = options.Args.Concat(new[] {$"--user-data-dir=\"{userDataDir}\""}).ToArray();

using (var browser2 = await Puppeteer.LaunchAsync(options, TestConstants.LoggerFactory))
{
var page2 = await browser2.NewPageAsync();
await page2.GoToAsync(TestConstants.EmptyPage);
Assert.Equal("doSomethingOnlyOnce=true", await page2.EvaluateExpressionAsync("document.cookie"));
}
using (var browser = await launcher.LaunchAsync(options))
{
var page = await browser.NewPageAsync();
await page.GoToAsync(TestConstants.EmptyPage);
await page.EvaluateExpressionAsync(
"document.cookie = 'doSomethingOnlyOnce=true; expires=Fri, 31 Dec 9999 23:59:59 GMT'");
}

await launcher.TryDeleteUserDataDir();
using (var browser2 = await Puppeteer.LaunchAsync(options, TestConstants.LoggerFactory))
{
var page2 = await browser2.NewPageAsync();
await page2.GoToAsync(TestConstants.EmptyPage);
Assert.Equal("doSomethingOnlyOnce=true", await page2.EvaluateExpressionAsync("document.cookie"));
}
}
}

[Fact]
public async Task HeadlessShouldBeAbleToReadCookiesWrittenByHeadful()
{
var launcher = new Launcher(TestConstants.LoggerFactory);
var userDataDir = Launcher.GetTemporaryDirectory();
var options = TestConstants.DefaultBrowserOptions();
options.Args = options.Args.Concat(new[] { $"--user-data-dir=\"{userDataDir}\"" }).ToArray();
options.Headless = false;

using (var browser = await launcher.LaunchAsync(options))
using (var userDataDir = new TempDirectory())
{
var page = await browser.NewPageAsync();
await page.GoToAsync(TestConstants.EmptyPage);
await page.EvaluateExpressionAsync(
"document.cookie = 'foo=true; expires=Fri, 31 Dec 9999 23:59:59 GMT'");
}
var launcher = new Launcher(TestConstants.LoggerFactory);
var options = TestConstants.DefaultBrowserOptions();
options.Args = options.Args.Concat(new[] {$"--user-data-dir=\"{userDataDir}\""}).ToArray();
options.Headless = false;

options.Headless = true;
using (var browser2 = await Puppeteer.LaunchAsync(options, TestConstants.LoggerFactory))
{
var page2 = await browser2.NewPageAsync();
await page2.GoToAsync(TestConstants.EmptyPage);
Assert.Equal("foo=true", await page2.EvaluateExpressionAsync("document.cookie"));
}
using (var browser = await launcher.LaunchAsync(options))
{
var page = await browser.NewPageAsync();
await page.GoToAsync(TestConstants.EmptyPage);
await page.EvaluateExpressionAsync(
"document.cookie = 'foo=true; expires=Fri, 31 Dec 9999 23:59:59 GMT'");
}

await launcher.TryDeleteUserDataDir();
options.Headless = true;
using (var browser2 = await Puppeteer.LaunchAsync(options, TestConstants.LoggerFactory))
{
var page2 = await browser2.NewPageAsync();
await page2.GoToAsync(TestConstants.EmptyPage);
Assert.Equal("foo=true", await page2.EvaluateExpressionAsync("document.cookie"));
}
}
}

[Fact]
Expand Down Expand Up @@ -278,7 +281,7 @@ public async Task ChromeShouldBeClosed()

await browser.CloseAsync();

Assert.True(launcher.IsChromeClosed);
Assert.True(launcher.Process.HasExited);
}
}

Expand All @@ -295,7 +298,8 @@ public async Task ChromeShouldBeClosedOnDispose()
Assert.Equal(HttpStatusCode.OK, response.Status);
}

Assert.True(launcher.IsChromeClosed);
Assert.True(await launcher.Process.WaitForExitAsync(TimeSpan.FromSeconds(10)));
Assert.True(launcher.Process.HasExited);
}

[Fact]
Expand Down
5 changes: 4 additions & 1 deletion lib/PuppeteerSharp.sln
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "PuppeteerSharp.TestServer",
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "PuppeteerSharp.Tests.DumpIO", "PuppeteerSharp.Tests.DumpIO\PuppeteerSharp.Tests.DumpIO.csproj", "{B1892212-CEE3-4DC3-ADB8-04A2D9A081A0}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "PuppeteerSharp.Tests.CloseMe", "PuppeteerSharp.Tests.CloseMe\PuppeteerSharp.Tests.CloseMe.csproj", "{B1B0358F-88A2-4038-9974-7850D906C76B}"
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "PuppeteerSharp.Tests.CloseMe", "PuppeteerSharp.Tests.CloseMe\PuppeteerSharp.Tests.CloseMe.csproj", "{B1B0358F-88A2-4038-9974-7850D906C76B}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Expand Down Expand Up @@ -45,6 +45,9 @@ Global
GlobalSection(ExtensibilityGlobals) = postSolution
SolutionGuid = {F9F7978B-89C0-4A54-8D80-A1E93E401D4C}
EndGlobalSection
GlobalSection(ExtensibilityGlobals) = postSolution
SolutionGuid = {F9F7978B-89C0-4A54-8D80-A1E93E401D4C}
EndGlobalSection
GlobalSection(MonoDevelopProperties) = preSolution
Policies = $0
$0.DotNetNamingPolicy = $1
Expand Down
77 changes: 51 additions & 26 deletions lib/PuppeteerSharp/Browser.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,14 @@ public class Browser : IDisposable
/// <param name="contextIds">The context ids></param>
/// <param name="ignoreHTTPSErrors">The option to ignoreHTTPSErrors</param>
/// <param name="setDefaultViewport">The option to setDefaultViewport</param>
/// <param name="process">The chrome process</param>
/// <param name="closeCallBack">An async function called before closing</param>
/// <param name="chromiumProcess">The Chromium process</param>
public Browser(
Connection connection,
string[] contextIds,
bool ignoreHTTPSErrors,
bool setDefaultViewport,
Process process,
Func<Task> closeCallBack)
ChromiumProcess chromiumProcess)
{
Process = process;
Connection = connection;
IgnoreHTTPSErrors = ignoreHTTPSErrors;
SetDefaultViewport = setDefaultViewport;
Expand All @@ -65,7 +62,7 @@ public Browser(
Connection.Closed += (object sender, EventArgs e) => Disconnected?.Invoke(this, new EventArgs());
Connection.MessageReceived += Connect_MessageReceived;

_closeCallBack = closeCallBack;
_chromiumProcess = chromiumProcess;
_logger = Connection.LoggerFactory.CreateLogger<Browser>();
}

Expand All @@ -74,9 +71,11 @@ public Browser(
internal readonly Dictionary<string, Target> TargetsMap;

private readonly Dictionary<string, BrowserContext> _contexts;
private readonly Func<Task> _closeCallBack;
private readonly ILogger<Browser> _logger;
private readonly BrowserContext _defaultContext;
private readonly ChromiumProcess _chromiumProcess;
private Task _closeTask;

#endregion

#region Properties
Expand Down Expand Up @@ -123,7 +122,7 @@ public Browser(
/// <summary>
/// Gets the spawned browser process. Returns <c>null</c> if the browser instance was created with <see cref="Puppeteer.ConnectAsync(ConnectOptions, ILoggerFactory)"/> method.
/// </summary>
public Process Process { get; }
public Process Process => _chromiumProcess?.Process;

/// <summary>
/// Gets or Sets whether to ignore HTTPS errors during navigation
Expand All @@ -133,7 +132,7 @@ public Browser(
/// <summary>
/// Gets a value indicating if the browser is closed
/// </summary>
public bool IsClosed { get; internal set; }
public bool IsClosed => _closeTask != null && _closeTask.IsCompleted && _closeTask.Exception != null;

internal TaskQueue ScreenshotTaskQueue { get; set; }
internal Connection Connection { get; }
Expand Down Expand Up @@ -236,24 +235,46 @@ public async Task<string> GetUserAgentAsync()
/// Closes Chromium and all of its pages (if any were opened). The browser object itself is considered disposed and cannot be used anymore
/// </summary>
/// <returns>Task</returns>
public async Task CloseAsync()
public Task CloseAsync() => _closeTask ?? (_closeTask = CloseCoreAsync());

private async Task CloseCoreAsync()
{
if (IsClosed)
Connection.StopReading();
try
{
return;
try
{
// Initiate graceful browser close operation but don't await it just yet,
// because we want to ensure chromium process shutdown first.
var browserCloseTask = Connection.SendAsync("Browser.close", null);

if (_chromiumProcess != null)
{
// Notify chromium process that exit is expected, but should be enforced if it
// doesn't occur withing the close timeout.
var closeTimeout = TimeSpan.FromMilliseconds(5000);
await _chromiumProcess.EnsureExitAsync(closeTimeout).ConfigureAwait(false);
}

// Now we can safely await the browser close operation without risking keeping chromium
// process running for indeterminate period.
await browserCloseTask.ConfigureAwait(false);
}
finally
{
Disconnect();
}
}

IsClosed = true;
Connection.StopReading();

var closeTask = _closeCallBack();

if (closeTask != null)
catch (Exception ex)
{
await closeTask.ConfigureAwait(false);
_logger.LogError(ex, ex.Message);

if (_chromiumProcess != null)
{
await _chromiumProcess.KillAsync().ConfigureAwait(false);
}
}

Disconnect();
Closed?.Invoke(this, new EventArgs());
}

Expand Down Expand Up @@ -372,10 +393,9 @@ internal static async Task<Browser> CreateAsync(
string[] contextIds,
bool ignoreHTTPSErrors,
bool appMode,
Process process,
Func<Task> closeCallBack)
ChromiumProcess chromiumProcess)
{
var browser = new Browser(connection, contextIds, ignoreHTTPSErrors, appMode, process, closeCallBack);
var browser = new Browser(connection, contextIds, ignoreHTTPSErrors, appMode, chromiumProcess);
await connection.SendAsync("Target.setDiscoverTargets", new
{
discover = true
Expand All @@ -386,8 +406,13 @@ internal static async Task<Browser> CreateAsync(
#endregion

#region IDisposable
/// <inheritdoc />
public void Dispose() => CloseAsync().GetAwaiter().GetResult();

/// <summary>
/// Closes <see cref="Connection"/> and any Chromium <see cref="Process"/> that was
/// created by Puppeteer.
/// </summary>
public void Dispose() => _ = CloseAsync();

#endregion
}
}
Loading

0 comments on commit b30afbb

Please sign in to comment.