Skip to content

Conversation

@jonpryor
Copy link
Contributor

@jonpryor jonpryor commented Dec 22, 2021

[Mono.Android] Type & Member Remapping Support

Context: dotnet/java-interop#936
Context: #6142 (comment)

Fixes: dotnet/java-interop#867

~~ The Scenarios ~~

Our Java binding infrastructure involves looking up types and methods
via JNI, and assumes that types and methods won't "move" in an
unexpected manner. Methods which move from a subclass to a
superclass works transparently. Methods which are moved to an
entirely different class causes us problems.

Case in point: desugaring, which can move Java types to places
that our bindings don't expect. For example, Arrays.stream(T[])
may be moved into a DesugarArrays class, or a default interface
method Example.m() may be moved into an Example$-CC type.
Java.Interop has not supported such changes, resulting in throwing a
Java.Lang.NoSuchMethodError on Android versions where methods are
not where we expect.

Additionally, the InTune Mobile Application Management team
needs a expanded type and member lookup mechanism in order to
simplify how they maintain their product. Currently they make things
work by rewriting IL, which can be brittle.

~~ The Pieces ~~

To improve support for this, dotnet/java-interop#936 introduces
new virtual methods into Java.Interop.JniRuntime.JniTypeManager
which are called as part of type and member lookup, allowing
AndroidTypeManager to participate in the type and member resolution
process.

AndroidTypeManager in turn needs to know what types and members can
be remapped, and what they should be remapped to. Some of these can
be algorithmic, such as pre-pending Desugar or appending $-CC for
the Desugar case.

The InTune use case involves a table, contained within the
Microsoft.Intune.MAM.Remapper.Tasks NuGet package.

Update src/Xamarin.Android.Build.Tasks to add a new
@(_AndroidMamMappingFile) Build action. Files with this build
action are expected to be JSON documents which follow the current
conventions of remapping-config.json, within the
Microsoft.Intune.MAM.Remapper.Tasks NuGet package. This build
action is not externally supported; this is currently for testing
purposes. @(_AndroidMamMappingFile) files are processed into an
XML intermediary as part of the App build, and bundled into the app
as an @(AndroidAsset) named xa-internal/xa-mam-mapping.xml:

<replacements>
  <replace-type
      from="android/app/Activity"
      to="com/microsoft/intune/mam/client/app/MAMActivity" />
  <replace-method
      source-type="com/microsoft/intune/mam/client/app/MAMActivity"
      source-method-name="onCreate" source-method-signature="(Landroid/os/Bundle;)"
      target-type="com/microsoft/intune/mam/client/app/MAMActivity"
      target-method-name="onMAMCreate" target-method-instance-to-static="false" />
</replacements>

//replacements/replace-method is structured with each attribute
corresponding to a member on the JniRuntime.ReplacementMethodInfo
struct, in dotnet/java-interop#936.

  • //replace-method/@source-type is
    JniRuntime.ReplacementMethodInfo.SourceJniType
  • //replace-method/@source-method-name is
    JniRuntime.ReplacementMethodInfo.SourceJniMethodName
  • //replace-method/@source-method-signature is
    JniRuntime.ReplacementMethodInfo.SourceJniMethodSignature
  • //replace-method/@target-type is
    JniRuntime.ReplacementMethodInfo.TargetJniType
  • //replace-method/@target-method-name is
    JniRuntime.ReplacementMethodInfo.TargetJniMethodName
  • //replace-method/@target-method-signature is
    JniRuntime.ReplacementMethodInfo.SourceJniMethodSignature
  • //replace-method/@target-method-instance-to-static is
    JniRuntime.ReplacementMethodInfo.TargetJniMethodIsStatic

During app startup on the Android device, the
xa-internal/xa-mam-mapping.xml asset is opened and provided to
JNIEnv.Initialize() as part of native app startup.

~~ Putting it all together ~~

This will only work on .NET 7+.

App project has a @(_AndroidMamMappingFile) file. This item is
processed during App build, stored into the .apk, and read during
app startup on an Android device.

Given a Java binding such as:

public partial class Activity {
    static readonly JniPeerMembers _members = new XAPeerMembers ("android/app/Activity", typeof (Activity));
}

when the JniPeerMembers constructor runs, it will call
JniEnvironment.Runtime.TypeManager.GetReplacementType("android/app/Activity").
If @(_AndroidMamMappingFile) uses the InTune remapping-config.json
file, then com/microsoft/intune/mam/client/app/MAMActivity will
be resolved via JNIEnv::FindClass(), not android/app/Activity.

If MAMActivity can't be found, e.g. you're testing this all out,
the app will ~immediately crash, as MAMActivity doesn't exist. 😅

If MAMActivity can be found, eventually Activity.OnCreate() will
need to be invoked:

partial class Activity {
    protected virtual unsafe void OnCreate (Android.OS.Bundle? savedInstanceState)
    {
        const string __id = "onCreate.(Landroid/os/Bundle;)V";
        try {
            JniArgumentValue* __args = stackalloc JniArgumentValue [1];
            __args [0] = new JniArgumentValue ((savedInstanceState == null) ? IntPtr.Zero : ((global::Java.Lang.Object) savedInstanceState).Handle);
            _members.InstanceMethods.InvokeVirtualVoidMethod (__id, this, __args);
        } finally {
            global::System.GC.KeepAlive (savedInstanceState);
        }
    }
}

_members.InstanceMethods.InvokeVirtualVoidMethod() will internally
make a call similar to:

var r = JniEnvironment.Runtime.TypeManager.GetReplacementMethodInfo (
    "android/app/Activity",
    "onCreate",
    "(Landroid/os/Bundle;)V"
);

The data returned will be equivalent to:

var r = new JniRuntime.ReplacementMethodInfo {
    SourceJniType                   = "android/app/Activity",                               // from input parameter
    SourceJniMethodName             = "onCreate",                                           // from input parameter
    SourceJniMethodSignature        = "(Landroid/os/Bundle;)V",                             // from input parameter

    TargetJniType                   = "com/microsoft/intune/mam/client/app/MAMActivity",    // from //replace-method/@target-type
    TargetJniMethodName             = "onMAMCreate",                                        // from //replace-method/@target-method-name
    SourceJniMethodSignature        = "(Landroid/os/Bundle;)V",                             // from input parameter, as signature didn't change
    TargetJniMethodParameterCount   = 1,                                                    // computed based on signature
    TargetJniMethodIsStatic         = false,                                                // from //replace-method/@target-method-instance-to-static
}

This will allow _members.InstanceMethods.InvokeVirtualVoidMethod()
to instead resolve and invoke MAMActivity.onMAMCreate().

@jonpryor jonpryor force-pushed the jonp-member-remapping branch from f19aedb to 34a3cb6 Compare December 28, 2021 19:17
@jonpryor jonpryor force-pushed the jonp-member-remapping branch 2 times, most recently from 86ca249 to 9977745 Compare January 6, 2022 19:15
@jonpryor jonpryor force-pushed the jonp-member-remapping branch 3 times, most recently from 808f98c to df583c1 Compare January 21, 2022 22:05
@jonpryor jonpryor force-pushed the jonp-member-remapping branch from f65291f to 13afaf5 Compare February 2, 2022 21:49
@jonpryor jonpryor force-pushed the jonp-member-remapping branch from 13afaf5 to 7616446 Compare May 5, 2022 21:14
Comment on lines 13 to 31
<ItemGroup>
<PackageReference
Include="Microsoft.Intune.MAM.Remapper.Tasks"
Version="0.1.4635.1"
IncludeAssets="none"
GeneratePathProperty="True"
ReferenceOutputAssembly="False"
/>
</ItemGroup>
<Target Name="_AddMamFiles"
BeforeTargets="_AddAndroidCustomMetaData">
<ItemGroup>
<_AndroidMamMappingFile Include="$(PkgMicrosoft_Intune_MAM_Remapper_Tasks)/content/MonoAndroid10/remapping-config.json" />
</ItemGroup>
</Target>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we really put this in the Hello World project? Should it maybe be an MSBuild test in Xamarin.Android.Build.Tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. This should be removed before merging. It's mostly for the InTune team to see the item group to use and the filename currently supported.

I don't think we can put it into a test project either, because it doesn't work right now: the HelloWorld.DotNet.csproj approach causes the app to cash at startup because MAMActivity can't be found, which is fair because it doesn't exist in the app.

If we extract classes.jar from Microsoft.Intune.MAM.Xamarin.Android, a'la dotnet/java-interop#936 (comment), it still crashes at startup, because:

JNI DETECTED ERROR IN APPLICATION: can't call void com.microsoft.intune.mam.client.app.MAMActivity.onMAMCreate(android.os.Bundle) on instance of example.MainActivity

which is likewise "fair", because the example.MainActivity Java bytecode wasn't updated by the InTune tooling, because the tooling didn't run.

If we try to do this "more correctly":

    <PackageReference
        Include="Microsoft.Intune.MAM.Xamarin.Android"
        Version="3.0.4635.1"
    />

it doesn't build, because .NET 7 doesn't have Microsoft.Build.Utilities.v4.0.dll:

$HOME/.nuget/packages/xamarin.android.support.annotations/28.0.0.1/build/monoandroid90/Xamarin.Android.Support.Annotations.targets(17,3): error MSB4062: The "Xamarin.Android.Support.BuildTasks.VerifyVersionsTask" task could not be loaded from the assembly $HOME/.nuget/packages/xamarin.android.support.annotations/28.0.0.1/build/monoandroid90/Xamarin.Android.Support.BuildTasks.dll. Could not load file or assembly 'Microsoft.Build.Utilities.v4.0, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. The system cannot find the file specified.

Right now, I don't think we can have an "end-to-end using InTune" test.

We could -- should? -- have a "lesser" test, which uses the remap-mam-json-to-xml output format directly along with a small set of Java files, and this would be useful to test things, but it wouldn't include InTune.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beginnings of a "lesser" test just pushed, via introduction of a new @(_AndroidRemapMembers) build action. This uses the same XML format that the InTune JSON is converted to, so we can test things by writing our XML format directly, instead of going through a JSON intermediary.

TODO: add an actual on-device unit test which does this.

Comment on lines 313 to 314
Logger.Log (LogLevel.Warn, "*jonp*", $"# jonp: looking for replacement type for `{jniSimpleReference}`");
Logger.Log (LogLevel.Warn, "*jonp*", new System.Diagnostics.StackTrace (true).ToString ());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably still a lot of jonp print statements throughout.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the basics are settled, I'll remove them. :-)

if (args->mappingXml != IntPtr.Zero) {
var xml = Encoding.UTF8.GetString ((byte*) args->mappingXml, args->mappingXmlLen);
Logger.Log (LogLevel.Warn, "*jonp*", $"# jonp: mapping xml: len={args->mappingXmlLen}; {xml}");
(ReplacementTypes, ReplacementMethods) = MamXmlParser.ParseStrings (xml);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using XML here at all, could we generate a type in the main assembly that has this data? So you would just use reflection to look for a couple well-known class and field names?

Maybe that is more work than we're willing to do right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my other comment, but…. We don't want to "privilege" the "main" assembly ("what's a main assembly?"), because when we have, things break; see e.g. 807e665, wherein an app crashed because the Activity launched wasn't in the "main" assembly.

The "best" fix is to put this data into libxamarin-app.so. I think we can do that by e.g. .NET 7 P7.


namespace Android.Runtime {

class MamXmlParser {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we went with the C# code generation idea, maybe this class could write a .cs file instead of XML? We don't have any Roslyn source-generators yet, so that would probably be the simplest route.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but…

The question is where to put this data? A source generator means "put it into the main assembly", and obtaining that data from the main assembly from JNIEnv.Initialize() sounds like A Bad Time™.

Instead, the mid-term solution should be to instead put this data into libxamarin-app.so, as a build-time action.

@jonpryor jonpryor force-pushed the jonp-member-remapping branch 4 times, most recently from c7b661a to 387b0f8 Compare May 17, 2022 00:44
}

[Test]
public void TypeAndMemberRemapping ([Values (false, true)] bool isRelease)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dellis1972 , @jonathanpeppers : please review this test for sanity. :-)

(I have no idea what I'm doing here. [dog-on-keyboard.gif])

Comment on lines 203 to 192
var dotnet = new DotNetCLI (proj, Path.Combine (fullProjDir, proj.ProjectFilePath));

Assert.IsTrue (dotnet.Build (), "`dotnet build` should succeed");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other XASdkTests class has some helper methods:

https://github.com/xamarin/xamarin-android/blob/1efa0cf46c9079fc06669a4434f597faf47504af/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/XASdkTests.cs#L1071-L1094

Should we put these somewhere, so that XASdkDeployTests can use them?

Then you can do:

var builder = CreateDotNetBuilder (proj);
Assert.IsTrue (builder.Build (), $`dotnet build` should succeed");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test looks good. But making use of methods like CreateDotNetBuilder would make it a bit nicer :)


<Target Name="_AddAndroidCustomMetaData">
<Target Name="_AddAndroidCustomMetaData"
DependsOnTargets="_ConvertAndroidMamMappingFileToXml;_CollectAndroidRemapMembers">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think (and @jonathanpeppers can correct me :) ) we tend to prefer that we explicitly define the dependencies in
src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.BuildOrder.targets for .NET and src/Xamarin.Android.Build.Tasks/Xamarin.Android.Legacy.targets for Legacy. We can do this in addition to DependsOnTargets but it does help when figuring out the build order if we have it in the other files too.

This is for .NET 7+ only right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, this is .NET 7+ only.

@jonpryor jonpryor force-pushed the jonp-member-remapping branch 3 times, most recently from f3a09e7 to cf89575 Compare May 19, 2022 00:32
}

[Test]
public void TypeAndMemberRemapping ([Values (false, true)] bool isRelease)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test never runs, for reasons I cannot currently explain or fathom. It's in the downloaded MSBuildDeviceIntegration.dll. It's not [Values]; ApplicationRunsWithoutDebugger is run. It's not position in the file (moving it didn't change anything).

Meanwhile, I can run it locally, via the updated docs:

./dotnet-local.sh test bin/TestDebug/MSBuildDeviceIntegration/net6.0/MSBuildDeviceIntegration.dll --filter "Name~TypeAndMemberRemapping"

Fortunately, type and member remapping is also tested as part of Java.Interop-Tests.dll, included in Mono.Android.NET-Tests.apk, so I'm confident it works. It would just be nice if this test also worked. Or at least ran.

@jonpryor jonpryor force-pushed the jonp-member-remapping branch from cafcf0d to e811daf Compare May 19, 2022 13:15
Comment on lines +91 to +98
"System.Collections.dll",
"System.Collections.Concurrent.dll",
"System.Collections.NonGeneric.dll",
"System.Console.dll",
"System.IO.Compression.dll",
"System.Net.Http.dll",
"System.Net.Primitives.dll",
"System.Net.Requests.dll",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait was this failing on main? I just merged this one and it was completely green: #6986

So is something in this PR causing these to show up in apps now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes; probably the new XmlReader use in JNIEnv.Initialize().

Presumably these will "disappear" once we do the "non-prototype" approach of putting al this data into libxamarin-app.so

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using System.Xml bloats app size a lot... So now we are doing it for all apps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This week? Yes.

Once we get all this re-hosted on libxamarin-app.so, no.

jonpryor added 4 commits May 19, 2022 11:05
Context: dotnet/java-interop#867
Context: dotnet/java-interop#936

Does It Build?

Does It Work?

(Is It Sane?)

For local "test" purposes, add a new `tools/remap-mam-json-to-xml`
utility which parses the MAM JSON file into XML.

	$ dotnet run --project tools/remap-mam-json-to-xml  -- \
	  $HOME/.nuget/packages/microsoft.intune.mam.remapper.tasks/0.1.4635.1/content/MonoAndroid10/remapping-config.json
	<replacements>
	  <replace-type from="android/app/Activity" to="com/microsoft/intune/mam/client/app/MAMActivity" />
	  …
	  <replace-method source-type="com/microsoft/intune/mam/client/support/v7/app/MAMAppCompatActivity" source-method-name="onStateNotSaved" target-type="com/microsoft/intune/mam/client/support/v7/app/MAMAppCompatActivity" target-method-name="onMAMStateNotSaved" target-method-instance-to-static="false" />
	</replacements>

dotnet bin/Debug/net6.0/remap-mam-json-to-xml.dll $HOME/.nuget/packages/microsoft.intune.mam.remapper.tasks/0.1.4635.1/content/MonoAndroid10/remapping-config.json

Add a new `@(_AndroidMamMappingFile)` item group which can be
used to produce the mapping XML file.

TODO: why is `$(_AndroidMamMappingXml)` e.g.
`obj/Debug/assets/obj/Debug/assets/xa-mam-mapping.xml`?
What's with the duplication?

Update src/java-runtime to use `javac -h`, to emit JNI headers.
(`javah` is not present in JDK11!)

Update src/monodroid to depend on src/java-runtime, then use the
headers generated by `javac -h` to ensure consistency.
Rando aside: apparently `createNewContextWithData` was inconsistent?)
While chatting with @grendello about how to eventually get type &
member remapping integrated into `libxamarin-app.so` -- thus avoiding
the need to parse XML during app startup -- he asked if it would be
possible to use a `Dictionary<string, string>` as the primary in-app
data structure, as that is more easily supportable.

See also e.g. how typemap lookup is done, which conceptually involved
string-to-string mapping.

Update `JNIEnv` & `AndroidRuntime` to use "string-oriented" lookup,
using tabs (`\t`) to separate out the "sub-parts" of the original
tuple keys and values.
jonpryor added 7 commits May 19, 2022 11:05
To *begin* answering the "how do we *test* this?" question,
add a new `@(_AndroidRemapMembers)` build action.

`@(_AndroidMamMappingFile)` is JSON input which is converted into XML
by the `<MamJsonToXml/>` task.  This is the XML described in prior
commits, and in the current PR body text.

`@(_AndroidRemapMembers)` is XML input, of the same format.

This allows us to use our "internal XML" format in "external"
apps -- HelloWorld, future unit tests -- in order to demonstrate
that the remapping infrastructure *works*.

This means we can update HelloWorld (for now, unit tests later)
to have the XML file:

    <replacements>
      <replace-type from="android/app/Activity" to="android/app/ListActivity" />
    </replacements>

and update `HelloWorld.DotNet.csproj` to have:

    <_AndroidRemapMembers Include="remap.xml" />

At runtime, `adb logcat` contains:

    # jonp: found replacement type: `android/app/Activity` => `android/app/ListActivity`

*And*, the app *works*!  No crashes!  (Yay!)
Use `@(AndroidJavaSource)` to add a pair of Java types:

    public class RemapActivity extends Activity {
        public void onMyCreate (android.os.Bundle bundle);
    }
    class ViewHelper {
        public static void mySetOnClickListener (android.view.View view, android.view.View.OnClickListener listener);
    }

Update `remap.xml` to:

 1. Rename `android.app.Activity` to `example.RemapActivity`
 2. Rename "`Activity.onCreate()`" to `RemapActivity.onMyCreate()`
 3. "Patch" `View.setOnClickListener()` to instead call
    `ViewHelper.mySetOnClickListener()`.

"Learnings" from this exercise:

 1. Changing the Java hierarchy "requires" changing the managed
    hierarchy to mirror it.

    If we rename `Activity` to `RemapActivity` but *don't* change
    `MainActivity` to inherit the (bound!) `Example.RemapActivity`,
    the app *crashes*:

        JNI DETECTED ERROR IN APPLICATION: can't call void example.RemapActivity.onMyCreate(android.os.Bundle) on instance of example.MainActivity

    This can be "fixed" *without* changing the base class
    of `MainActivity` by instead changing the base class of the
    Java Callable Wrapper for `MainActivity` to `example.RemapActivity`.
    This can be done manually, but isn't really supported in "normal"
    xamarin-android usage.

    Presumably InTune would make this Just Work by e.g. patching the
    `MainActivity.class` file.

 2. Note the "scare quotes" in (2): you *don't* rename
    `Activity.onCreate()`, because after renaming `Activity` to
    `example.RemapActivity`, `Activity` *no longer exists*.
    This warps the brain a bit.
    You can rename methods, but the `/replace-method/@source-type`
    value needs to be the `/replace-type/@to` value.

    This:

        <replace-method
            source-type="example/RemapActivity"
            source-method-name="onCreate"
            target-type="example/RemapActivity"
            target-method-name="onMyCreate" target-method-instance-to-static="false" />

    not this:

        <replace-method
            source-type="android/app/Activity"
            source-method-name="onCreate"
            target-type="example/RemapActivity"
            target-method-name="onMyCreate" target-method-instance-to-static="false" />

    While it would be more intuitive to support the latter,
    I couldn't figure out how to make it work robustly
    and efficiently.

 3. Don't intermix type renames with
    `/replace-method]@target-method-instance-to-static='true']`.
    It *can* be done, but also warps the brain.

    The deal with `@target-method-instance-to-static` is that it
    it changes the target method signature -- unless explicitly
    provided in `/replace-method/@target-method-signature` --
    so that the "source declaring type" is a prefix.

    Thus given

        <replace-method
            source-type="android/view/View"
            source-method-name="setOnClickListener"
            target-type="example/ViewHelper"
            target-method-name="mySetOnClickListener" target-method-instance-to-static="true" />

    we'll look for `ViewHelper.mySetOnClickListener(View, View.OnClickListener)`.

    If we renamed `View` to `MyView`, we would instead look for
    `ViewHelper.mySetOnClickListener(MyView, View.OnClickListener)`
    (note changed parameter type).

    This almost certainly *won't* work right for many circumstances.
Otherwise update unit tests.
When `debug.mono.log` = `gref+`, the app could crash:

    signal 11 (SIGSEGV), code 2 (SEGV_ACCERR), fault addr 0x79c045dead

This was likely because a constant string was provided to
`OSBridge::_write_stack_trace()`, which tried to write into the
constant string, promptly blowing things up.

Workaround: don't use `gref+` logging when a GC occurs?

Better workaround: Don't Do That™. Don't write to const strings.
Move `TypeAndMemberRemapping()` *earlier* in the file.
`DotNetInstallAndRun()` appears in the **Tests** tab, but
`TypeAndMemberRemapping()` was not, which was very confusing.
On a lark, suspect that `TypeAndMemberRemapping()` isn't run
because it's after `DotNetDebug()`, which is `[Category("Debugger")]`.
Will moving the test fix things?

Fix Mono.Android.NET-Tests, via removing a debug message from
AndroidRuntime, and test fixes within external/Java.Interop.
@jonpryor jonpryor force-pushed the jonp-member-remapping branch from e811daf to 29a3568 Compare May 19, 2022 15:07
.gitmodules Outdated
url = https://github.com/xamarin/java.interop.git
branch = main
url = https://github.com/jonpryor/java.interop.git
branch = jonp-member-remapping
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it point to JI/main?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, when dotnet/java-interop#936 is merged, but not until then.

Please review dotnet/java-interop#936 ;-)

var slash = jniSimpleReference.LastIndexOf ('/');
var desugarType = slash <= 0
? "Desugar" + jniSimpleReference
: jniSimpleReference.Substring (0, slash+1) + "Desugar" + jniSimpleReference.Substring (slash+1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be faster and more memory conservative to do

var desugarType = slash <= 0
? new StringBuilder ("Desugar").Append (jniSimpleReference).ToString ()
: new StringBuilder (jniSimpleReference.Substring (0, slash+1)).Append ("Desugar").Append(jniSimpleReference.Substring (slash+1)).ToString ();

And potentially use Span<T>.Slice instead of .Substring

if (JNIEnv.ReplacementTypes == null) {
return null;
}
Logger.Log (LogLevel.Warn, "*jonp*", $"# jonp: looking for replacement type for `{jniSimpleReference}`; ReplacementTypes? {JNIEnv.ReplacementTypes != null}");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this go in?

Logger.Log (LogLevel.Warn, "*jonp*", $"# jonp: looking for replacement type for `{jniSimpleReference}`; ReplacementTypes? {JNIEnv.ReplacementTypes != null}");
// Logger.Log (LogLevel.Warn, "*jonp*", new System.Diagnostics.StackTrace (true).ToString ());
if (JNIEnv.ReplacementTypes.TryGetValue (jniSimpleReference, out var v)) {
Logger.Log (LogLevel.Warn, "*jonp*", $"# jonp: found replacement type: `{jniSimpleReference}` => `{v}`");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

if (JNIEnv.ReplacementMethods == null) {
return null;
}
Logger.Log (LogLevel.Warn, "*jonp*", $"# jonp: looking for replacement method for (\"{jniSourceType}\", \"{jniMethodName}\", \"{jniMethodSignature}\")");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

methodsStrings [key] = value;
}

return (types, methodsStrings);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be faster to use out parameters for the dictionaries, to avoid instantiating a tuple

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will be moot once this is all in libxamarin-app.so. :-)

}

public static (ReplacementTypesDict ReplacementTypes, ReplacementMethodsDictStructured ReplacementMethods) ParseStructured (string xml)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto wrt tuple

if (string.IsNullOrEmpty (from) || string.IsNullOrEmpty (to)) {
return;
}
replacementTypes [from] = to;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how important it is, but this form of assignment will silently overwrite the previous value, if any. Perhaps .Add (key, val) would be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And chance an exception if there are duplicates?

Something to consider in future revisions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it depends on whether it matters or not. If it's not an error to overwrite an earlier value, then this code's fine :)

jstring runtimeNativeLibDir, jobjectArray appDirs, jobject loader,
jobjectArray assembliesJava, jint apiLevel, jboolean isEmulator,
jobjectArray assembliesJava, jbyteArray mappingXml, jint mappingXmlLen,
jint apiLevel, jboolean isEmulator,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish we could somehow kill this monster of a method... So many arguments :)

init.mappingXml = env->GetByteArrayElements (mappingXml, nullptr);
init.mappingXmlLen = mappingXmlLen;
}
log_warn (LOG_DEFAULT, "# jonp: mappingXml? len=%i, xml=%p", init.mappingXmlLen, init.mappingXml);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto logging

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, i'll leave this in so that there is some runtime indicator that mapping is being used, but i'll move it into the above if block so it's only printed when used.

@jonpryor jonpryor marked this pull request as ready for review May 19, 2022 21:23
@jonpryor
Copy link
Contributor Author

jonpryor commented May 20, 2022

[Mono.Android] Type & Member Remapping Support

Fixes: https://github.com/xamarin/java.interop/issues/867

Context: https://github.com/xamarin/java.interop/commit/1f27ab552d03aeb74cdc6f8985fcffbfdb9a7ddf
Context: https://github.com/xamarin/xamarin-android/issues/6142#issuecomment-889435599
Context: https://github.com/xamarin/xamarin-android/issues/7020

Changes: https://github.com/xamarin/java.interop/compare/843f3c7817dc4bdae9ce69d04274f29fce574e09...1f27ab552d03aeb74cdc6f8985fcffbfdb9a7ddf

  * xamarin/java.interop@1f27ab55: [Java.Interop] Type & Member Remapping Support (#936)
  * xamarin/java.interop@02aa54e0: [Java.Interop.Tools.JavaCallableWrappers] marshal method decl types (#987)
  * xamarin/java.interop@e7bacc37: [ci] Update azure-pipelines.yaml to Pin .NET 6.0.202 (#986)
  * xamarin/java.interop@fb94d598: [Java.Interop.Tools.JavaCallableWrappers] Collect overriden methods (#985)
  * xamarin/java.interop@3fcce746: [Java.Interop.{Dynamic,Export}] Nullable Reference Type support (#980)

~~ The Scenarios ~~

Our Java binding infrastructure involves looking up types and methods
via [JNI][0], and assumes that types and methods won't "move" in an
unexpected manner.  Methods which move from a subclass to a
superclass works transparently.  Methods which are moved to an
entirely different class causes us problems.

Case in point: [desugaring][0], which can *move* Java types to places
that our bindings don't expect.  For example, [`Arrays.stream(T[])`][1]
may be moved into a `DesugarArrays` class, or a default interface
method `Example.m()` may be moved into an `Example$-CC` type.
Java.Interop has not supported such changes, resulting in throwing a
`Java.Lang.NoSuchMethodError` on Android versions where methods are
not where we expect.

Additionally, the [InTune Mobile Application Management][3] team
needs a expanded type and member lookup mechanism in order to
simplify how they maintain their product.  Currently they make things
work by rewriting IL, which can be brittle.


~~ Build actions ~~

To improve support for this, xamarin/java.interop#936 introduces
new `virtual` methods into `Java.Interop.JniRuntime.JniTypeManager`
which are called as part of type and member lookup, allowing
`AndroidTypeManager` to participate in the type and member resolution
process.

`AndroidTypeManager` in turn needs to know what types and members can
be remapped, and what they should be remapped to.  Some of these can
be algorithmic, such as pre-pending `Desugar` or appending `$-CC` for
the Desugar case.

The InTune use case involves a table, contained within the
[Microsoft.Intune.MAM.Remapper.Tasks NuGet package][4].

Update `src/Xamarin.Android.Build.Tasks` to add a new
`@(_AndroidRemapMembers)` Build action.  This build action is not
externally supported; it's to help test the feature.  Files with this
build action are XML files which control type and member remapping:

	<replacements>
	  <replace-type
	      from="android/app/Activity"
	      to="com/microsoft/intune/mam/client/app/MAMActivity" />
	  <replace-method
	      source-type="com/microsoft/intune/mam/client/app/MAMActivity"
	      source-method-name="onCreate" source-method-signature="(Landroid/os/Bundle;)V"
	      target-type="com/microsoft/intune/mam/client/app/MAMActivity"
	      target-method-name="onMAMCreate" target-method-instance-to-static="false" />
	</replacements>

`//replacements/replace-method` is structured with each attribute
corresponding to a member on the `JniRuntime.ReplacementMethodInfo`
structure, in xamarin/java.interop@1f27ab55.

  * `//replace-method/@source-type` is
    `JniRuntime.ReplacementMethodInfo.SourceJniType`
  * `//replace-method/@source-method-name` is
    `JniRuntime.ReplacementMethodInfo.SourceJniMethodName`
  * `//replace-method/@source-method-signature` is
    `JniRuntime.ReplacementMethodInfo.SourceJniMethodSignature`
  * `//replace-method/@target-type` is
    `JniRuntime.ReplacementMethodInfo.TargetJniType`
  * `//replace-method/@target-method-name` is
    `JniRuntime.ReplacementMethodInfo.TargetJniMethodName`
  * `//replace-method/@target-method-signature` is
    `JniRuntime.ReplacementMethodInfo.TargetJniMethodSignature`
    This attribute is optional.
  * `//replace-method/@target-method-parameter-count` is
    `JniRuntime.ReplacementMethodInfo.TargetJniMethodParameterCount`.
    This attribute is optional.
  * `//replace-method/@target-method-instance-to-static` is
    `JniRuntime.ReplacementMethodInfo.TargetJniMethodIsStatic`

`@source-type`, `@source-method-name`, and `@source-method-signature`
combined serve as a "key" for looking up the associated `@target-*`
information.

Update `src/Xamarin.Android.Build.Tasks` to add a new
`@(_AndroidMamMappingFile)` Build action.  This build action is not
externally supported; it's to help test the feature.  Files with this
build action are expected to be JSON documents which follow the
current conventions of `remapping-config.json`, within the
`Microsoft.Intune.MAM.Remapper.Tasks` NuGet package.  This build
action is not externally supported; this is currently for testing
purposes.  `@(_AndroidMamMappingFile)` files are processed at build
time into `@(_AndroidRemapMembers)` XML files.

During App builds, all `@(_AndroidRemapMembers)` files are merged
into an `@(AndrodAsset)` named `xa-internal/xa-mam-mapping.xml`.
This asset is opened and provided to `JNIEnv.Initialize()` as part of
native app startup.


~~ Putting it all together ~~

This will only work on .NET 7+.

App project has a `@(_AndroidRemapMembers)` file.  This item is
processed during App build, stored into the `.apk`, and read during
app startup on an Android device.

Given a Java binding such as:

	public partial class Activity {
	    static readonly JniPeerMembers _members = new XAPeerMembers ("android/app/Activity", typeof (Activity));
	}

when the `JniPeerMembers` constructor runs, it will call
`JniEnvironment.Runtime.TypeManager.GetReplacementType("android/app/Activity")`.
If `@(_AndroidRemapMembers)` is based on the InTune
`remapping-config.json` file, then `android/app/Activity` is mapped
to `com/microsoft/intune/mam/client/app/MAMActivity`, and
`JNIEnv::FindClass()` will be told to lookup `MAMActivity`, *not*
`Activity`.

*If `MAMActivity` can't be found*, e.g. you're testing this all out,
the app will ~immediately crash, as `MAMActivity` doesn't exist. 😅

If `MAMActivity` can be found, eventually `Activity.OnCreate()` will
need to be invoked:

	partial class Activity {
	    protected virtual unsafe void OnCreate (Android.OS.Bundle? savedInstanceState)
	    {
	        const string __id = "onCreate.(Landroid/os/Bundle;)V";
	        try {
	            JniArgumentValue* __args = stackalloc JniArgumentValue [1];
	            __args [0] = new JniArgumentValue ((savedInstanceState == null) ? IntPtr.Zero : ((global::Java.Lang.Object) savedInstanceState).Handle);
	            _members.InstanceMethods.InvokeVirtualVoidMethod (__id, this, __args);
	        } finally {
	            global::System.GC.KeepAlive (savedInstanceState);
	        }
	    }
	}

`_members.InstanceMethods.InvokeVirtualVoidMethod()` will internally
make a call similar to:

	var r = JniEnvironment.Runtime.TypeManager.GetReplacementMethodInfo (
	    "com/microsoft/intune/mam/client/app/MAMActivity",
	    "onCreate",
	    "(Landroid/os/Bundle;)V"
	);

The data returned will be equivalent to:

	var r = new JniRuntime.ReplacementMethodInfo {
	    SourceJniType                   = "com/microsoft/intune/mam/client/app/MAMActivity",    // from input parameter
	    SourceJniMethodName             = "onCreate",                                           // from input parameter
	    SourceJniMethodSignature        = "(Landroid/os/Bundle;)V",                             // from input parameter

	    TargetJniType                   = "com/microsoft/intune/mam/client/app/MAMActivity",    // from //replace-method/@target-type
	    TargetJniMethodName             = "onMAMCreate",                                        // from //replace-method/@target-method-name
	    TargetJniMethodSignature        = "(Landroid/os/Bundle;)V",                             // from input parameter, as signature didn't change
	    TargetJniMethodParameterCount   = 1,                                                    // computed based on signature
	    TargetJniMethodIsStatic         = false,                                                // from //replace-method/@target-method-instance-to-static
	}

This will allow `_members.InstanceMethods.InvokeVirtualVoidMethod()`
to instead resolve and invoke `MAMActivity.onMAMCreate()`.


~~ Tools ~~

`tools/remap-mam-json-to-xml` is added, and will process the InTune
JSON file into `@(_AndroidRemapMembers)` XML:

	$ dotnet run --project tools/remap-mam-json-to-xml  -- \
	  $HOME/.nuget/packages/microsoft.intune.mam.remapper.tasks/0.1.4635.1/content/MonoAndroid10/remapping-config.json
	<replacements>…


~~ Unit Tests ~~

`@(_AndroidRemapMembers)` usage is required by
`Mono.Android.NET-Tests.apk`, as `Java.Interop-Tests.dll` exercises
the type and member remapping logic.


~~ Unrelated `gref+` logging fixes ~~

When `debug.mono.log` = `gref+`, the app could crash:

	signal 11 (SIGSEGV), code 2 (SEGV_ACCERR), fault addr 0x79c045dead

This was likely because a constant string was provided to
`OSBridge::_write_stack_trace()`, which tried to write into the
constant string, promptly blowing things up.

Workaround: don't use `gref+` logging when a GC occurs?
(Fortunately, `gref+` logging isn't the default.)

Better workaround: Don't Do That™. Don't write to const strings.


~~ About `@(_AndroidRemapMembers)` Semantics… ~~

 1. Changing the Java hierarchy "requires" changing the managed
    hierarchy to mirror it.

    If we rename `Activity` to `RemapActivity` but *don't* change
    `MainActivity` to inherit the (bound!) `Example.RemapActivity`,
    the app *crashes*:

        JNI DETECTED ERROR IN APPLICATION: can't call void example.RemapActivity.onMyCreate(android.os.Bundle) on instance of example.MainActivity

    This can be "fixed" *without* changing the base class
    of `MainActivity` by instead changing the base class of the
    Java Callable Wrapper for `MainActivity` to `example.RemapActivity`.
    This can be done manually (just edit the files in `obj/…`!), but
    isn't really supported in "normal" xamarin-android usage
    (the next Clean will wipe your changes).

    Presumably InTune would make this Just Work by e.g. patching the
    `MainActivity.class` file.

 2. `/replacements/replace-type` interacts with
    `/replacements/replace-method`: at runtime, `//replace-type@from`
    *no longer exists*, meaning you ***cannot*** use that name
    in `//replace-method/@source-type` either!

    If `Activity` is remapped to `RemapActivity`, then *there is no*
    `Activity.onCreate()` method to similarly remap.  Instead, you
    need to specify `RemapActivity.onCreate()`.

    This warps the brain a bit.

    This:

        <replace-method
            source-type="example/RemapActivity"
            source-method-name="onCreate"
            target-type="example/RemapActivity"
            target-method-name="onMyCreate" target-method-instance-to-static="false" />

    not this:

        <replace-method
            source-type="android/app/Activity"
            source-method-name="onCreate"
            target-type="example/RemapActivity"
            target-method-name="onMyCreate" target-method-instance-to-static="false" />

 3. Don't intermix type renames with
    `/replace-method/@target-method-instance-to-static='true']`.
    It *can* be done, but also warps the brain.

    The deal with `@target-method-instance-to-static` is that it
    it changes the target method signature -- unless explicitly
    provided in `/replace-method/@target-method-signature` --
    so that the "source declaring type" is a prefix.

    Thus given

        <replace-method
            source-type="android/view/View"
            source-method-name="setOnClickListener"
            target-type="example/ViewHelper"
            target-method-name="mySetOnClickListener" target-method-instance-to-static="true" />

    we'll look for `ViewHelper.mySetOnClickListener(View, View.OnClickListener)`.

    If we renamed `View` to `MyView`, we would instead look for
    `ViewHelper.mySetOnClickListener(MyView, View.OnClickListener)`
    (note changed parameter type).

    This almost certainly *won't* work.


~~ InTune Integration Testing? ~~

For "more complete" InTune integration testing, one will want the
path to `remapping-config.json`, without hardcoding things.
This can be done with `%(PackageReference.GeneratePathProperty)`=True
and using `$(PkgMicrosoft_Intune_MAM_Remapper_Tasks)`:

	<ItemGroup>
	  <PackageReference
	      Include="Microsoft.Intune.MAM.Remapper.Tasks"
	      Version="0.1.4635.1"
	      IncludeAssets="none"
	      GeneratePathProperty="True"
	      ReferenceOutputAssembly="False"
	  />
	</ItemGroup>
	<Target Name="_AddMamFiles"
	    BeforeTargets="_AddAndroidCustomMetaData">
	  <ItemGroup>
	    <_AndroidMamMappingFile Include="$(PkgMicrosoft_Intune_MAM_Remapper_Tasks)/content/MonoAndroid10/remapping-config.json" />
	  </ItemGroup>
	</Target>

This is still fraught with some peril, as it likely also depends on
getting the right "inner" build, which may require using the plural
`$(TargetFrameworks)` property, not the singular `$(TargetFramework)`.
This might still be a useful start.


~~ TODO ~~

Optimize this mess:
https://github.com/xamarin/xamarin-android/issues/7020

[0]: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html
[1]: https://developer.android.com/studio/write/java8-support#library-desugaring
[2]: https://developer.android.com/reference/java/util/Arrays#stream(T[])
[3]: https://docs.microsoft.com/en-us/mem/intune/fundamentals/what-is-intune
[4]: https://www.nuget.org/packages/Microsoft.Intune.MAM.Remapper.Tasks/

@jonpryor jonpryor merged commit f6f11a5 into dotnet:main May 20, 2022
simonrozsival pushed a commit to simonrozsival/xamarin-android that referenced this pull request May 31, 2022
Fixes: dotnet/java-interop#867

Context: dotnet/java-interop@1f27ab5
Context: dotnet#6142 (comment)
Context: dotnet#7020

Changes: dotnet/java-interop@843f3c7...1f27ab5

  * dotnet/java-interop@1f27ab55: [Java.Interop] Type & Member Remapping Support (dotnet#936)
  * dotnet/java-interop@02aa54e0: [Java.Interop.Tools.JavaCallableWrappers] marshal method decl types (dotnet#987)
  * dotnet/java-interop@e7bacc37: [ci] Update azure-pipelines.yaml to Pin .NET 6.0.202 (dotnet#986)
  * dotnet/java-interop@fb94d598: [Java.Interop.Tools.JavaCallableWrappers] Collect overriden methods (dotnet#985)
  * dotnet/java-interop@3fcce746: [Java.Interop.{Dynamic,Export}] Nullable Reference Type support (dotnet#980)

~~ The Scenarios ~~

Our Java binding infrastructure involves looking up types and methods
via [JNI][0], and assumes that types and methods won't "move" in an
unexpected manner.  Methods which move from a subclass to a
superclass works transparently.  Methods which are moved to an
entirely different class causes us problems.

Case in point: [desugaring][0], which can *move* Java types to places
that our bindings don't expect.  For example, [`Arrays.stream(T[])`][1]
may be moved into a `DesugarArrays` class, or a default interface
method `Example.m()` may be moved into an `Example$-CC` type.
Java.Interop has not supported such changes, resulting in throwing a
`Java.Lang.NoSuchMethodError` on Android versions where methods are
not where we expect.

Additionally, the [InTune Mobile Application Management][3] team
needs a expanded type and member lookup mechanism in order to
simplify how they maintain their product.  Currently they make things
work by rewriting IL, which can be brittle.


~~ Build actions ~~

To improve support for this, dotnet/java-interop#936 introduces
new `virtual` methods into `Java.Interop.JniRuntime.JniTypeManager`
which are called as part of type and member lookup, allowing
`AndroidTypeManager` to participate in the type and member resolution
process.

`AndroidTypeManager` in turn needs to know what types and members can
be remapped, and what they should be remapped to.  Some of these can
be algorithmic, such as pre-pending `Desugar` or appending `$-CC` for
the Desugar case.

The InTune use case involves a table, contained within the
[Microsoft.Intune.MAM.Remapper.Tasks NuGet package][4].

Update `src/Xamarin.Android.Build.Tasks` to add a new
`@(_AndroidRemapMembers)` Build action.  This build action is not
externally supported; it's to help test the feature.  Files with this
build action are XML files which control type and member remapping:

	<replacements>
	  <replace-type
	      from="android/app/Activity"
	      to="com/microsoft/intune/mam/client/app/MAMActivity" />
	  <replace-method
	      source-type="com/microsoft/intune/mam/client/app/MAMActivity"
	      source-method-name="onCreate" source-method-signature="(Landroid/os/Bundle;)V"
	      target-type="com/microsoft/intune/mam/client/app/MAMActivity"
	      target-method-name="onMAMCreate" target-method-instance-to-static="false" />
	</replacements>

`//replacements/replace-method` is structured with each attribute
corresponding to a member on the `JniRuntime.ReplacementMethodInfo`
structure, in dotnet/java-interop@1f27ab55.

  * `//replace-method/@source-type` is
    `JniRuntime.ReplacementMethodInfo.SourceJniType`
  * `//replace-method/@source-method-name` is
    `JniRuntime.ReplacementMethodInfo.SourceJniMethodName`
  * `//replace-method/@source-method-signature` is
    `JniRuntime.ReplacementMethodInfo.SourceJniMethodSignature`
  * `//replace-method/@target-type` is
    `JniRuntime.ReplacementMethodInfo.TargetJniType`
  * `//replace-method/@target-method-name` is
    `JniRuntime.ReplacementMethodInfo.TargetJniMethodName`
  * `//replace-method/@target-method-signature` is
    `JniRuntime.ReplacementMethodInfo.TargetJniMethodSignature`
    This attribute is optional.
  * `//replace-method/@target-method-parameter-count` is
    `JniRuntime.ReplacementMethodInfo.TargetJniMethodParameterCount`.
    This attribute is optional.
  * `//replace-method/@target-method-instance-to-static` is
    `JniRuntime.ReplacementMethodInfo.TargetJniMethodIsStatic`

`@source-type`, `@source-method-name`, and `@source-method-signature`
combined serve as a "key" for looking up the associated `@target-*`
information.

Update `src/Xamarin.Android.Build.Tasks` to add a new
`@(_AndroidMamMappingFile)` Build action.  This build action is not
externally supported; it's to help test the feature.  Files with this
build action are expected to be JSON documents which follow the
current conventions of `remapping-config.json`, within the
`Microsoft.Intune.MAM.Remapper.Tasks` NuGet package.  This build
action is not externally supported; this is currently for testing
purposes.  `@(_AndroidMamMappingFile)` files are processed at build
time into `@(_AndroidRemapMembers)` XML files.

During App builds, all `@(_AndroidRemapMembers)` files are merged
into an `@(AndrodAsset)` named `xa-internal/xa-mam-mapping.xml`.
This asset is opened and provided to `JNIEnv.Initialize()` as part of
native app startup.


~~ Putting it all together ~~

This will only work on .NET 7+.

App project has a `@(_AndroidRemapMembers)` file.  This item is
processed during App build, stored into the `.apk`, and read during
app startup on an Android device.

Given a Java binding such as:

	public partial class Activity {
	    static readonly JniPeerMembers _members = new XAPeerMembers ("android/app/Activity", typeof (Activity));
	}

when the `JniPeerMembers` constructor runs, it will call
`JniEnvironment.Runtime.TypeManager.GetReplacementType("android/app/Activity")`.
If `@(_AndroidRemapMembers)` is based on the InTune
`remapping-config.json` file, then `android/app/Activity` is mapped
to `com/microsoft/intune/mam/client/app/MAMActivity`, and
`JNIEnv::FindClass()` will be told to lookup `MAMActivity`, *not*
`Activity`.

*If `MAMActivity` can't be found*, e.g. you're testing this all out,
the app will ~immediately crash, as `MAMActivity` doesn't exist. 😅

If `MAMActivity` can be found, eventually `Activity.OnCreate()` will
need to be invoked:

	partial class Activity {
	    protected virtual unsafe void OnCreate (Android.OS.Bundle? savedInstanceState)
	    {
	        const string __id = "onCreate.(Landroid/os/Bundle;)V";
	        try {
	            JniArgumentValue* __args = stackalloc JniArgumentValue [1];
	            __args [0] = new JniArgumentValue ((savedInstanceState == null) ? IntPtr.Zero : ((global::Java.Lang.Object) savedInstanceState).Handle);
	            _members.InstanceMethods.InvokeVirtualVoidMethod (__id, this, __args);
	        } finally {
	            global::System.GC.KeepAlive (savedInstanceState);
	        }
	    }
	}

`_members.InstanceMethods.InvokeVirtualVoidMethod()` will internally
make a call similar to:

	var r = JniEnvironment.Runtime.TypeManager.GetReplacementMethodInfo (
	    "com/microsoft/intune/mam/client/app/MAMActivity",
	    "onCreate",
	    "(Landroid/os/Bundle;)V"
	);

The data returned will be equivalent to:

	var r = new JniRuntime.ReplacementMethodInfo {
	    SourceJniType                   = "com/microsoft/intune/mam/client/app/MAMActivity",    // from input parameter
	    SourceJniMethodName             = "onCreate",                                           // from input parameter
	    SourceJniMethodSignature        = "(Landroid/os/Bundle;)V",                             // from input parameter

	    TargetJniType                   = "com/microsoft/intune/mam/client/app/MAMActivity",    // from //replace-method/@target-type
	    TargetJniMethodName             = "onMAMCreate",                                        // from //replace-method/@target-method-name
	    TargetJniMethodSignature        = "(Landroid/os/Bundle;)V",                             // from input parameter, as signature didn't change
	    TargetJniMethodParameterCount   = 1,                                                    // computed based on signature
	    TargetJniMethodIsStatic         = false,                                                // from //replace-method/@target-method-instance-to-static
	}

This will allow `_members.InstanceMethods.InvokeVirtualVoidMethod()`
to instead resolve and invoke `MAMActivity.onMAMCreate()`.


~~ Tools ~~

`tools/remap-mam-json-to-xml` is added, and will process the InTune
JSON file into `@(_AndroidRemapMembers)` XML:

	$ dotnet run --project tools/remap-mam-json-to-xml  -- \
	  $HOME/.nuget/packages/microsoft.intune.mam.remapper.tasks/0.1.4635.1/content/MonoAndroid10/remapping-config.json
	<replacements>…


~~ Unit Tests ~~

`@(_AndroidRemapMembers)` usage is required by
`Mono.Android.NET-Tests.apk`, as `Java.Interop-Tests.dll` exercises
the type and member remapping logic.


~~ Unrelated `gref+` logging fixes ~~

When `debug.mono.log` = `gref+`, the app could crash:

	signal 11 (SIGSEGV), code 2 (SEGV_ACCERR), fault addr 0x79c045dead

This was likely because a constant string was provided to
`OSBridge::_write_stack_trace()`, which tried to write into the
constant string, promptly blowing things up.

Workaround: don't use `gref+` logging when a GC occurs?
(Fortunately, `gref+` logging isn't the default.)

Better workaround: Don't Do That™. Don't write to const strings.


~~ About `@(_AndroidRemapMembers)` Semantics… ~~

 1. Changing the Java hierarchy "requires" changing the managed
    hierarchy to mirror it.

    If we rename `Activity` to `RemapActivity` but *don't* change
    `MainActivity` to inherit the (bound!) `Example.RemapActivity`,
    the app *crashes*:

        JNI DETECTED ERROR IN APPLICATION: can't call void example.RemapActivity.onMyCreate(android.os.Bundle) on instance of example.MainActivity

    This can be "fixed" *without* changing the base class
    of `MainActivity` by instead changing the base class of the
    Java Callable Wrapper for `MainActivity` to `example.RemapActivity`.
    This can be done manually (just edit the files in `obj/…`!), but
    isn't really supported in "normal" xamarin-android usage
    (the next Clean will wipe your changes).

    Presumably InTune would make this Just Work by e.g. patching the
    `MainActivity.class` file.

 2. `/replacements/replace-type` interacts with
    `/replacements/replace-method`: at runtime, `//replace-type@from`
    *no longer exists*, meaning you ***cannot*** use that name
    in `//replace-method/@source-type` either!

    If `Activity` is remapped to `RemapActivity`, then *there is no*
    `Activity.onCreate()` method to similarly remap.  Instead, you
    need to specify `RemapActivity.onCreate()`.

    This warps the brain a bit.

    This:

        <replace-method
            source-type="example/RemapActivity"
            source-method-name="onCreate"
            target-type="example/RemapActivity"
            target-method-name="onMyCreate" target-method-instance-to-static="false" />

    not this:

        <replace-method
            source-type="android/app/Activity"
            source-method-name="onCreate"
            target-type="example/RemapActivity"
            target-method-name="onMyCreate" target-method-instance-to-static="false" />

 3. Don't intermix type renames with
    `/replace-method/@target-method-instance-to-static='true']`.
    It *can* be done, but also warps the brain.

    The deal with `@target-method-instance-to-static` is that it
    it changes the target method signature -- unless explicitly
    provided in `/replace-method/@target-method-signature` --
    so that the "source declaring type" is a prefix.

    Thus given

        <replace-method
            source-type="android/view/View"
            source-method-name="setOnClickListener"
            target-type="example/ViewHelper"
            target-method-name="mySetOnClickListener" target-method-instance-to-static="true" />

    we'll look for `ViewHelper.mySetOnClickListener(View, View.OnClickListener)`.

    If we renamed `View` to `MyView`, we would instead look for
    `ViewHelper.mySetOnClickListener(MyView, View.OnClickListener)`
    (note changed parameter type).

    This almost certainly *won't* work.


~~ InTune Integration Testing? ~~

For "more complete" InTune integration testing, one will want the
path to `remapping-config.json`, without hardcoding things.
This can be done with `%(PackageReference.GeneratePathProperty)`=True
and using `$(PkgMicrosoft_Intune_MAM_Remapper_Tasks)`:

	<ItemGroup>
	  <PackageReference
	      Include="Microsoft.Intune.MAM.Remapper.Tasks"
	      Version="0.1.4635.1"
	      IncludeAssets="none"
	      GeneratePathProperty="True"
	      ReferenceOutputAssembly="False"
	  />
	</ItemGroup>
	<Target Name="_AddMamFiles"
	    BeforeTargets="_AddAndroidCustomMetaData">
	  <ItemGroup>
	    <_AndroidMamMappingFile Include="$(PkgMicrosoft_Intune_MAM_Remapper_Tasks)/content/MonoAndroid10/remapping-config.json" />
	  </ItemGroup>
	</Target>

This is still fraught with some peril, as it likely also depends on
getting the right "inner" build, which may require using the plural
`$(TargetFrameworks)` property, not the singular `$(TargetFramework)`.
This might still be a useful start.


~~ TODO ~~

Optimize this mess:
dotnet#7020

[0]: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html
[1]: https://developer.android.com/studio/write/java8-support#library-desugaring
[2]: https://developer.android.com/reference/java/util/Arrays#stream(T[])
[3]: https://docs.microsoft.com/en-us/mem/intune/fundamentals/what-is-intune
[4]: https://www.nuget.org/packages/Microsoft.Intune.MAM.Remapper.Tasks/
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2024
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 this pull request may close these issues.

Support Type & member remapping

4 participants