[BUG] Stack inspection done by playwright is fragile and breaks with Dynamic PGO enabled

[AndyAyersMS]

Context:

We enabled DynamicPGO in .NET 8 Preview 5 and are seeing reports of some failures in applications that use playwright.

See for instance dotnet/runtime#87628

This should readily repro if you run release mode builds with .NET 8 Preview 5 and default runtime options.

Describe the bug

playwright-dotnet/src/Playwright/Transport/Connection.cs

Lines 477 to 502 in ca7b02a

	for (int i = 0; i < st.FrameCount; ++i)
	{
	var sf = st.GetFrame(i);
	string fileName = sf.GetFileName();
	if (IsPlaywrightInternalNamespace(sf.GetMethod().ReflectedType?.Namespace))
	{
	string methodName = $"{sf?.GetMethod()?.DeclaringType?.Name}.{sf?.GetMethod()?.Name}";
	if (methodName.Contains("WrapApiBoundaryAsync"))
	{
	apiBoundaryReached = true;
	}
	var hasCleanMethodName = !methodName.StartsWith("<", StringComparison.InvariantCultureIgnoreCase);
	if (hasCleanMethodName)
	{
	lastInternalApiName = methodName;
	}
	}
	else if (!string.IsNullOrEmpty(fileName))
	{
	stack.Add(new() { File = fileName, Line = sf.GetFileLineNumber(), Column = sf.GetFileColumnNumber() });
	if (!string.IsNullOrEmpty(lastInternalApiName) && !apiBoundaryReached)
	{
	apiName = lastInternalApiName;
	}
	}
	}

is looking for a specific pattern of method names in the stack trace. This is fragile unless all the methods being looked for are marked with [MethodImpl(MethodImplOptions.NoInlining)]. Note that with PGO the JIT may now inline methods at virtual, interface, and delegate call sites.

I suspect the fix may be as simple as marking this method noinline

playwright-dotnet/src/Playwright/Transport/Connection.cs

Lines 521 to 528 in ca7b02a

	internal Task WrapApiCallAsync(Func<Task> action, bool isInternal = false)
	=> WrapApiCallAsync(
	async () =>
	{
	await action().ConfigureAwait(false);
	return true;
	},
	isInternal);

but there may be more cases elsewhere in this code that need similar treatment.

[slang25]

It looks like WrapApiBoundaryAsync might need marking as NoInlining too as it's searched for in the stack trace
Edit: I presume the try/finally is why is isn't an issue actually, so ignore that

[davidfowl]

Why is this code looking at the call stack?

[slang25]

Playwright has a tracing feature that's frankly amazing (see https://stu.dev/pw). It captures the API that was called by the users code, and optionally you can include sources, this works by walking the stack frames here and only passing to playwright (the node implementation) the users stack frames.

Stack frame walking is so brittle (we did it in Shouldly and I hate it), [CallerMemberName] might be a good solution to capture the API name, but don't think there's any alternative for the include sources functionality. This stuff probably does play well with deterministic builds either I suspect, as it's unlikely to be able to find the source files.

[mxschmitt]

Thanks for the investigation! The internal stack collection gets used for various things, e.g. as per above trace viewer where the actual .NET API calls get shown or we use it internally for logging (to know which .NET API calls (which are exposed) belong to which internal protocol API call).

I tried to reproduce it using dotnet/runtime#87628 (comment) and our official getting started on .NET 8 preview 5 (8.0.100-preview.5.23303.2) but was in both cases unfortunately not able to. I'm on macOS, as per the issue in the runtime repo, it was also reproducible there.

What did you do differently? I made sure its a Release build.

[AndyAyersMS]

I tried to reproduce it using dotnet/runtime#87628 (comment) and our official getting started on .NET 8 preview 5 (8.0.100-preview.5.23303.2) but was in both cases unfortunately not able to. I'm on macOS, as per the issue in the runtime repo, it was also reproducible there.

What did you do differently? I made sure its a Release build.

So you ran with dotnet run -c Release ?

You may need to run enough code to get things to properly tier up (eg repeatedly call things in a loop, 100x or so should do the trick). You can set DOTNET_JitDisasmSummary=1 in the environment where you launch dotnet, and see how many methods report being optimized with Dynamic PGO.

If you run enough code eventually you should see things like:

4135: JIT compiled System.Buffer:Memmove(byref,byref,ulong) [Tier1 with Dynamic PGO, IL size=480, code size=248, hash=0x3b15845e]

[AndyAyersMS]

I can repro if I update the getting started example to:

using Microsoft.Playwright;

using var playwright = await Playwright.CreateAsync();

for (int i = 0; i < 100; i++)
{
await using var browser = await playwright.Chromium.LaunchAsync();
var page = await browser.NewPageAsync();
await page.GotoAsync("https://playwright.dev/dotnet");
await page.ScreenshotAsync(new()
{
    Path = "screenshot.png"
});
}

for example

C:\bugs\playwright>dotnet build -c Release
MSBuild version 17.7.0-preview-23281-03+4ce2ff1f8 for .NET
  Determining projects to restore...
  All projects are up-to-date for restore.
C:\Program Files\dotnet\sdk\8.0.100-preview.5.23303.2\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.RuntimeIdentifierInference.targets(314,5): message NETSDK1057: You are usi
ng a preview version of .NET. See: https://aka.ms/dotnet-support-policy [C:\bugs\playwright\playwright.csproj]
  playwright -> C:\bugs\playwright\bin\Release\net8.0\playwright.dll

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:01.07

C:\bugs\playwright>dotnet exec C:\bugs\playwright\bin\Release\net8.0\playwright.dll
Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.Playwright.Transport.Connection.InnerSendMessageToServerAsync[T](String guid, String method, Dictionary`2 dictionary, Boolean keepNulls) in /_/src/Playwright/Transport/Connection.cs:line 150
   at Microsoft.Playwright.Transport.Connection.WrapApiCallAsync[T](Func`1 action, Boolean isInternal)
   at Microsoft.Playwright.Core.BrowserType.LaunchAsync(BrowserTypeLaunchOptions options) in /_/src/Playwright/Core/BrowserType.cs:line 61
   at Program.<Main>$(String[] args) in C:\bugs\playwright\Program.cs:line 7
   at Program.<Main>(String[] args)

I'll try this on my M1 and see if the same thing happens there.

[AndyAyersMS]

I can also repro this on the M1 with .Net 8 Preview 5.

[AndyAyersMS]

Any idea when this fix might appear in a release? A couple other partner teams are running into this.

[mxschmitt]

Any idea when this fix might appear in a release? A couple other partner teams are running into this.

Probably in 2-3 weeks.

[timmydo]

@AndyAyersMS I don't think we will be able to ship on preview 5 then.

[AndyAyersMS]

@mxschmitt would it be possible to create a release sooner? I would like to be able to unblock @timmydo if possible.