From 78f88633baf99211a2157efbadf072d4981de1b0 Mon Sep 17 00:00:00 2001
From: Jonathan Peppers <jonathan.peppers@microsoft.com>
Date: Thu, 21 Nov 2024 12:36:09 -0800
Subject: [PATCH] [Xamarin.Android.Build.Tasks] close `XAAssemblyResolvers`
 (#9531)

Fixes: https://github.com/dotnet/android/issues/9133

Context: https://discord.com/channels/732297728826277939/732297916680765551/1308554103206580244
Context: 86260ed36dfe1a90c8ed6a2bb1cd0607d637f403

Various customers have been reporting `UnauthorizedAccessExceptions`
in incremental builds, which seems to be a new problem in .NET 9.
We were not able to reproduce the issue locally, but with the number
of reports it seems to be a real issue.

One customer shared a [`MSBuild.dmp`][0] file (while the file was
locked), where I could observe the objects in memory:

	MemoryMappedViewStream	132
	    Mono.Cecil.PE.Image	100
	        Mono.Cecil.ModuleDefinition	100
	            Mono.Cecil.TypeDefinition	100
	                Mono.Cecil.TypeDefinition[]	100
	                    List<Mono.Cecil.TypeDefinition>	1
	                        Xamarin.Android.Tasks.NativeCodeGenState [Static variable Xamarin.Android.Tasks.NativeCodeGenState.<Template>k__BackingField]	1

Then realized the problem was:

  * We opened some `.dll` files with `Mono.Cecil`.

  * They were never closed.

  * Future incremental build attempts would fail on various
    operations of the same `.dll` files.

We were also storing some static state (`NativeCodeGenState`) to be
shared across multiple MSBuild tasks:

	partial class NativeCodeGenState {
	    public static NativeCodeGenState? Template { get; set; }
	}

`NativeCodeGenState` also holds a `XAAssemblyResolver` in a `Resolver`
property.  This means this `XAAssemblyResolver` instance would *also*
be kept alive.

It appears we only use the static `Template` property for a `bool`
flag, so I changed the property to a `bool` instead:

	partial class NativeCodeGenState {
	    public static bool TemplateJniAddNativeMethodRegistrationAttributePresent { get; set; }
	}

After this change, we can safely dispose `Resolver` instances.
I looped over the `NativeCodeGenState` instances disposing of each
`Resolver` at the end of the `<GenerateJavaStubs/>` and
`<GeneratePackageManagerJava/>` MSBuild tasks.

[0]: https://drive.google.com/file/d/12dOkO6ZdbK67sNeY_PVS4d0mgnLyOwUA/view?pli=1
---
 .../Tasks/GenerateJavaStubs.cs                 | 18 +++++++++++++++---
 .../Tasks/GeneratePackageManagerJava.cs        | 10 +++++++++-
 .../Utilities/NativeCodeGenState.cs            |  2 +-
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs b/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
index bc1324a5957..7c13c195b16 100644
--- a/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
+++ b/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
@@ -222,9 +222,6 @@ void Run (bool useMarshalMethods)
 			}
 			JCWGenerator.EnsureAllArchitecturesAreIdentical (Log, nativeCodeGenStates);
 
-			NativeCodeGenState.Template = templateCodeGenState;
-			BuildEngine4.RegisterTaskObjectAssemblyLocal (ProjectSpecificTaskObjectKey (NativeCodeGenStateRegisterTaskKey), nativeCodeGenStates, RegisteredTaskObjectLifetime.Build);
-
 			if (useMarshalMethods) {
 				// We need to parse the environment files supplied by the user to see if they want to use broken exception transitions. This information is needed
 				// in order to properly generate wrapper methods in the marshal methods assembly rewriter.
@@ -262,6 +259,9 @@ void Run (bool useMarshalMethods)
 				WriteTypeMappings (state);
 			}
 
+			// Set for use by <GeneratePackageManagerJava/> task later
+			NativeCodeGenState.TemplateJniAddNativeMethodRegistrationAttributePresent = templateCodeGenState.JniAddNativeMethodRegistrationAttributePresent;
+
 			var acwMapGen = new ACWMapGenerator (Log);
 			if (!acwMapGen.Generate (templateCodeGenState, AcwMapFile)) {
 				Log.LogDebugMessage ("ACW map generation failed");
@@ -270,6 +270,18 @@ void Run (bool useMarshalMethods)
 			IList<string> additionalProviders = MergeManifest (templateCodeGenState, MaybeGetArchAssemblies (userAssembliesPerArch, templateCodeGenState.TargetArch));
 			GenerateAdditionalProviderSources (templateCodeGenState, additionalProviders);
 
+			if (useMarshalMethods) {
+				// Save NativeCodeGenState for <GeneratePackageManagerJava/> task later
+				Log.LogDebugMessage ($"Saving {nameof (NativeCodeGenState)} to {nameof (NativeCodeGenStateRegisterTaskKey)}");
+				BuildEngine4.RegisterTaskObjectAssemblyLocal (ProjectSpecificTaskObjectKey (NativeCodeGenStateRegisterTaskKey), nativeCodeGenStates, RegisteredTaskObjectLifetime.Build);
+			} else {
+				// Otherwise, dispose all XAAssemblyResolvers
+				Log.LogDebugMessage ($"Disposing all {nameof (NativeCodeGenState)}.{nameof (NativeCodeGenState.Resolver)}");
+				foreach (var state in nativeCodeGenStates.Values) {
+					state.Resolver.Dispose ();
+				}
+			}
+
 			Dictionary<string, ITaskItem> MaybeGetArchAssemblies (Dictionary<AndroidTargetArch, Dictionary<string, ITaskItem>> dict, AndroidTargetArch arch)
 			{
 				if (!dict.TryGetValue (arch, out Dictionary<string, ITaskItem> archDict)) {
diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs b/src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs
index 6f095c30cb2..c0f7f0432ce 100644
--- a/src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs
+++ b/src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs
@@ -331,7 +331,7 @@ void AddEnvironment ()
 				BrokenExceptionTransitions = environmentParser.BrokenExceptionTransitions,
 				PackageNamingPolicy = pnp,
 				BoundExceptionType = boundExceptionType,
-				JniAddNativeMethodRegistrationAttributePresent = NativeCodeGenState.Template != null ? NativeCodeGenState.Template.JniAddNativeMethodRegistrationAttributePresent : false,
+				JniAddNativeMethodRegistrationAttributePresent = NativeCodeGenState.TemplateJniAddNativeMethodRegistrationAttributePresent,
 				HaveRuntimeConfigBlob = haveRuntimeConfigBlob,
 				NumberOfAssembliesInApk = assemblyCount,
 				BundledAssemblyNameWidth = assemblyNameWidth,
@@ -395,6 +395,14 @@ void AddEnvironment ()
 				}
 			}
 
+			if (nativeCodeGenStates is not null) {
+				// Dispose all XAAssemblyResolvers
+				Log.LogDebugMessage ($"Disposing all {nameof (NativeCodeGenState)}.{nameof (NativeCodeGenState.Resolver)}");
+				foreach	(var state in nativeCodeGenStates.Values) {
+					state.Resolver.Dispose ();
+				}
+			}
+
 			NativeCodeGenState EnsureCodeGenState (AndroidTargetArch targetArch)
 			{
 				if (nativeCodeGenStates == null || !nativeCodeGenStates.TryGetValue (targetArch, out NativeCodeGenState? state)) {
diff --git a/src/Xamarin.Android.Build.Tasks/Utilities/NativeCodeGenState.cs b/src/Xamarin.Android.Build.Tasks/Utilities/NativeCodeGenState.cs
index 96d95391a49..5a541f2d09c 100644
--- a/src/Xamarin.Android.Build.Tasks/Utilities/NativeCodeGenState.cs
+++ b/src/Xamarin.Android.Build.Tasks/Utilities/NativeCodeGenState.cs
@@ -12,7 +12,7 @@ namespace Xamarin.Android.Tasks;
 /// </summary>
 class NativeCodeGenState
 {
-	public static NativeCodeGenState? Template                 { get; set; }
+	public static bool TemplateJniAddNativeMethodRegistrationAttributePresent { get; set; }
 
 	/// <summary>
 	/// Target architecture for which this instance was created.