From 9eaca87e72d90c219948e05596e8d1d980649417 Mon Sep 17 00:00:00 2001
From: Jonathan Peppers <jonathan.peppers@microsoft.com>
Date: Tue, 19 Nov 2024 21:24:24 -0600
Subject: [PATCH 1/4] [Xamarin.Android.Build.Tasks] close `XAAssemblyResolvers`

Context: https://discord.com/channels/732297728826277939/732297916680765551/1308554103206580244
Fixes: https://github.com/dotnet/android/issues/9133
Context: 86260ed3

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` 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:

    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.

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/>` MSBuild task.
---
 src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs | 7 ++++++-
 .../Tasks/GeneratePackageManagerJava.cs                    | 2 +-
 .../Utilities/NativeCodeGenState.cs                        | 2 +-
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs b/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
index bc1324a5957..ba882a98c00 100644
--- a/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
+++ b/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
@@ -222,7 +222,7 @@ void Run (bool useMarshalMethods)
 			}
 			JCWGenerator.EnsureAllArchitecturesAreIdentical (Log, nativeCodeGenStates);
 
-			NativeCodeGenState.Template = templateCodeGenState;
+			NativeCodeGenState.TemplateJniAddNativeMethodRegistrationAttributePresent = templateCodeGenState.JniAddNativeMethodRegistrationAttributePresent;
 			BuildEngine4.RegisterTaskObjectAssemblyLocal (ProjectSpecificTaskObjectKey (NativeCodeGenStateRegisterTaskKey), nativeCodeGenStates, RegisteredTaskObjectLifetime.Build);
 
 			if (useMarshalMethods) {
@@ -270,6 +270,11 @@ void Run (bool useMarshalMethods)
 			IList<string> additionalProviders = MergeManifest (templateCodeGenState, MaybeGetArchAssemblies (userAssembliesPerArch, templateCodeGenState.TargetArch));
 			GenerateAdditionalProviderSources (templateCodeGenState, additionalProviders);
 
+			// Dispose all XAAssemblyResolvers
+			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..beb645c27b2 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,
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.

From f762c612ce00d45ce849678993acf457ccc79ce3 Mon Sep 17 00:00:00 2001
From: Jonathan Peppers <jonathan.peppers@microsoft.com>
Date: Wed, 20 Nov 2024 08:46:18 -0600
Subject: [PATCH 2/4] Conditionally Dispose() if using marshal methods

Then dispose later if appropriate
---
 .../Tasks/GenerateJavaStubs.cs                    | 15 ++++++++++-----
 .../Tasks/GeneratePackageManagerJava.cs           |  8 ++++++++
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs b/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
index ba882a98c00..3ef0d0cd478 100644
--- a/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
+++ b/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
@@ -223,8 +223,6 @@ void Run (bool useMarshalMethods)
 			JCWGenerator.EnsureAllArchitecturesAreIdentical (Log, nativeCodeGenStates);
 
 			NativeCodeGenState.TemplateJniAddNativeMethodRegistrationAttributePresent = templateCodeGenState.JniAddNativeMethodRegistrationAttributePresent;
-			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.
@@ -270,9 +268,16 @@ void Run (bool useMarshalMethods)
 			IList<string> additionalProviders = MergeManifest (templateCodeGenState, MaybeGetArchAssemblies (userAssembliesPerArch, templateCodeGenState.TargetArch));
 			GenerateAdditionalProviderSources (templateCodeGenState, additionalProviders);
 
-			// Dispose all XAAssemblyResolvers
-			foreach (var state in nativeCodeGenStates.Values) {
-				state.Resolver.Dispose ();
+			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)
diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs b/src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs
index beb645c27b2..898a895e50a 100644
--- a/src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs
+++ b/src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs
@@ -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)) {

From bff43ff638c237e88e837e14fc2eaadeee936517 Mon Sep 17 00:00:00 2001
From: Jonathan Peppers <jonathan.peppers@microsoft.com>
Date: Wed, 20 Nov 2024 21:14:26 -0600
Subject: [PATCH 3/4] Update GenerateJavaStubs.cs

---
 src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs b/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
index 3ef0d0cd478..cac0e7f7747 100644
--- a/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
+++ b/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
@@ -222,7 +222,6 @@ void Run (bool useMarshalMethods)
 			}
 			JCWGenerator.EnsureAllArchitecturesAreIdentical (Log, nativeCodeGenStates);
 
-			NativeCodeGenState.TemplateJniAddNativeMethodRegistrationAttributePresent = templateCodeGenState.JniAddNativeMethodRegistrationAttributePresent;
 			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.
@@ -260,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");

From 79f6359b9e575016360c31021f75c070c8629e8f Mon Sep 17 00:00:00 2001
From: Jonathan Peppers <jonathan.peppers@microsoft.com>
Date: Thu, 21 Nov 2024 08:21:02 -0600
Subject: [PATCH 4/4] formatting

---
 src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs    | 4 ++--
 .../Tasks/GeneratePackageManagerJava.cs                       | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs b/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
index cac0e7f7747..7c13c195b16 100644
--- a/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
+++ b/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
@@ -272,11 +272,11 @@ void Run (bool useMarshalMethods)
 
 			if (useMarshalMethods) {
 				// Save NativeCodeGenState for <GeneratePackageManagerJava/> task later
-				Log.LogDebugMessage($"Saving {nameof (NativeCodeGenState)} to {nameof (NativeCodeGenStateRegisterTaskKey)}");
+				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)}");
+				Log.LogDebugMessage ($"Disposing all {nameof (NativeCodeGenState)}.{nameof (NativeCodeGenState.Resolver)}");
 				foreach (var state in nativeCodeGenStates.Values) {
 					state.Resolver.Dispose ();
 				}
diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs b/src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs
index 898a895e50a..c0f7f0432ce 100644
--- a/src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs
+++ b/src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs
@@ -397,7 +397,7 @@ void AddEnvironment ()
 
 			if (nativeCodeGenStates is not null) {
 				// Dispose all XAAssemblyResolvers
-				Log.LogDebugMessage($"Disposing all {nameof (NativeCodeGenState)}.{nameof (NativeCodeGenState.Resolver)}");
+				Log.LogDebugMessage ($"Disposing all {nameof (NativeCodeGenState)}.{nameof (NativeCodeGenState.Resolver)}");
 				foreach	(var state in nativeCodeGenStates.Values) {
 					state.Resolver.Dispose ();
 				}