dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.49k stars 4.77k forks source link

JSON deserialization fails in iOS release build if class implements an interface #75802

Closed markuspalme closed 1 year ago

markuspalme commented 2 years ago

Description

A simple JSON deserialization using Newtonsoft.Json fails on iOS in a release build as soon as the class that is trying be be de-serialized implements an interface.

The same app works in Debug build in the iOS simulator as expected.

Reproduction Steps

  1. Create a new iOS app with dotnet new ios

  2. Set a valid bundle identifier that allows installing the app on a physical device

  3. Add this code to the project:

    public class ProductImage : IEntity
    {
       public string ProductNumber { get; set; }
       public Guid Id { get; set; }
    }
    
    public interface IEntity
    {
       Guid Id { get; set; }
    }

    During startup, run this code:

      var x = @"{
         ""productNumber"": ""P1"",
         ""id"": ""46c67d7c-fd15-4d89-9fed-991c48c1bacf""
      }";
    
      var pi = JsonConvert.DeserializeObject<ProductImage>(x);
    
  4. Build the app for a device: dotnet publish testapp.csproj -f:net6.0-ios -c:Release /p:CodesignKey="Apple Development: ******" /p:CodesignProvision="Test app" /p:ArchiveOnBuild=true -r ios-arm64 --self-contained

  5. Deploy to device and run

Full project file:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>net6.0-ios</TargetFramework>
    <OutputType>Exe</OutputType>
    <Nullable>enable</Nullable>
    <ImplicitUsings>true</ImplicitUsings>
    <SupportedOSPlatformVersion>13.0</SupportedOSPlatformVersion>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="Newtonsoft.Json" Version="13.0.1" />
  </ItemGroup>
</Project>

Self-contained example project: https://github.com/markuspalme/dotnetruntime-75802

Expected behavior

The app starts successfully and manages to de-serialize the JSON regardless of whether the class implements an interface or not.

Note that removing the interface will make the de-serialization work as expected.

Actual behavior

JSON deserialization fails with this exception:

Error setting value to 'Id' on 'testapp.ProductImage'.

Regression?

The same code works in Xamarin.iOS.

Known Workarounds

No response

Configuration

.NET 6.0.401 iOS 16, same on 15.6.1

Other information

No response

ghost commented 2 years ago

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

Issue Details
### Description A simple JSON deserialization using `Newtonsoft.Json` fails on iOS in a release build as soon as the class that is trying be be de-serialized implements an interface. The same app works in Debug build in the iOS simulator as expected. ### Reproduction Steps 1. Create a new iOS app with `dotnet new ios` 2. Set a valid bundle identifier that allows installing the app on a physical device 3. Add this code to the project: ``` public class ProductImage : IEntity { public string ProductNumber { get; set; } public Guid Id { get; set; } } public interface IEntity { Guid Id { get; set; } } ``` During startup, run this code: ``` var x = @"{ ""productNumber"": ""P1"", ""id"": ""46c67d7c-fd15-4d89-9fed-991c48c1bacf"" }"; var pi = JsonConvert.DeserializeObject(x); ``` 4. Build the app for a device: `dotnet publish testapp.csproj -f:net6.0-ios -c:Release /p:CodesignKey="Apple Development: ******" /p:CodesignProvision="Test app" /p:ArchiveOnBuild=true -r ios-arm64 --self-contained` 5. Deploy to device and run Full project file: ``` net6.0-ios Exe enable true 13.0 ``` ### Expected behavior The app starts successfully and manages to de-serialize the JSON regardless of whether the class implements an interface or not. Note that removing the interface ### Actual behavior JSON deserialization fails with this exception: `Error setting value to 'Id' on 'testapp.ProductImage'.` ### Regression? The same code works in Xamarin.iOS. ### Known Workarounds _No response_ ### Configuration .NET 6.0.4.1 iOS 16 ### Other information _No response_
Author: markuspalme
Assignees: -
Labels: `area-System.Text.Json`, `untriaged`
Milestone: -
gregsdennis commented 2 years ago

Are you certain this is a bug in .Net? Seems like it's a Newtonsoft issue. If so, the bug should be filed in that repo.

markuspalme commented 2 years ago

@gregsdennis It works just fine in net6 on Windows, MacOS and also in the iOS simulator - all with the same Newtonsoft.Json package. So I think this is a runtime or maybe AOT issue (iOS requires AOT).

gregsdennis commented 2 years ago

Understood, but it might be that the package just needs handle a special case in this OS. There is the possibility that the issue is the package.

markuspalme commented 2 years ago

Unlikely, the same code works with Xamarin.iOS.

Cheesebaron commented 2 years ago

@markuspalme could it be the linker stripping out stuff?

Could you try add <PublishTrimmed>false</PublishTrimmed> in your Project Properties and see if that helps?

markuspalme commented 2 years ago

@Cheesebaron Thanks for the suggestion, on iOS that is not an option though:

image
ghost commented 2 years ago

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger See info in area-owners.md if you want to be subscribed.

Issue Details
### Description A simple JSON deserialization using `Newtonsoft.Json` fails on iOS in a release build as soon as the class that is trying be be de-serialized implements an interface. The same app works in Debug build in the iOS simulator as expected. ### Reproduction Steps 1. Create a new iOS app with `dotnet new ios` 2. Set a valid bundle identifier that allows installing the app on a physical device 3. Add this code to the project: ``` public class ProductImage : IEntity { public string ProductNumber { get; set; } public Guid Id { get; set; } } public interface IEntity { Guid Id { get; set; } } ``` During startup, run this code: ``` var x = @"{ ""productNumber"": ""P1"", ""id"": ""46c67d7c-fd15-4d89-9fed-991c48c1bacf"" }"; var pi = JsonConvert.DeserializeObject(x); ``` 4. Build the app for a device: `dotnet publish testapp.csproj -f:net6.0-ios -c:Release /p:CodesignKey="Apple Development: ******" /p:CodesignProvision="Test app" /p:ArchiveOnBuild=true -r ios-arm64 --self-contained` 5. Deploy to device and run Full project file: ``` net6.0-ios Exe enable true 13.0 ``` ### Expected behavior The app starts successfully and manages to de-serialize the JSON regardless of whether the class implements an interface or not. Note that removing the interface will make the de-serialization work as expected. ### Actual behavior JSON deserialization fails with this exception: `Error setting value to 'Id' on 'testapp.ProductImage'.` ### Regression? The same code works in Xamarin.iOS. ### Known Workarounds _No response_ ### Configuration .NET 6.0.401 iOS 16, same on 15.6.1 ### Other information _No response_
Author: markuspalme
Assignees: -
Labels: `area-System.Text.Json`, `untriaged`, `os-ios`
Milestone: -
markuspalme commented 2 years ago

I have tried System.Text.Json instead of Newtonsoft.Json. It does not throw an exception, but it does not set any property values.

var pi = System.Text.Json.JsonSerializer.Deserialize<ProductImage>(x);
markuspalme commented 2 years ago

Here's a small self-contained example: https://github.com/markuspalme/dotnetruntime-75802

markuspalme commented 2 years ago

Adding <UseInterpreter>true</UseInterpreter> as suggested in this issue helps - but I don't think this should be neccesary: https://github.com/xamarin/xamarin-macios/issues/15961

Cheesebaron commented 2 years ago

@markuspalme UseInterpreter is not for Release builds, Apple won't allow that.

markuspalme commented 2 years ago

@Cheesebaron Yes, I figured but wanted to share that it helps here - maybe it helps pinpointing the root cause.

ghost commented 2 years ago

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

Issue Details
### Description A simple JSON deserialization using `Newtonsoft.Json` fails on iOS in a release build as soon as the class that is trying be be de-serialized implements an interface. The same app works in Debug build in the iOS simulator as expected. ### Reproduction Steps 1. Create a new iOS app with `dotnet new ios` 2. Set a valid bundle identifier that allows installing the app on a physical device 3. Add this code to the project: ``` public class ProductImage : IEntity { public string ProductNumber { get; set; } public Guid Id { get; set; } } public interface IEntity { Guid Id { get; set; } } ``` During startup, run this code: ``` var x = @"{ ""productNumber"": ""P1"", ""id"": ""46c67d7c-fd15-4d89-9fed-991c48c1bacf"" }"; var pi = JsonConvert.DeserializeObject(x); ``` 4. Build the app for a device: `dotnet publish testapp.csproj -f:net6.0-ios -c:Release /p:CodesignKey="Apple Development: ******" /p:CodesignProvision="Test app" /p:ArchiveOnBuild=true -r ios-arm64 --self-contained` 5. Deploy to device and run Full project file: ``` net6.0-ios Exe enable true 13.0 ``` Self-contained example project: https://github.com/markuspalme/dotnetruntime-75802 ### Expected behavior The app starts successfully and manages to de-serialize the JSON regardless of whether the class implements an interface or not. Note that removing the interface will make the de-serialization work as expected. ### Actual behavior JSON deserialization fails with this exception: `Error setting value to 'Id' on 'testapp.ProductImage'.` ### Regression? The same code works in Xamarin.iOS. ### Known Workarounds _No response_ ### Configuration .NET 6.0.401 iOS 16, same on 15.6.1 ### Other information _No response_
Author: markuspalme
Assignees: -
Labels: `area-System.Text.Json`, `untriaged`, `os-ios`
Milestone: -
eiriktsarpalis commented 2 years ago

@jeffschwMSFT I don't believe the issue is related to System.Text.Json. I removed the label but wasn't sure what the appropriate area label should be. Perhaps @steveisok or @akoeplinger know.

jkotas commented 2 years ago

System.Text.Json should be the right area label.

Source generators are the only 100% reliable way to perform Json serialization/deserialization in the presence of trimming or AOT compilation without fallbacks.

Unfortunately, trim warnings that would notify about this problem are disabled for iOS apps by default. You can set <SuppressTrimAnalysisWarnings>false</SuppressTrimAnalysisWarnings> in your .csproj to enable trim warnings to see all potential places in your app that can break due to trimming.

This is duplicate https://github.com/dotnet/runtime/issues/74141 and number of other similar issues.

@eiriktsarpalis My suggestion would be:

eiriktsarpalis commented 2 years ago

System.Text.Json should be the right area label.

The OP concerns Json.NET (Newtonsoft) serialization issues in iOS. I agree that it falls under the same category as #74141, but I wonder what our approach should be when users report trimming-related failures when using third party reflection-based libraries. We might want to consider asking them to file an issue with said third-party libraries asking about AOT support.

Do we have a documentation that says "System.Text.Json source generators are the only 100% reliable way to perform Json serialization/deserialization in the presence of trimming"?

It is, although the wording is certainly more understated than that.

markuspalme commented 2 years ago

@jkotas I will try to enable the trim warnings.

JSON is at the heart of many apps and this feels like a regression coming from Xamarin.iOS where such scenarios worked out of the box. Migrating toSystem.Text.Json source generators is a possibility for user code, but many libraries depend on Newtonsoft.Json, consider a generic key-value store like Akavache (https://github.com/reactiveui/Akavache)

eiriktsarpalis commented 2 years ago

@markuspalme I'm not sure if and when Json.NET plans on bringing AOT support, but you might want to consider opening an issue in its own repo or those of the other libraries that depend on it. In the meantime, migrating to System.Text.Json source generation is probably the only viable option.

markuspalme commented 2 years ago

@eiriktsarpalis Thanks for your input. Given that earlier versions of Xamarin.iOS supported this scenario out of the box even in AOT mode, this seems like a bad surprise or even a blocker for anyone porting applications to net6-ios.

markuspalme commented 2 years ago

@eiriktsarpalis So my understanding is that this problem here is caused by trimming. To preserve all the types in my code, I have added this to the project file as described here (https://devblogs.microsoft.com/dotnet/customizing-trimming-in-net-core-5/):

<ItemGroup>
  <TrimmerRootDescriptor Include="TrimmerRoots.xml" />
</ItemGroup>

The TrimmerRoots.xml looks like this:

<?xml version="1.0" encoding="utf-8"?>

<linker>
    <assembly fullname="testapp" preserve="all" />
</linker>

The de-serialization does not work with that in place. Shouldn't this ensure that all my types are preserved and available through reflection?

The whole story around trimming and linking is rather intimidating:

Is there any guidance available on which one to use when and how they work together?

ghost commented 2 years ago

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr See info in area-owners.md if you want to be subscribed.

Issue Details
### Description A simple JSON deserialization using `Newtonsoft.Json` fails on iOS in a release build as soon as the class that is trying be be de-serialized implements an interface. The same app works in Debug build in the iOS simulator as expected. ### Reproduction Steps 1. Create a new iOS app with `dotnet new ios` 2. Set a valid bundle identifier that allows installing the app on a physical device 3. Add this code to the project: ``` public class ProductImage : IEntity { public string ProductNumber { get; set; } public Guid Id { get; set; } } public interface IEntity { Guid Id { get; set; } } ``` During startup, run this code: ``` var x = @"{ ""productNumber"": ""P1"", ""id"": ""46c67d7c-fd15-4d89-9fed-991c48c1bacf"" }"; var pi = JsonConvert.DeserializeObject(x); ``` 4. Build the app for a device: `dotnet publish testapp.csproj -f:net6.0-ios -c:Release /p:CodesignKey="Apple Development: ******" /p:CodesignProvision="Test app" /p:ArchiveOnBuild=true -r ios-arm64 --self-contained` 5. Deploy to device and run Full project file: ``` net6.0-ios Exe enable true 13.0 ``` Self-contained example project: https://github.com/markuspalme/dotnetruntime-75802 ### Expected behavior The app starts successfully and manages to de-serialize the JSON regardless of whether the class implements an interface or not. Note that removing the interface will make the de-serialization work as expected. ### Actual behavior JSON deserialization fails with this exception: `Error setting value to 'Id' on 'testapp.ProductImage'.` ### Regression? The same code works in Xamarin.iOS. ### Known Workarounds _No response_ ### Configuration .NET 6.0.401 iOS 16, same on 15.6.1 ### Other information _No response_
Author: markuspalme
Assignees: -
Labels: `question`, `area-System.Text.Json`, `linkable-framework`, `os-ios`, `trimming-for-aot`
Milestone: -
vitek-karas commented 2 years ago

The trimming options are described here: https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/trimming-options?pivots=dotnet-6-0

The Preserve attribute has been deprecated... I don't actually think it works anymore (but maybe it does for iOS targets)

The trimmer should produce warnings if the XML is not understood - but mobile targets disable these warnings by default (there are just way too many warnings produced by all layers of the system currently). You can turn it on by setting SuppressTrimAnalysisWarnings=false and look for warnings pointing to your XML file.

@sbomer for additional details.

markuspalme commented 2 years ago

@vitek-karas I did that and get the expected warning about Newtonsoft.Json. But all types that are involded in the de-serialization are marked for preservation in the XML file.

There are no warnings regarding the XML file. You can access the full project here: https://github.com/markuspalme/dotnetruntime-75802

vitek-karas commented 2 years ago

@sbomer - could you please try to take a look if the XML descriptor works as expected in this case?

sbomer commented 2 years ago

I tried the repro as a win-x64 console app and the descriptor you shared fixes deserialization, so this might be specific to the Xamarin SDK. I am not currently set up to publish this for iOS. @markuspalme would you be able to share a binlog of the publish command?

markuspalme commented 2 years ago

@sbomer here you go: build.binlog.zip

Build command: dotnet publish testapp.csproj -f:net6.0-ios -c:Release /p:CodesignKey="Apple Development: ******" /p:CodesignProvision="Test app" /p:ArchiveOnBuild=true -r ios-arm64 --self-contained -bl:build.binlog

markuspalme commented 2 years ago

@sbomer Did the binlog reveal anything interesting?

steveisok commented 2 years ago

@sbomer if you need to bounce ideas off of someone who has ios set up, please let me know.

sbomer commented 2 years ago

Sorry for the delay - I wasn't able to see anything wrong from the binlog, which indicates that the descriptor is getting passed along correctly. @vitek-karas would your repro tool help here to collect the input assemblies?

sbomer commented 2 years ago

@markuspalme would you be able to try this tool by @vitek-karas to see if you can obtain a repro? https://github.com/vitek-karas/illinkrepro

You'll need to clone it on the machine where you ran the publish command and obtained the binlog. Then inside of the cloned directory, do dotnet run -- create path/to/msbuild.binlog. It tries creates a repro directory containing the linker command-line and input assemblies. If you can share that with us, it would be very helpful.

markuspalme commented 2 years ago

@sbomer Here you go: https://github.com/markuspalme/dotnetruntime-75802/raw/master/repro.zip

sbomer commented 2 years ago

Thanks @markuspalme, that gets us a bit further. I was able to tweak the repro as I described here to get some linker output.

I was hoping that the linker output would show something obviously wrong, but I didn't notice anything. I see that ProductImage's interface implementation of IEntity is kept. It seems like it's actually kept with or without the descriptor, which is a bit surprising since the win-x64 repro I tried (without root descriptor) was failing with the interface and ctor being removed.

Would you also be able to share your publish output (ideally both with and without the descriptor) so I can compare? If that doesn't show what's wrong I will probably need to set up an iOS development environment to debug (or debug it with @steveisok).

markuspalme commented 2 years ago

@sbomer Here are the IPA files:

With descriptor: testapp-descriptor.ipa.zip

Without descriptor: testapp-without-descriptor.ipa.zip

There is is at least a difference in size:

image
sbomer commented 2 years ago

Thanks @markuspalme, in the output you shared I noticed that the ProductImage accessors don't look right:

Screenshot 2022-10-05 121113

I am not seeing the same thing in any of my repros so far. I also got your original repro to run on a simulator, but didn't see the issue there either. Now I'm trying to get set up with a provisioning profile with some help from @steveisok. Thanks for your patience.

markuspalme commented 2 years ago

@sbomer I saw the same but was not sure it's relevant because I don't know if the assembly is used for anything but metadata in the AOT build.

The error is not reproducible on the simulator. I do think it's an AOT issue and not a trimming issue for this reason - but I have no idea how to investigate any further by myself.

sbomer commented 2 years ago

So far I tend to agree that it doesn't look like a linker issue. I think we need someone more familiar with the AOT/Xamarin scenarios to investigate. /cc @steveisok

MichalStrehovsky commented 2 years ago

Looks like the assembly has been ran through ILStrip after trimming. ILStrip builds on top of some unserviced version of Cecil. Is there a way to disable it to get it out of the picture? I can't figure out which targets enable it, but the methods are marked noininling and the bodies are just a ret and that's ILStrip.

I saw some hints in the .targets I found that ILStrip might be disabled if interpreter is enabled and we've established that enabling interpreted fixes it here https://github.com/dotnet/runtime/issues/75802#issuecomment-1250702757 so this might be ILStrip.

MichalStrehovsky commented 2 years ago

Cc @akoeplinger for ILStrip

akoeplinger commented 2 years ago

@MichalStrehovsky @markuspalme you can try setting EnableAssemblyILStripping to false in your csproj

markuspalme commented 2 years ago

@MichalStrehovsky I did that. The assembly is now intact:

image

But the error still occurs on device. As far as I know, the IL is used for metadata only with AOT enabled which is probably why method bodies are stripped away to save space.

markuspalme commented 2 years ago

@MichalStrehovsky Is there anything else I can help with analyzing this issue?

markuspalme commented 1 year ago

@MichalStrehovsky @steveisok @sbomer Could this be related to #69410?

markuspalme commented 1 year ago

Can still be reproduced with .NET 7.

minglu commented 1 year ago

following

steveisok commented 1 year ago

@akoeplinger please investigate if this is indeed an ILStrip issue.

steveisok commented 1 year ago

@akoeplinger do you the recent ILStrip changes might have fixed this? Can you please validate?

markuspalme commented 1 year ago

This is fixed in 8.0.100-preview.7.23376.3, presumably by #69410.