Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.NET 6: $(AndroidEnablePreloadAssemblies)=False by default #5838

Closed
jonpryor opened this issue Apr 13, 2021 · 3 comments · Fixed by #5914
Closed

.NET 6: $(AndroidEnablePreloadAssemblies)=False by default #5838

jonpryor opened this issue Apr 13, 2021 · 3 comments · Fixed by #5914
Assignees
Milestone

Comments

@jonpryor
Copy link
Member

Context: d13d0f9
Context: b90d3ab
Context: PR #5790
Context: PR #5811
Context? 9e6ce03

For .NET 6 we want $(AndroidEnablePreloadAssemblies)=False by default, so that all assemblies don't need to be loaded as part of process startup.

This was done in PR #5790 / d13d0f9 , after which point we found a problem (missed during PR review, alas):

  1. .NET 6 +
  2. "Debug" build +
  3. Fast Deployment +
  4. App launch

resulted in a crash (!) in DebuggingTest.ClassLibraryMainLauncherRuns():

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)

The cause of the crash is that, because not all assemblies are loaded as part of process startup, ResourceIdManager.UpdateIdValues() isn't able to update all fields in all Resource types to have the values from the app Resource.designer.cs file. Consequently, the values may be invalid, thus the NullReferenceException.

In order to actually default $(AndroidEnablePreloadAssemblies)=False for .NET 6, we need to figure out how to make this work.

Perhaps we can enable $(AndroidLinkResources)=True (9e6ce03) for all .NET 6 builds? What would the build-time impacts be?

@jonpryor jonpryor added this to the One .NET milestone Apr 13, 2021
jonpryor pushed a commit that referenced this issue Apr 14, 2021
#5811)

Context: #5838

This reverts commit d13d0f9.

Commit d13d0f9 inadvertently broke the
[`DebuggingTest.ClassLibraryMainLauncherRuns()` unit test][0], when
using a test app which:

 1. Uses .NET 6, which -- because of d13d0f9 -- sets
    `$(AndroidEnablePreloadAssemblies)`=False by default -- with-

 2. A "Debug" build (Fast Deployment enabled), with-

 3. `$(AndroidLinkResources)`=False (the default; see also 9e6ce03),
    and-

 4. launching the app, which

 5. Loads an `Activity` subclass located in an assembly which is
    *not* the "main" App assembly

then the app crashes during process startup:

	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)

The cause of the crash is that, because not all assemblies are loaded
as part of process startup -- the whole *point* to d13d0f9 and
defaulting `$(AndroidEnablePreloadAssemblies)`=False -- then
[`ResourceIdManager.UpdateIdValues()`][1] isn't able to update all
fields in all `Resource` types to have the values from the app's
`Resource.designer.cs` file.  Consequently, the values *may* be
invalid, and thus the `NullReferenceException`.

As an "immediate" fix is not quickly forthcoming -- though enabling
`$(AndroidLinkResources)`=False *by default* is an "interesting"
solution, with unknown inner dev loop performance implications --
we're reverting commit d13d0f9.

Issue #5838 will track the re-enabling of
`$(AndroidEnablePreloadAssemblies)`=False by default in .NET 6 apps.

~~~

Additionally, update `DebuggingTest.ClassLibraryMainLauncherRuns()`
so that `Resources\layout\foo.xml` contains *valid* layout XML.
It was originally:

	<?xml version="1.0" encoding="utf-8" ?>
	<LinearLayout
	  xmlns:android="http://schemas.android.com/apk/res/android"
	/>

which would crash 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.

because the layout XML was, in fact, invalid, as it wasn't providing
the required `android:layout_width` attribute.

Update `foo.xml` so that it has valid layout XML:

	<?xml version="1.0" encoding="utf-8"?>
	<LinearLayout
	  xmlns:android="http://schemas.android.com/apk/res/android"
	  android:layout_width="fill_parent"
	  android:layout_height="wrap_content"
	/>"

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

The above `UnsupportedOperationException` *hid* the
`NullReferenceException` in d13d0f9; the `NullReferenceException`
wasn't visible until after the invalid XML was fixed.

[0]: https://github.com/xamarin/xamarin-android/blob/bf4f4f42af26cdddb46087deed59aae8424f7942/tests/MSBuildDeviceIntegration/Tests/DebuggingTest.cs#L74-L124
[1]: https://github.com/xamarin/xamarin-android/blob/bf4f4f42af26cdddb46087deed59aae8424f7942/src/Mono.Android/Android.Runtime/ResourceIdManager.cs#L9-L31
@jonpryor
Copy link
Member Author

I thought I had an idea of a workaround: we should add a handler to the AppDomain.AssemblyLoad event, and fixup resource ids on the assembly when it's loaded.

Then while discussing with @dellis1972 we realized that none of this makes any sense: ResourceIdManager.UpdateIdValues() calls the generated Resource.UpdateIdValues() method, which is e.g.

// From the tests/Xamarin.Forms-Performance-Integration build
namespace Xamarin.Forms.Performance.Integration.Droid {
	public partial class Resource {
		public static void UpdateIdValues()
		{
			global::Xamarin.Forms.Platform.Android.Resource.Animation.abc_fade_in = global::Xamarin.Forms.Performance.Integration.Droid.Resource.Animation.abc_fade_in;
			global::Xamarin.Forms.Platform.Android.Resource.Animation.abc_fade_out = global::Xamarin.Forms.Performance.Integration.Droid.Resource.Animation.abc_fade_out;
			global::Xamarin.Forms.Platform.Android.Resource.Animation.abc_grow_fade_in_from_bottom = global::Xamarin.Forms.Performance.Integration.Droid.Resource.Animation.abc_grow_fade_in_from_bottom;
			// …
		}
	}
}

How does this fail? When $(AndroidEnablePreloadAssemblies) is False, "no" assemblies are preloaded, sure. However, once the above method is invoked, every referenced assembly must be loaded, simply so that the specified field can be resolved and it's value updated.

Thus, why does MainActivity.OnCreate() throw a NullReferenceException? Presumably it's because MainActivity.cs' FindViewById<Button>() invocation is returning null:

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

That still doesn't make sense though; why would button be null? (Is that the actual reason for the NullReferenceException?)

More investigation required.

@jonpryor
Copy link
Member Author

More investigation: https://discord.com/channels/732297728826277939/732297837953679412/836964276438564884

The cause of the issue is that when $(AndroidEnablePreloadAssemblies)=False, the "main" assembly isn't loaded by default. The main assembly is also the assembly which has the Resource.UpdateIdValues() method which is responsible for updating all of the other Resource values (see snippet in previous comment).

Because the main assembly isn't loaded, no assembly is found by ResourceIdManager.UpdateIdValues(), and thus Resource.UpdateIdValues() is never invoked, and thus the resource ids for non-main assemblies are never updated to contain the correct values.

Possible solutions:

  1. Always preload the "main" assembly. (The "main" assembly is the assembly created by the "App" .csproj build.)

    @dellis1972 is looking into this now.

  2. Otherwise "change" ResourceIdManager.UpdateIdValues() logic so that instead of probing all loaded assemblies for the Resource.UpdateIdValues() method to execute, we specify the type to use "elsewhere", e.g. environment variable, libxamarin-app.so value, etc.

  3. ...?

dellis1972 added a commit to dellis1972/xamarin-android that referenced this issue Apr 28, 2021
Fixes dotnet#5838

Turning off Assembly preloading results in the following runtime error

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

This only occurs on one unit test [`DebuggingTest.ClassLibraryMainLauncherRuns()`](https://github.com/xamarin/xamarin-android/blob/bf4f4f42af26cdddb46087deed59aae8424f7942/tests/MSBuildDeviceIntegration/Tests/DebuggingTest.cs#L74-L124)
This happens because the `MainActivity` in that test is in a library project.
As a result when we do not preload assemblies the main assembly is NEVER
loaded. This means `ResourceIdManager.UpdateIdValues()` never finds the
`Resource` class from the main assembly, and as such none of the ids are
updated.

To fix this , when running without assembly preloading we will ALWAYS
load the first assembly in the assembly list. This will always be the
main assembly and will contain the `Resource` class.
@dellis1972
Copy link
Contributor

Work around #5883

jonpryor pushed a commit that referenced this issue Apr 30, 2021
…5883)

Context: #5838

Setting `$(AndroidEnablePreloadAssemblies)`=False, which disables
assembly preloading (see af02a65), could result in a crash when:

 1. The launched `Activity` isn't located within the "main App
    assembly", the assembly built as part of the `App.apk` build, and

 2. The launched `Activity` uses Android Resource IDs, and

 3. Those Android Resource IDs differ between the Library project
    build and the Application project build (usually the case).

This setup is exemplified by the
`DebuggingTest.ClassLibraryMainLauncherRuns()` unit test.

Our current Resource architecture assumes/ensures that there is
Only One™ `Resource` type mentioned by an assembly-level
`ResourceDesignerAttribute` custom attribute, with
`ResourceDesignerAttribute.IsApplication`=true:

	[assembly: global::Android.Runtime.ResourceDesignerAttribute("My.Resource", IsApplication=true)]

The `ResourceIdManager.UpdateIdValues()` method looks for this
attribute in all loaded assemblies, and when (if) found, will
invoke the `UpdateIdValues()` static method on the specified type.

The problem is that only the *App assembly* `Resource.UpdateIdValues()`
method does anything, and if the App assembly isn't loaded -- which
may be the case when the `Activity` loaded isn't from the App assembly
and assembly preload is disabled -- then:

 1. `ResourceIdManager.UpdateIdValues()` never finds the
    `ResourceDesignerAttribute`, and

 2. `Resource.UpdateIdValues()` is never invoked, and

 3. Android Resource IDs in non-App assemblies are never updated to
    contain their correct values.

Thus, attempting to use an Android resource may result in unexpected
failures:

	var button = FindViewById<Button> (Resource.Id.myButton);
	// `button` is null, because `Resource.Id.myButton` has the wrong value.

This can result in `NullReferenceException`s:

	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)

Fix this by *always* loading the App assembly during process startup,
which also happens to be the *first* assembly listed within the
generated `mono.MonoPackageManager_Resources.Assemblies` array, which
is provided to `Runtime.initInternal()`.
grendello added a commit to grendello/xamarin-android that referenced this issue May 5, 2021
…#5790)

Fixes: dotnet#5838
Context: e0da1f1

Partially reverts 522d7fb which
reverted d13d0f9

The [`$(AndroidEnablePreloadAssemblies)`][0] property controls
whether or not *all* `.dll` files contained within a `.apk` are
loaded during process startup.  *Not* doing so reduces process
startup times, which is desirable, but this also caused certain
Xamarin.Forms apps to fail to run as they previously had, as
not loading all assemblies during startup broke their
Dependency Injection infrastructure.

For .NET 6, we feel we have *some* "wiggle-room" to change default
semantics, so for .NET 6 projects set the default value of
`$(AndroidEnablePreloadAssemblies)` to False, so that assemblies are
*not* pre-loaded during process startup.

`$(AndroidEnablePreloadAssemblies)` can be set to True within the
app's `.csproj` file to return to "legacy" semantics.  This will
cause all assemblies to be loaded during startup, with a
commensurate increase in app startup overheads.

[0]: https://docs.microsoft.com/en-us/xamarin/android/deploy-test/building-apps/build-properties#androidenablepreloadassemblies
jonpryor pushed a commit that referenced this issue May 7, 2021
…#5914)

Fixes: #5838
Context: e0da1f1

Partially reverts 522d7fb which, reverted d13d0f9.

The [`$(AndroidEnablePreloadAssemblies)`][0] property controls whether
or not *all* `.dll` files contained within a `.apk` are loaded during
process startup.  *Not* doing so reduces process startup times, which
is desirable, but this also caused certain Xamarin.Forms apps to fail
to run as they previously had, as not loading all assemblies during
startup broke their Dependency Injection infrastructure.

For .NET 6, we feel we have *some* "wiggle-room" to change default
semantics, so for .NET 6 projects set the default value of
`$(AndroidEnablePreloadAssemblies)` to False, so that assemblies are
*not* pre-loaded during process startup.

`$(AndroidEnablePreloadAssemblies)` can be set to True within the
app's `.csproj` file to return to "legacy" semantics.  This will cause
all assemblies to be loaded during startup, with a commensurate
increase in app startup overheads.

[0]: https://docs.microsoft.com/en-us/xamarin/android/deploy-test/building-apps/build-properties#androidenablepreloadassemblies
@ghost ghost locked as resolved and limited conversation to collaborators Jun 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants