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

RuntimeInstanceNotAllowed Error Initializing XmlSerializer on Android Release Build in .NET 9 #109724

Open
david-maw opened this issue Nov 12, 2024 · 24 comments
Assignees

Comments

@david-maw
Copy link

Description

App fails with an exception in a release build of .NET 9 (.NET 8 and debug builds of .NET 9 work fine).

It looks like the fault happened because of a call on new XmlSerializer(typeof(Meal)) (deferring that call defers the fault).

The abbreviated stack trace looks like this:

System.ArgumentException: RuntimeInstanceNotAllowed
  ?, in object DefaultValueAttribute.get_Value()
  ?, in new XmlAttributes(ICustomAttributeProvider)
  ?, in XmlAttributes XmlReflectionImporter.GetAttributes(MemberInfo)
  ?, in bool XmlReflectionImporter.InitializeStructMembers(StructMapping, StructModel, bool, string, RecursionLimiter)
  ?, in StructMapping XmlReflectionImporter.ImportStructLikeMapping(StructModel, string, bool, XmlAttributes, RecursionLimiter)
...
(1 additional frame(s) were not displayed)

System.InvalidOperationException: XmlTypeReflectionError, DivisiBill.Models.Meal
  ?, in XmlSerializer Meal.get_MealSerializer()
  ?, in void Meal.SaveToStream(Stream streamParameter)
  ?, in void Meal.SaveToApp()
  ?, in void Meal.OverwriteCurrent()
  ?, in async Task Meal.BecomeCurrentMealAsync()
...
(12 additional frame(s) were not displayed)

Steps to Reproduce

  1. Clone the repository, pull the class-constructor-fault branch.
  2. Start VS2022 and select the DivisiBill project and a release build.
  3. At the VS developer PowerShell prompt enter dotnet restore
  4. Run the app without debugging on an Android Emulator or physical device (ctrl+F5). You should see:Image
    5.ThenImage
    6.Press the left arrow to see:Image
  5. Click "While Using the App" and see:Image
  6. At this point the app will fault.

If it were working correctlyyou would see:Image
ThenImage

Link to public reproduction project repository

https://github.com/david-maw/DivisiBill.git

Version with bug

9.0.0-rc.2.24503.2

Is this a regression from previous behavior?

Yes, this used to work in .NET MAUI

Last version that worked well

8.0.93 SR9.3

Affected platforms

Android

Affected platform versions

Android 14

Did you find any workaround?

No response

Relevant log output

@drasticactions
Copy link

I don't believe your issue is related to the MAUI UI framework, it's a runtime one:

https://github.com/dotnet/runtime/blob/release/8.0/src/libraries/System.Private.CoreLib/src/System/ComponentModel/DefaultValueAttribute.cs

https://github.com/dotnet/runtime/blob/release/9.0/src/libraries/System.Private.CoreLib/src/System/ComponentModel/DefaultValueAttribute.cs#L246-L257

Between 8.0 and 9.0, a switch was added (#100416 and #105766) to throw on these values. Most likely what's tripping it up on Release mode is trimming (which would explain it working in Debug mode, and not Release.) I'm not sure how to specifically handle trimming with XmlSerializer, @jonathanpeppers would you know of what to do here?

@vitek-karas vitek-karas transferred this issue from dotnet/maui Nov 12, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Nov 12, 2024
@vitek-karas
Copy link
Member

@LakshanF @sbomer - this is due to the addition of System.ComponentModel.DefaultValueAttribute.IsSupported feature switch and it being disable by default when trimming is enabled. I don't remember what's the story around this (I know that it was difficult to make it work with trimming/AOT), and if there are work arounds/solutions for the user.

@sbomer
Copy link
Member

sbomer commented Nov 13, 2024

Minimal repro:

using System.Xml.Serialization;
using System.ComponentModel;

var s = new XmlSerializer(typeof(C));

_ = new C().T; // Prevent C.T from being removed by trimming

public class C {
    [DefaultValue(typeof(C), "c")]
    public Type T { get; }
}
<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net9.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <PublishTrimmed>true</PublishTrimmed>
  </PropertyGroup>
</Project>

This is currently by design - the ctor of DefaultValueAttribute that takes a type and a string is not trim compatible. We don't have a model for annotating attributes (annotating this constructor with RequiresUnreferencedCode would warn on every attribute instance, not the places that reflect over the attributed members), so we introduced the throwing behavior when trimming.

Note that in this example the use of XmlSerializer produces trim warnings.

It's possible to disable the throwing behavior by setting <_DefaultValueAttributeSupport>true</_DefaultValueAttributeSupport>, but note that this is not a supported configuration.

Most likely what's tripping it up on Release mode is trimming (which would explain it working in Debug mode, and not Release.)

Is it correct that the app was working in Debug mode? These settings should result in the same behavior whether you're in Debug or Release mode, so that might be a bug.

@david-maw
Copy link
Author

Yes, the app works in Debug mode but I thought the code would not be trimmed in a debug build.

I'm not aware of any trim warnings in the repro I provided, and I typically treat warnings as errors, so while it is possible I missed one it seems unlikely.

On the face of it, if DefaultValue and PublishTrimmed are incompatible it seems like something stronger than a warning is needed, since the result is code that works in .NET 8 but not .NET 9. That said,

I think what you've written means that to operate in a supported configuration I must eliminate uses of DefaultValue or set PublishTrimmed false in the build, is that correct?

@sbomer
Copy link
Member

sbomer commented Nov 14, 2024

@jonathanpeppers I'm aware that we made some changes to the trimming defaults for android recently - are both Debug and Release builds getting the defaults set by the illink targets?

As far as I understand, android doesn't show trim warnings unless using <TrimMode>full</TrimMode>. I think if you set that you should see the warnings.

I think what you've written means that to operate in a supported configuration I must eliminate uses of DefaultValue or set PublishTrimmed false in the build, is that correct?

Instances of DefaultValueAttribute won't cause problems unless something looks for them - so to be supported you would need to eliminate uses of code that depends on them (like XmlSerializer which is not trim-compatible).

@david-maw
Copy link
Author

Given that I need to serialize and deserialize XML and tell the XML serializer not to serialize some properties if they have their default value, it seems like my only option is to use XmlSerializer and DefaultValue and turn off trimming.

Does that seem like it would work (albeit at the cost of a larger compiled app) and is there some other method that I'm missing to achieve the same XML serialization behavior?

I checked and <TrimMode>full</TrimMode> does indeed trigger a warning when DefaultValue is used, but the failure happens even without setting it, so a more aggressive warning is probably needed so as to avoid failures with no warning.

@sbomer
Copy link
Member

sbomer commented Nov 14, 2024

Does that seem like it would work (albeit at the cost of a larger compiled app) and is there some other method that I'm missing to achieve the same XML serialization behavior?

That may work (I'm not sure how well supported it is to disable trimming entirely in an android app) - but it sounds like you are mainly interested in compat with the .NET 8 behavior, so I would recommend setting _DefaultValueAttributeSupport. We are considering taking a fix for .NET 9 that would preserve this behavior with <TrimMode>partial</TrimMod>.

@david-maw
Copy link
Author

I'm mainly interested in the ability to omit default valued items from XML serialization, I'd prefer it be done as it was in .NET 8 just because that's less work for me, but if there's a preferred method, I can probably convert to using that.

I'd shied away from DefaultValueAttributeSupport because of the earlier comment that it was not a supported configuration, but it seems to work on my code, so thank you.

That means I have a workaround for my instance of the problem but there remains a situation where code that worked fine on NET 8 receives no warnings and appears to work fine on NET 9, right up until you try an Android release build, then it fails. I'd suggest at the very least a warning, so users who may fall afoul of this get a hint that all may not be well.

david-maw added a commit to david-maw/DivisiBill that referenced this issue Nov 15, 2024
So as to detour a problem with XmlSerializr (see dotnet/runtime#109724)
@vitek-karas
Copy link
Member

We are definitely planning to fix the apparent breaking change between 8 and 9 - thanks a lot for spotting this and filing an issue!

Note that the partial trim mode which is the default for MAUI apps doesn't make hard guarantees about trim compatibility. It tends to work in most cases, but there's definitely ways where you can get it to break. That has been like that since a long time ago (goes back to Xamarin).

The full trim mode on the other hand does make guarantees, but it also produces lot of trim related warnings. If there are warnings, the app may break. If there are no warnings it will work (if not, it's a bug). In .NET 9 we've made lot of improvements to MAUI to make at least the core of MAUI trim compatible, so that a simple app in MAUI doesn't produce any warnings and thus can work with full trimming. Larger apps tend to depend on lot of additional components all of which need to be compatible with trimming for this to work - getting the ecosystem adapted to trimming is a long journey though.

@vitek-karas
Copy link
Member

@matouskozak could you please look into preparing a fix for the breaking change?

In short - we should set DefaultValueAttributeSupport to true for mobile targets if in partial trim mode. Similar to how we enable reflection based JSON serialization in that case. We should also check what happens in WASM (I suspect it's going to be a similar problem to mobile).

@david-maw
Copy link
Author

You are welcome @vitek-karas, and thanks for the explanation, which clarifies things for me, While I was happy to have a workaround that was simple to apply (thanks again @sbomer), a supported fix is preferable.

@vitek-karas
Copy link
Member

Well, the fix is very likely going to be basically the workaround, just done for you... but I agree it should be done.

@david-maw
Copy link
Author

I'd pretty much expected that, the key is, it'll be a supported configuration, so thanks.

david-maw added a commit to david-maw/DivisiBill that referenced this issue Nov 18, 2024
So as to detour a problem with XmlSerializr (see dotnet/runtime#109724)
@matouskozak
Copy link
Member

@matouskozak could you please look into preparing a fix for the breaking change?

In short - we should set DefaultValueAttributeSupport to true for mobile targets if in partial trim mode. Similar to how we enable reflection based JSON serialization in that case. We should also check what happens in WASM (I suspect it's going to be a similar problem to mobile).

Putting an update on the fix:

  • Android: Enable DefaultValueAttributeSupport in partial trim mode. android#9525
  • WASM: unknown (haven't tried yet)
  • iOS: Issue doesn't reproduce. I've been looking at the usage of ILLink by xamarin/macios and interestingly
    the System.ComponentModel.DefaultValueAttribute.IsSupported is not getting passed as a feature setting at all (on Android it is passed with value false) and passing it forcefully doesn't change the behavior. The difference is that iOS is passing System.AggressiveAttributeTrimming feature settings and I wonder if that could be overwriting the behavior.

@sbomer
Copy link
Member

sbomer commented Nov 19, 2024

System.ComponentModel.DefaultValueAttribute.IsSupported is not getting passed as a feature setting at all (on Android it is passed with value false) and passing it forcefully doesn't change the behavior.

That surprises me - we should make sure to understand this. I don't think AggressiveAttributeTrimming would be making the difference based on a quick look, since DefaultValueAttribute isn't one of the trimmed attributes under https://github.com/dotnet/runtime/blob/e3b3aaaf21b88b7992cd8a42321e01cf1aa704a2/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.Shared.xml#L89C68-L90.

jonathanpeppers added a commit to dotnet/android that referenced this issue Nov 20, 2024
…=partial` (#9525)

Context: dotnet/runtime#109724

In .NET 9, certain apps could crash with:

    System.ArgumentException: RuntimeInstanceNotAllowed
      ?, in object DefaultValueAttribute.get_Value()
      ?, in new XmlAttributes(ICustomAttributeProvider)
      ?, in XmlAttributes XmlReflectionImporter.GetAttributes(MemberInfo)
      ?, in bool XmlReflectionImporter.InitializeStructMembers(StructMapping, StructModel, bool, string, RecursionLimiter)
      ?, in StructMapping XmlReflectionImporter.ImportStructLikeMapping(StructModel, string, bool, XmlAttributes, RecursionLimiter)

.NET's concept of `$(PublishTrimmed)` is used to decide which trimmer
feature switches are toggled. This is normally set for both Debug &
Release, but Android only sets it for Release. This means that the
`$(_DefaultValueAttributeSupport)` feature switch is not set properly
in some cases, which causes the crash.

For now, we can set `$(_DefaultValueAttributeSupport)` for
`TrimMode=partial`, the default in .NET MAUI apps.

See #9526 for how we might
better address this in the future.

In order to test this change:

* Add a `XmlSerializerTest` to `Mono.Android-Tests`

* Run a copy of `Mono.Android-Tests` with `-p:TestsFlavor=TrimModePartial`

* Also set `$(_DefaultValueAttributeSupport)` for `TrimMode=full` in
  our test project, so `XmlSerializerTest` will pass in that mode as
  well.

Co-authored-by: Jonathan Peppers <[email protected]>
jonathanpeppers added a commit to dotnet/android that referenced this issue Nov 20, 2024
…=partial` (#9525)

Context: dotnet/runtime#109724

In .NET 9, certain apps could crash with:

    System.ArgumentException: RuntimeInstanceNotAllowed
      ?, in object DefaultValueAttribute.get_Value()
      ?, in new XmlAttributes(ICustomAttributeProvider)
      ?, in XmlAttributes XmlReflectionImporter.GetAttributes(MemberInfo)
      ?, in bool XmlReflectionImporter.InitializeStructMembers(StructMapping, StructModel, bool, string, RecursionLimiter)
      ?, in StructMapping XmlReflectionImporter.ImportStructLikeMapping(StructModel, string, bool, XmlAttributes, RecursionLimiter)

.NET's concept of `$(PublishTrimmed)` is used to decide which trimmer
feature switches are toggled. This is normally set for both Debug &
Release, but Android only sets it for Release. This means that the
`$(_DefaultValueAttributeSupport)` feature switch is not set properly
in some cases, which causes the crash.

For now, we can set `$(_DefaultValueAttributeSupport)` for
`TrimMode=partial`, the default in .NET MAUI apps.

See #9526 for how we might
better address this in the future.

In order to test this change:

* Add a `XmlSerializerTest` to `Mono.Android-Tests`

* Run a copy of `Mono.Android-Tests` with `-p:TestsFlavor=TrimModePartial`

* Also set `$(_DefaultValueAttributeSupport)` for `TrimMode=full` in
  our test project, so `XmlSerializerTest` will pass in that mode as
  well.

Co-authored-by: Jonathan Peppers <[email protected]>
@matouskozak
Copy link
Member

System.ComponentModel.DefaultValueAttribute.IsSupported is not getting passed as a feature setting at all (on Android it is passed with value false) and passing it forcefully doesn't change the behavior.

That surprises me - we should make sure to understand this. I don't think AggressiveAttributeTrimming would be making the difference based on a quick look, since DefaultValueAttribute isn't one of the trimmed attributes under https://github.com/dotnet/runtime/blob/e3b3aaaf21b88b7992cd8a42321e01cf1aa704a2/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.Shared.xml#L89C68-L90.

Putting additional update as I continue investigating:

  • NativeAOT on iOS is passing System.ComponentModel.DefaultValueAttribute.IsSupported to the ILLink FeatureSettings yet it doesn't crash or produce warnings.
  • On Android, the MAUI dll is rooted with all option, whereas on iOS it uses entrypoint
  • The System.Xml.Serialization assembly is getting trimmed out on iOS as per: Image

Note: the android fix got merged and will be part of .NET 9 servicing (dotnet/android@20f08bd). I'll keep this issue open to continue investigating the iOS case.

@matouskozak matouskozak removed the untriaged New issue has not been triaged by the area owner label Nov 22, 2024
@matouskozak
Copy link
Member

Update on the iOS investigation.

I believe the XMLSeriliazer behavior is related to DynamicCodeSupport on iOS (perhaps related to #109724).

By default, DynamicCodeSupport=false on iOS. However, even when DynamicCodeSupport is enabled, the throwing behavior doesn't happen as the System.ComponentModel.DefaultValueAttribute.IsSupported is still NULL. However, forcefully disabling the DefaultValueAttributeSupport triggers the throw.

At the moment I'm unsure why System.ComponentModel.DefaultValueAttribute.IsSupported is NULL and not being set but it appears that when it is not set, it defaults to true

internal static bool IsSupported => AppContext.TryGetSwitch("System.ComponentModel.DefaultValueAttribute.IsSupported", out bool isSupported) ? isSupported : true;

Not sure if this is intended behavior @sbomer but at least we don't need any fix for iOS currently.

@sbomer
Copy link
Member

sbomer commented Dec 10, 2024

I believe the XMLSeriliazer behavior is related to DynamicCodeSupport on iOS (perhaps related to #109724).

Did you mean to link a different issue?

it appears that when it is not set, it defaults to true

That is expected - the code is shared with coreclr's corelib where the default must be true for un-trimmed apps, and we rely on the setting being passed at publish time to disable the feature.

I'm unsure why System.ComponentModel.DefaultValueAttribute.IsSupported is NULL

Agreed, I would expect the trimming settings from https://github.com/dotnet/runtime/blob/f9c86dc7b449bbdfd2fcafa58e513357372509a5/src/tools/illink/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets#L34-L61 to kick in and disable to kick in.

@matouskozak
Copy link
Member

Did you mean to link a different issue?

Yes, my bad. #107252

I'm unsure why System.ComponentModel.DefaultValueAttribute.IsSupported is NULL

Agreed, I would expect the trimming settings from https://github.com/dotnet/runtime/blob/f9c86dc7b449bbdfd2fcafa58e513357372509a5/src/tools/illink/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets#L34-L61 to kick in and disable to kick in.

cc: @rolfbjarne do you happen to know why the trimming settings from the linked Microsoft.NET.ILLink.targets are not being set on iOS?

The feature settings for the ILLink task that are set:
Image

@rolfbjarne
Copy link
Member

cc: @rolfbjarne do you happen to know why the trimming settings from the linked Microsoft.NET.ILLink.targets are not being set on iOS?

Do you have a binlog I can look at?

@rolfbjarne
Copy link
Member

cc: @rolfbjarne do you happen to know why the trimming settings from the linked Microsoft.NET.ILLink.targets are not being set on iOS?

Do you have a binlog I can look at?

Actually I think I know the difference: we set PublishTrimmed=true in a target:

https://github.com/xamarin/xamarin-macios/blob/a8eeb279f87b9d801255cf97d91c7ac49057bec1/dotnet/targets/Xamarin.Shared.Sdk.targets#L310-L314

which is after you check for PublishTrimmed to set the defaults:

https://github.com/dotnet/runtime/blob/f9c86dc7b449bbdfd2fcafa58e513357372509a5/src/tools/illink/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets#L34C32-L34C46

The reason for this is that when building on Windows, and the build is not connected to a Mac (i.e. using Hot Restart), we can't run ILLink (one reason being that the custom linker steps only works on macOS) - but connecting to a Mac is done in a target, so we only know if we're connected to a Mac or not later in the build.

You could probably confirm this by setting PublishTrimmed=true in your csproj, which should reproduce the Android behavior.

Ideally (from our point of view at least!) you'd set the trimmer defaults in a public target, which we could make our "_ComputePublishTrimmed" target depend on. It's not the first time this problem with default values being out of sync comes up, we have a rather big list of copied defaults which I'd love to be able to get rid of:

https://github.com/xamarin/xamarin-macios/blob/a8eeb279f87b9d801255cf97d91c7ac49057bec1/dotnet/targets/Xamarin.Shared.Sdk.targets#L122-L149

@matouskozak
Copy link
Member

Thank you Rolf.

I can confirm that the issue also reproduces when you set manually <PublishTrimmed>true</PublishTrimmed> in csproj.

To reproduce the crash on iOS you need to:

  • Enable interpreter (dynamic code support): <UseInterpreter>true</UseInterpreter> (<DynamicCodeSupport>true</DynamicCodeSupport>)
  • <_DefaultValueAttributeSupport>false</_DefaultValueAttributeSupport>

or

  • <PublishTrimmed>true</PublishTrimmed> which is not recommended as it always should be set to true on iOS and is handled by the iOS sdk.

@sbomer
Copy link
Member

sbomer commented Dec 11, 2024

Thanks @rolfbjarne and @matouskozak for the investigation.

we set PublishTrimmed=true in a target: [....] which is after you check for PublishTrimmed to set the defaults:

Ah, right, I remember this coming up before. We need to set the defaults early enough for them to flow to RuntimeHostConfigurationOption, which is currently computed during evaluation: https://github.com/dotnet/sdk/blob/20ba0c29c5be4ebc4d14b453c0efc743a43bf277/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets#L515-L520.

Setting these early usually ensures that they flow into the build (not just publish). Our thinking is that PublishTrimmed signals intent to trim during build, as well as actually trimming during publish.

Ideally (from our point of view at least!) you'd set the trimmer defaults in a public target, which we could make our "_ComputePublishTrimmed" target depend on.

Just to make sure I understand - is the idea that the dependency would be conditioned on the computed value of PublishTrimmed?

From our point of view I think we want to encourage folks to set PublishTrimmed early - see what we are recommending for Android: dotnet/android#9526. Could iOS use a similar approach (set PublishTrimmed even for the Windows build, and use some other mechanism to prevent running ILLink)? cc @vitek-karas for opinions.

@vitek-karas
Copy link
Member

From our point of view I think we want to encourage folks to set PublishTrimmed early

I think we should try to change the behavior to set PublishTrimmed=true early on. There's some complexity around TrimMode=partial but that's already handled by the mobile SDKs.

I do agree that we need a different mechanism to suppress running ILLink - we've ran into this several times now. It's easy enough to override the internal target, but that still leaves lot of the associated things in place, so we might want to make this a bit more "official" (not in the sense of exposing this as a public contract, but have some prescribed way to do this and use that across all the places).

As described above, the meaning of PublishTrimmed=true sort of changed over time. Now it means the intent to trim the app in some configuration. It doesn't mean that the trimmer has to run in all cases - we already have several such examples:

  • It doesn't run during build (it being called Publish helps here, but still)
  • It doesn't run when we use NativeAOT (the ilc will trim, but the illink won't run, so it's similar)
    But otherwise it has the same effect in all cases (feature switches, analyzers, ...).
    (There was a discussion about the name having Publish in it even though it's not a publish-only property anymore, and we gave up on trying to fix that :-))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants