From 30b0b2ca63324a6cf90bb7bc7b951ed0d3bd52bb Mon Sep 17 00:00:00 2001
From: Jonathan Pryor <jonpryor@vt.edu>
Date: Fri, 25 Feb 2022 12:25:21 -0500
Subject: [PATCH] [Xamarin.Android.Tools.AndroidSdk] Attributes can be null!

Context: https://github.com/xamarin/xamarin-android-tools/pull/157
Context: 2d3690e428c8523b3779f84e5c804d1fd3c0d6fe

Commit 2d3690e4 added Nullable Reference Type support to
`Xamarin.Android.Tools.AndroidSdk.dll`.  Unfortunately, it was
largely "surface level", updating *public* API, but not all method
implementations were appropriately updated.

Case in point: if `$HOME/.config/xbuild/monodroid-config.xml`
contains *no* value in `<java-sdk/>`, e.g.

	<monodroid>
	  <android-sdk path="/path/to/android/sdk" />
	  <java-sdk /> <!-- empty! -->
	</monodroid>

then Visual Studio for Mac may report a first-chance exception
(only reported when debugging Visual Studio for Mac, as the exception
is caught internally):

	TODO: exception + stack trace

A major reason to adopt Nullable Reference Types is to *prevent* the
occurrence of `NullReferenceException`s, so what went wrong?

What went wrong with 2d3690e4 is that when XML attributes don't
exist, [`XElement.Attribute()`][0] will return `null`, and most of
our `XElement.Attribute()` invocations cast the `XAttribute` return
value to `string`, "asserting" that a *non-`null`* is returned.

Review the codebase for all `XElement.Attribute()` invocations, and
update all casts from `(string)` to instead cast to `(string?)`.
This ensures that we don't circumvent the C# compilers Nullable
Reference Type checks, catches the circumvention which was present
in `JdkLocations.GetUnixConfiguredJdkPaths()`, and thus avoids the
first-chance exception that VSMac could see.

[0]: https://docs.microsoft.com/en-us/dotnet/api/system.xml.linq.xelement.attribute?view=net-6.0

Co-Authored by: @KirillOsenkov
---
 .../AndroidAppManifest.cs                     | 43 +++++++++++--------
 .../JdkInfo.cs                                |  1 +
 .../Jdks/JdkLocations.MacOS.cs                |  6 ++-
 .../Sdks/AndroidSdkUnix.cs                    |  6 +--
 4 files changed, 32 insertions(+), 24 deletions(-)

diff --git a/src/Xamarin.Android.Tools.AndroidSdk/AndroidAppManifest.cs b/src/Xamarin.Android.Tools.AndroidSdk/AndroidAppManifest.cs
index 5a9e5a75..952834d2 100644
--- a/src/Xamarin.Android.Tools.AndroidSdk/AndroidAppManifest.cs
+++ b/src/Xamarin.Android.Tools.AndroidSdk/AndroidAppManifest.cs
@@ -136,37 +136,37 @@ public void WriteToFile (string fileName)
 		}
 
 		public string? PackageName {
-			get { return (string) manifest.Attribute ("package");  }
+			get { return (string?) manifest.Attribute ("package");  }
 			set { manifest.SetAttributeValue ("package", NullIfEmpty (value)); }
 		}
 
 		public string? ApplicationLabel {
-			get { return (string) application.Attribute (aNS + "label");  }
+			get { return (string?) application.Attribute (aNS + "label");  }
 			set { application.SetAttributeValue (aNS + "label", NullIfEmpty (value)); }
 		}
 
 		public string? ApplicationIcon {
-			get { return (string) application.Attribute (aNS + "icon");  }
+			get { return (string?) application.Attribute (aNS + "icon");  }
 			set { application.SetAttributeValue (aNS + "icon", NullIfEmpty (value)); }
 		}
 
 		public string? ApplicationTheme {
-			get { return (string) application.Attribute (aNS + "theme"); }
+			get { return (string?) application.Attribute (aNS + "theme"); }
 			set { application.SetAttributeValue (aNS + "theme", NullIfEmpty (value)); }
 		}
 
 		public string? VersionName {
-			get { return (string) manifest.Attribute (aNS + "versionName");  }
+			get { return (string?) manifest.Attribute (aNS + "versionName");  }
 			set { manifest.SetAttributeValue (aNS + "versionName", NullIfEmpty (value)); }
 		}
 
 		public string? VersionCode {
-			get { return (string) manifest.Attribute (aNS + "versionCode");  }
+			get { return (string?) manifest.Attribute (aNS + "versionCode");  }
 			set { manifest.SetAttributeValue (aNS + "versionCode", NullIfEmpty (value)); }
 		}
 
 		public string? InstallLocation {
-			get { return (string) manifest.Attribute (aNS + "installLocation"); }
+			get { return (string?) manifest.Attribute (aNS + "installLocation"); }
 			set { manifest.SetAttributeValue (aNS + "installLocation", NullIfEmpty (value)); }
 		}
 
@@ -182,8 +182,8 @@ public int? TargetSdkVersion {
 
 		int? ParseSdkVersion (XAttribute attribute)
 		{
-			var version = (string)attribute;
-			if (string.IsNullOrEmpty (version))
+			var version = (string?) attribute;
+			if (version == null || string.IsNullOrEmpty (version))
 				return null;
 			int vn;
 			if (!int.TryParse (version, out vn)) {
@@ -198,7 +198,7 @@ public int? TargetSdkVersion {
 		public IEnumerable<string> AndroidPermissions {
 			get {
 				foreach (var el in manifest.Elements ("uses-permission")) {
-					var name = (string) el.Attribute (aName);
+					var name = (string?) el.Attribute (aName);
 					if (name == null)
 						continue;
 					var lastDot = name.LastIndexOf ('.');
@@ -211,7 +211,7 @@ public IEnumerable<string> AndroidPermissions {
 		public IEnumerable<string> AndroidPermissionsQualified {
 			get {
 				foreach (var el in manifest.Elements ("uses-permission")) {
-					var name = (string) el.Attribute (aName);
+					var name = (string?) el.Attribute (aName);
 					if (name != null)
 						yield return name;
 				}
@@ -267,7 +267,11 @@ void RemoveAndroidPermissions (IEnumerable<string> permissions)
 		{
 			var perms = new HashSet<string> (permissions);
 			var list = manifest.Elements ("uses-permission")
-				.Where (el => perms.Contains ((string)el.Attribute (aName))).ToList ();
+				.Where (el => {
+					var name = (string?) el.Attribute (aName);
+					return name != null && perms.Contains (name);
+				})
+				.ToList ();
 			foreach (var el in list)
 				el.Remove ();
 		}
@@ -284,7 +288,7 @@ void RemoveAndroidPermissions (IEnumerable<string> permissions)
 		{
 			string? first = null;
 			foreach (var a in GetLaunchableActivities ()) {
-				var name = (string) a.Attribute (aName);
+				var name = (string?) a.Attribute (aName);
 				//prefer the fastdev launcher, it's quicker
 				if (name == "mono.android.__FastDevLauncher") {
 					return name;
@@ -303,7 +307,7 @@ void RemoveAndroidPermissions (IEnumerable<string> permissions)
 		public string? GetLaunchableUserActivityName ()
 		{
 			return GetLaunchableActivities ()
-				.Select (a => (string) a.Attribute (aName))
+				.Select (a => (string?) a.Attribute (aName))
 				.FirstOrDefault (name => !string.IsNullOrEmpty (name) && name != "mono.android.__FastDevLauncher");
 		}
 
@@ -313,7 +317,7 @@ IEnumerable<XElement> GetLaunchableActivities ()
 				var filter = activity.Element ("intent-filter");
 				if (filter != null) {
 					foreach (var category in filter.Elements ("category"))
-						if (category != null && (string)category.Attribute (aName) == "android.intent.category.LAUNCHER")
+						if (category != null && (string?)category.Attribute (aName) == "android.intent.category.LAUNCHER")
 							yield return activity;
 				}
 			}
@@ -322,8 +326,8 @@ IEnumerable<XElement> GetLaunchableActivities ()
 		public IEnumerable<string> GetAllActivityNames ()
 		{
 			foreach (var activity in application.Elements ("activity")) {
-				var activityName = (string) activity.Attribute (aName);
-				if (activityName != "mono.android.__FastDevLauncher")
+				var activityName = (string?) activity.Attribute (aName);
+				if (activityName != null && activityName != "mono.android.__FastDevLauncher")
 					yield return activityName;
 			}
 		}
@@ -331,8 +335,9 @@ public IEnumerable<string> GetAllActivityNames ()
 		public IEnumerable<string> GetLaunchableActivityNames ()
 		{
 			return GetLaunchableActivities ()
-				.Select (a => (string) a.Attribute (aName))
-				.Where (name => !string.IsNullOrEmpty (name) && name != "mono.android.__FastDevLauncher");
+				.Select (a => (string?) a.Attribute (aName))
+				.Where (name => !string.IsNullOrEmpty (name) && name != "mono.android.__FastDevLauncher")
+				.Select (name => name!);
 		}
 	}
 }
diff --git a/src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs b/src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs
index f1ba5259..a6c86660 100644
--- a/src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs
+++ b/src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs
@@ -285,6 +285,7 @@ static Dictionary<string, List<string>> GetJavaProperties (Action<TraceLevel, st
 					if (e.Data.StartsWith (ContinuedValuePrefix, StringComparison.Ordinal)) {
 						if (curKey == null) {
 							logger (TraceLevel.Error, $"No Java property previously seen for continued value `{e.Data}`.");
+							return;
 						}
 						props [curKey].Add (e.Data.Substring (ContinuedValuePrefix.Length));
 						return;
diff --git a/src/Xamarin.Android.Tools.AndroidSdk/Jdks/JdkLocations.MacOS.cs b/src/Xamarin.Android.Tools.AndroidSdk/Jdks/JdkLocations.MacOS.cs
index dc2a8f5c..0559c9de 100644
--- a/src/Xamarin.Android.Tools.AndroidSdk/Jdks/JdkLocations.MacOS.cs
+++ b/src/Xamarin.Android.Tools.AndroidSdk/Jdks/JdkLocations.MacOS.cs
@@ -21,8 +21,10 @@ static IEnumerable<string> GetUnixConfiguredJdkPaths (Action<TraceLevel, string>
 		{
 			var config = AndroidSdkUnix.GetUnixConfigFile (logger);
 			foreach (var java_sdk in config.Root.Elements ("java-sdk")) {
-				var path    = (string) java_sdk.Attribute ("path");
-				yield return path;
+				var path    = (string?) java_sdk.Attribute ("path");
+				if (path != null && !string.IsNullOrEmpty (path)) {
+					yield return path;
+				}
 			}
 		}
 
diff --git a/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs b/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs
index 2f3b2f9c..e3b8abac 100644
--- a/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs
+++ b/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs
@@ -49,7 +49,7 @@ public override string? PreferedAndroidSdkPath {
 				var androidEl = config_file.Root.Element ("android-sdk");
 
 				if (androidEl != null) {
-					var path = (string)androidEl.Attribute ("path");
+					var path = (string?)androidEl.Attribute ("path");
 
 					if (ValidateAndroidSdkLocation (path))
 						return path;
@@ -64,7 +64,7 @@ public override string? PreferedAndroidNdkPath {
 				var androidEl = config_file.Root.Element ("android-ndk");
 
 				if (androidEl != null) {
-					var path = (string)androidEl.Attribute ("path");
+					var path = (string?)androidEl.Attribute ("path");
 
 					if (ValidateAndroidNdkLocation (path))
 						return path;
@@ -79,7 +79,7 @@ public override string? PreferedJavaSdkPath {
 				var javaEl = config_file.Root.Element ("java-sdk");
 
 				if (javaEl != null) {
-					var path = (string)javaEl.Attribute ("path");
+					var path = (string?)javaEl.Attribute ("path");
 
 					if (ValidateJavaSdkLocation (path))
 						return path;
