Revert "[One .NET] $(AndroidEnablePreloadAssemblies)=False by default"

[jonathanpeppers]

This reverts commit d13d0f9.

I also updated a test that was triggering an issue with
$(AndroidEnablePreloadAssemblies). It was originally crashing with:

Java.Lang.RuntimeException: Unable to start activity ComponentInfo{com.xamarin.classlibrarymainlauncherruns/com.xamarin.classlibrarymainlauncherruns.MainActivity}: android.view.InflateException: Binary XML file line #1 in com.xamarin.classlibrarymainlauncherruns:layout/foo: Binary XML file line #1: You must supply a layout_width attribute.
---> Android.Views.InflateException: Binary XML file line #1 in com.xamarin.classlibrarymainlauncherruns:layout/foo: Binary XML file line #1: You must supply a layout_width attribute.
---> Java.Lang.UnsupportedOperationException: Binary XML file line #1: You must supply a layout_width attribute.
--- End of managed Java.Lang.UnsupportedOperationException stack trace ---
java.lang.UnsupportedOperationException: Binary XML file line #1: You must supply a layout_width attribute.

I fixed the invalid XML in foo.xml, so the test will get a better
crash going forward:

Process: com.xamarin.classlibrarymainlauncherruns, PID: 2993
android.runtime.JavaProxyThrowable: System.NullReferenceException: Object reference not set to an instance of an object
    at MyLibrary.MainActivity.OnCreate(Bundle bundle)
    at Android.App.Activity.n_OnCreate_Landroid_os_Bundle_(IntPtr jnienv, IntPtr native__this, IntPtr native_savedInstanceState)
    at com.xamarin.classlibrarymainlauncherruns.MainActivity.n_onCreate(Native Method)
    at com.xamarin.classlibrarymainlauncherruns.MainActivity.onCreate(MainActivity.java:29)
    at android.app.Activity.performCreate(Activity.java:8000)
    at android.app.Activity.performCreate(Activity.java:7984)
    at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1309)
    at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3422)
    at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3601)
    at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:85)
    at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:135)
    at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:95)
    at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2066)
    at android.os.Handler.dispatchMessage(Handler.java:106)
    at android.os.Looper.loop(Looper.java:223)
    at android.app.ActivityThread.main(ActivityThread.java:7656)
    at java.lang.reflect.Method.invoke(Native Method)
    at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947)

This way we won't be chasing invalid XML going forward.

[jonpryor]

https://discord.com/channels/732297728826277939/732297837953679412/827199058279923712

do we want to spend any time figuring out how that test ever worked (works?!) in the first place?
feels like we might have a "false positive" scenario here

[jonathanpeppers]

@jonpryor yes, I think the actual problem here was quite different. I updated the PR description.

[jonpryor]

@grendello: don't be too quick to mark this as "good". Between this PR, d13d0f9, and decfbcc, I think this bears additional thought.

For starters, in decfbcc, why does GeneratePackageManagerJava.EnablePreloadAssembliesDefault accept the $(_AndroidEnablePreloadAssembliesDefault) property, and not $(AndroidEnablePreloadAssemblies)? Does that actually make sense?

https://github.com/xamarin/xamarin-android/blob/b595e11bf4d4dff99ca09866a79f609645d889b9/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets#L1565

GeneratePackageManagerJava.EnablePreloadAssembliesDefault is what actually sets the required bits in libxamarin-app.so:

• https://github.com/xamarin/xamarin-android/blob/b595e11bf4d4dff99ca09866a79f609645d889b9/src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs#L163
• https://github.com/xamarin/xamarin-android/blob/b595e11bf4d4dff99ca09866a79f609645d889b9/src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs#L274

Thus, I think the actual fix should instead be:

diff --git a/src/Xamarin.Android.Build.Tasks/MSBuild/Xamarin/Android/Xamarin.Android.Designer.targets b/src/Xamarin.Android.Build.Tasks/MSBuild/Xamarin/Android/Xamarin.Android.Designer.targets
index 7eee40439..f47d8e9d9 100644
--- a/src/Xamarin.Android.Build.Tasks/MSBuild/Xamarin/Android/Xamarin.Android.Designer.targets
+++ b/src/Xamarin.Android.Build.Tasks/MSBuild/Xamarin/Android/Xamarin.Android.Designer.targets
@@ -137,7 +137,7 @@ Copyright (C) 2016 Xamarin. All rights reserved.
       IsBundledApplication="$(BundleAssemblies)"
       SupportedAbis="$(_BuildTargetAbis)"
       AndroidPackageName="$(_AndroidPackage)"
-      EnablePreloadAssembliesDefault="$(_AndroidEnablePreloadAssembliesDefault)"
+      EnablePreloadAssembliesDefault="$(AndroidEnablePreloadAssemblies)"
       InstantRunEnabled="$(_InstantRunEnabled)">
     <Output TaskParameter="BuildId" PropertyName="_XamarinBuildId" />
   </GeneratePackageManagerJava>
diff --git a/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets b/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets
index a9fcc31f1..6ed26ed22 100644
--- a/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets
+++ b/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets
@@ -1562,7 +1562,7 @@ because xbuild doesn't support framework reference assemblies.
     IsBundledApplication="$(BundleAssemblies)"
     SupportedAbis="@(_BuildTargetAbis)"
     AndroidPackageName="$(_AndroidPackage)"
-    EnablePreloadAssembliesDefault="$(_AndroidEnablePreloadAssembliesDefault)"
+    EnablePreloadAssembliesDefault="$(AndroidEnablePreloadAssemblies)"
     PackageNamingPolicy="$(AndroidPackageNamingPolicy)"
     BoundExceptionType="$(AndroidBoundExceptionType)"
     InstantRunEnabled="$(_InstantRunEnabled)"

@grendello: does ^^ seem better/more consistent?

[jonathanpeppers]

It seems like this is the target that actually turns the setting on or off:
https://github.com/xamarin/xamarin-android/blob/b595e11bf4d4dff99ca09866a79f609645d889b9/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets#L1509-L1524

<GeneratePackageManagerJava/> needs to know the default value the runtime expects.

Is that correct @grendello?

[grendello]

It seems like this is the target that actually turns the setting on or off:

https://github.com/xamarin/xamarin-android/blob/b595e11bf4d4dff99ca09866a79f609645d889b9/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets#L1509-L1524

<GeneratePackageManagerJava/> needs to know the default value the runtime expects.

Is that correct @grendello?

Yes, that is correct. We need to know both the default and the actual setting. That's why we pass $(_AndroidEnablePreloadAssembliesDefault) to GeneratePackageManagerJava and not $(AndroidEnablePreloadAssemblies). This is to ascertain that the default setting is actually the default when runtime starts and $(AndroidEnablePreloadAssemblies) is absent or empty

[jonpryor]

On a related point, I think there's a bug here: https://github.com/xamarin/xamarin-android/blob/b595e11bf4d4dff99ca09866a79f609645d889b9/src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs#L209-L210

shouldn't line 210 use val == 1, not idx == 1? idx is the index within lineToWrite for where = is, which (1) will never be 1, and (2) doesn't make sense when setting usesAssemblyPreload.

Thus, as-is, usesAssemblyPreload is set incorrectly based on Environment files. But _GenerateEnvironmentFiles works in terms of editing/creating an environment file.

¯_(ツ)_/¯

[jonpryor]

@jonathanpeppers wrote:

<GeneratePackageManagerJava/> needs to know the default value the runtime expects.

I'm not sure I understand this? As-is, this implies that we have "two sources of knowledge":

1. The build system, presumably via $(AndroidEnablePreloadAssemblies), and
2. The runtime, presumably somewhere within src/monodroid?

This doesn't make sense to me, particularly given that the <GeneratePackageManagerJava/> task also uses ApplicationConfigNativeAssemblyGenerator, to create the environment.{abi}.s files, compiled into into libxamarin-app.so.

As far as I know/can easily find, src/monodroid doens't have "it's own" default value for uses_assembly_preload. The only value for uses_assembly_preload comes from libxamarin-app.so, which in turn comes from <GenreratePackageManagerJava/>, which in turn is coming from -- currently! -- $(_AndroidEnablePreloadAssembliesDefault).

[jonpryor]

In a pre-d13d0f972a4eb663251b2a6b5a934c59a6789db1 world, how it looks like it worked was:

1. $(_AndroidEnablePreloadAssembliesDefault) was always True.
2. If an app did not override $(AndroidEnablePreloadAssemblies), then <GeneratePackageManagerJava EnablePreloadAssembliesDefault="True" />, and libxamarin-app.so has uses_assembly_preload=1.
3. If an app did override $(AndroidEnablePreloadAssemblies)=False, then we still execute <GeneratePackageManagerJava EnablePreloadAssembliesDefault="True" />, and <GeneratePackageManagerJava/> also parses __environment__.txt, which sets usesAssemblyPreload=false. Thus, libxamarin-app.so has uses_assembly_preload=0`.

From a documented customer perspective, it worked. (Yay.)

"However", if you knew the internals, the other issue arises: What should happen if you add an @(AndroidEnvironment) file which contains the line:

mono.enable_assembly_preload=1

The result is that uses_assembly_preload=0! (This is due to using idx==1 and not val==1.)

However, this is internal; we never documented mono.enable_assembly_preload, only $(AndroidEnablePreloadAssemblies), so this "weird mismatch" is confusing, but not "important". (This should still be fixed, to reduce confusion.)

[jonpryor]

In a post- d13d0f9 world, with .NET 6:

1. $(_AndroidEnablePreloadAssembliesDefault) is False by default.

2. $(AndroidEnablePreloadAssemblies) is False by default.

3. If an app did not override $(AndroidEnablePreloadAssemblies), then <GeneratePackageManagerJava EnablePreloadAssembliesDefault="False" />, and libxamarin-app.so has uses_assembly_preload=0.

4. If an app did override $(AndroidEnablePreloadAssemblies)=True, then we still execute <GeneratePackageManagerJava EnablePreloadAssembliesDefault="False" />, and <GeneratePackageManagerJava/> also parses __environment__.txt -- which doesn't set mono.enable_assembly_preload=1 -- and libxamarin-app.so still has uses_assembly_preload=0.

   This is a (the?) bug: when using .NET 6, $(AndroidEnablePreloadAssemblies)=True can't be expressed.

[jonpryor]

I still don't understand how ClassLibraryMainLauncherRuns enters the picture, but I do see two related issues:

1. in .NET 6, $(AndroidEnablePreloadAssemblies)=True doesn't set uses_assembly_preload=1.
2. If an environment file ever specifies mono.enable_assembly_preload=1, this isn't parsed properly.

[jonathanpeppers]

ClassLibraryMainLauncherRuns is only relevant in that it is failing on main since d13d0f9.

After fixing foo.xml, I get the NullReferenceException because MyLibrary.dll does not seem to be loaded at the time ResourceManagerId.UpdateIdValues() runs.

https://github.com/xamarin/xamarin-android/blob/b595e11bf4d4dff99ca09866a79f609645d889b9/src/Mono.Android/Android.Runtime/ResourceIdManager.cs#L9

So when Resource.Layout.Main is called, it has the wrong value which unluckily loads foo.xml instead:

https://github.com/xamarin/xamarin-android/blob/b595e11bf4d4dff99ca09866a79f609645d889b9/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/MainActivity.cs#L22

[jonathanpeppers]

Either way, we can fix this after my vacation next week -- d13d0f9 isn't on our .NET 6 Preview 3 release branch.

[jonpryor]

@jonathanpeppers wrote:

I get the NullReferenceException because MyLibrary.dll does not seem to be loaded at the time ResourceManagerId.UpdateIdValues() runs.

Shouldn't 9e6ce03 fix that, and likewise avoid the call to ResourceManagerId.UpdateIdValues()? Or is the NullReferenceException happening in debug builds, where 9e6ce03 doesn't run?

If so, should we consider some form of 9e6ce03 on every build?

[jonathanpeppers]

@jonpryor as we discussed on our call today, I changed this to revert d13d0f9, and then also updated 1 test.

[jonathanpeppers]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).