[monodroid] Properly process satellite assemblies

[grendello]

Fixes: #7819

Our native runtime uses a cache of pointers to loaded managed assembly
images (essentially an array of the native struct MonoImage pointers)
which is pre-allocated at build time and placed in the native
libxamarin-app.so library.

While generating the cache, we also generate hashes for a number of
assembly name permutations (currently two per assembly - with and
without the extension). Only unique assembly names are considered when
generating the cache (it's possible to have duplicate names, because we
package more than one copy of some assemblies - those which are
architecture specific). This algorithm had a bug which made it ignore
culture prefix in satellite assembly names (e.g.
en/MyAssembly.resources.dll) and instead of several entries for each
culture, we generated only two entries for MyAssembly.resources.dll
and MyAssembly.resources but we still counted each culture-prefixed
assembly and stored that number in libxamarin-app.so to be used at
runtime to calculate number of entries in the cache.

This made the array storing cached MonoImage* pointers to be smaller
than the number of actual assemblies in the APK times 2 and in some
cases we failed to look up pointer to some images and, as the result,
passed a NULL pointer to MonoVM which then caused a segmentation fault
trying to dereference the pointer.

Stop ignoring the culture prefix for satellite assemblies in order to
avoid the situation. Additionally, since the previous assumption that
MonoVM will validate all pointers passed to its APIs turned out to be
unwarranted, we now check more carefully for NULL pointers when trying
to obtain a native function pointer from the MonoVM runtime.

[jonathanpeppers]

Do you have a .binlog of what was happening here?

In other places we used %(DestinationSubDirectory) because it was item metadata we create & control what the value is:

https://github.com/xamarin/monodroid/commit/1f52d5873960cf72685b9b08dbe262fc1ad4ca13#diff-9eaf7f76682d3de57a39276db5c6229ac0dd7ea19c9b755c11c7be5d748456a5R571

I think we should just check which one we should use everywhere. /cc @dellis1972

[grendello]

These are the relevant entries for statellite assemblies as passed to GeneratePackageManagerJava. DestinationDirectory or DestinationSubPath could be used as well, but I figured RelativePath was a standard MSBuild metadata item?

Task Parameter:
                         ResolvedUserAssemblies=
                             ~/.nuget/packages/microsoft.maui.controls.core/8.0.0-preview.1.7762/lib/net8.0-android33.0/ar/Microsoft.Maui.Controls.resources.dll
                                     AdditionalProperties=RuntimeIdentifier=android-arm;
                             _ComputeFilesToPublishForRuntimeIdentifiers=true
                             ;AppendRuntimeIdentifierToOutputPath=true
                             ;SkipCompilerExecution=true
                             ;_OuterIntermediateAssembly=obj/Release/net8.0-android/issue7819.dll
                             ;_OuterOutputPath=bin/Release/net8.0-android/
                             ;_OuterIntermediateOutputPath=obj/Release/net8.0-android/
                           
                                     AssetType=resources
                                     CopyLocal=true
                                     CopyToPublishDirectory=PreserveNewest
                                     Culture=ar
                                     DestinationSubDirectory=ar/
                                     DestinationSubPath=ar/Microsoft.Maui.Controls.resources.dll
                                     FrameworkAssembly=False
                                     HasMonoAndroidReference=False
                                     MSBuildSourceProjectFile=~/.../issue7819/issue7819.csproj
                                     MSBuildSourceTargetName=_ComputeFilesToPublishForRuntimeIdentifiers
                                     NuGetPackageId=Microsoft.Maui.Controls.Core
                                     NuGetPackageVersion=8.0.0-preview.1.7762
                                     OriginalItemSpec=issue7819.csproj
                                     PathInPackage=lib/net8.0-android33.0/ar/Microsoft.Maui.Controls.resources.dll
                                     RelativePath=ar/Microsoft.Maui.Controls.resources.dll
                                     RuntimeIdentifier=android-arm
                             ~/.nuget/packages/microsoft.maui.controls.core/8.0.0-preview.1.7762/lib/net8.0-android33.0/ca/Microsoft.Maui.Controls.resources.dll
                                     AdditionalProperties=RuntimeIdentifier=android-arm;
                             _ComputeFilesToPublishForRuntimeIdentifiers=true
                             ;AppendRuntimeIdentifierToOutputPath=true
                             ;SkipCompilerExecution=true
                             ;_OuterIntermediateAssembly=obj/Release/net8.0-android/issue7819.dll
                             ;_OuterOutputPath=bin/Release/net8.0-android/
                             ;_OuterIntermediateOutputPath=obj/Release/net8.0-android/
                           
                                     AssetType=resources
                                     CopyLocal=true
                                     CopyToPublishDirectory=PreserveNewest
                                     Culture=ca
                                     DestinationSubDirectory=ca/
                                     DestinationSubPath=ca/Microsoft.Maui.Controls.resources.dll
                                     FrameworkAssembly=False
                                     HasMonoAndroidReference=False
                                     MSBuildSourceProjectFile=~/.../issue7819/issue7819.csproj
                                     MSBuildSourceTargetName=_ComputeFilesToPublishForRuntimeIdentifiers
                                     NuGetPackageId=Microsoft.Maui.Controls.Core
                                     NuGetPackageVersion=8.0.0-preview.1.7762
                                     OriginalItemSpec=issue7819.csproj
                                     PathInPackage=lib/net8.0-android33.0/ca/Microsoft.Maui.Controls.resources.dll
                                     RelativePath=ca/Microsoft.Maui.Controls.resources.dll
                                     RuntimeIdentifier=android-arm

[grendello]

Note that it isn't just a path - it's the de-facto assembly name. The runtime will ask us to load an assembly named culture/Assembly.dll, so we absolutely must keep that convention

[dellis1972]

I think we should be ok to use RelativePath here, but DestinationSubPath should also work as we use that for fast deployment.

[jonathanpeppers]

We have some logic that sets %(DestinationSubPath) if the assembly is architecture-specific:

https://github.com/xamarin/xamarin-android/blob/17916001b80e9e5a963418c14fb0254497903ab3/src/Xamarin.Android.Build.Tasks/Tasks/ProcessAssemblies.cs#L235-L239

I don't think %(RelativePath) would have a value for armeabi-v7a/Assembly.dll.

[grendello]

@jonathanpeppers yeah, %(RelativePath) didn't have the arch-specific prefix

[jonpryor]

@grendello: standard-request-is-standard: icanhaz unit test? kthxbye! :-D

[jonpryor]

Commit message for review:

Fixes: https://github.com/xamarin/xamarin-android/issues/7819

Our native runtime uses a cache of pointers to loaded managed assembly
images (essentially an array of the native `struct MonoImage` pointers)
which is pre-allocated at build time and placed in the native
`libxamarin-app.so` library.

While generating the cache, we also generate hashes for a number of
assembly name permutations (currently two per assembly: with and
without the extension).  Only unique assembly names are considered when
generating the cache (it's possible to have duplicate names, because we
package more than one copy of some assemblies - those which are
architecture specific).

This algorithm had a bug which made it ignore culture prefix in
satellite assembly names (e.g. `en/MyAssembly.resources.dll`);
instead of several entries for each culture, we generated only two
entries (e.g. `MyAssembly.resources.dll` and `MyAssembly.resources`)
but we still counted each culture-prefixed assembly and stored that
number in `libxamarin-app.so` to be used at runtime to calculate number
of entries in the cache.

This made the array storing cached `MonoImage*` pointers to be smaller
than the number of actual assemblies in the APK times 2 and in some
cases we failed to look up pointer to some images and, as the result,
passed a `NULL` pointer to MonoVM which then caused a segmentation fault
trying to dereference the pointer:

	F DEBUG   : signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x000000000000001c
	F DEBUG   : Cause: null pointer dereference
	…
	F DEBUG   :       #00 pc 00000000000bdad8  /data/app/~~7gCkYmQcKwTyS9NmxfgKxA==/com.companyname.bubby-PJIkJ0Lv0RiQhEVLeWi4wg==/split_config.arm64_v8a.apk!libmonosgen-2.0.so (mono_class_get_checked+24) (BuildId: 80b786675a56824331f363bf29a2b54f454cf006)

Update the `<BuildApk/>` task to stop ignoring the culture prefix for
satellite assemblies in order to avoid the situation.  Additionally,
since the previous assumption that MonoVM will validate all pointers
passed to its APIs turned out to be unwarranted, we now check more
carefully for `NULL` pointers when trying to obtain a native function
pointer from the MonoVM runtime.

Add a test for the issue, based on the
`MissingSatelliteAssemblyInLibrary()` packaging test which, when it
fails, will result in a `SIGABRT`:

	D monodroid-assembly: assembly_store_open_from_bundles: looking for bundled name: 'System.Private.CoreLib' (hash 0x6b0ff375198b9c17)
	F monodroid-assembly: Invalid assembly index 19, exceeds the maximum index of 11
	F libc    : Fatal signal 6 (SIGABRT), code -1 (SI_QUEUE) in tid 17957 (semblyinlibrary), pid 17957 (semblyinlibrary)

Enhance the packaging test `MissingSatelliteAssemblyInLibrary()` by
adding more languages, to see if they're all packaged correctly.

[jonathanpeppers]

Do we still need to change this to use %(DestinationSubPath)? I thought we saw that architecture-specific assemblies could also hit a similar issue?

[grendello]

No, architecture specific assemblies are handled correctly (if they didn't, we'd see this issue much earlier). DestinationSubPath would only make matters worse - because I would have to know which is arch-specific assembly and which is a satellite assembly, to treat the prefix differently. RelativePath appears to be unambiguous for this purpose.

[softlion]

Is there the same issue on iOS ?

[grendello]

@softlion no, the code is specific to Xamarin.Android. If iOS shows similar issues then it's caused by something else there.