XmlSerializer support for IsDynamicCodeSupported=false #59386

eerhardt · 2021-09-21T00:07:11Z

Add more checks to XmlSerializer to check the SerializationMode. Don't try to use Reflection.Emit if the SerializationMode is ReflectionOnly.

These changes were ported from

Fix #59167

NOTE: I'm not sure how to test these changes. If anyone has any ideas how to add tests, please let me know.

cc @steveisok @marek-safar

Add more checks to XmlSerializer to check the SerializationMode. Don't try to use Reflection.Emit if the SerializationMode is ReflectionOnly. These changes were ported from * dotnet/runtimelab#593 * dotnet/runtimelab#600 Fix dotnet#59167

steveisok · 2021-09-21T01:23:07Z

@eerhardt Pull this into the iOS functional test. I believe that matches Rolf's test case.

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.Xml/tests/TrimmingTests/XmlSerializer.Deserialize.cs

steveisok · 2021-09-21T14:05:36Z

XmlSerializerTests.Xml_DifferentSerializeDeserializeOverloads is the failing test in System.Xml.XmlSerializer.ReflectionOnly.Tests

eerhardt · 2021-09-21T20:56:24Z

Pull this into the iOS functional test. I believe that matches Rolf's test case.

@steveisok - I did that, but the test still passes locally without any product modifications.

Looking into Rolf's case, what I discern is happening is the linker has an optimization that seals classes that aren't being derived from. The Reflection.Emit code is failing because it is trying to generate a new type that derives from XmlSerializer (which has now become sealed by the linker optimization). Since this optimization isn't happening, the test isn't failing.

I would have thought that since on iOS IsDynamicCodeSupported == false, that calling any Reflection.Emit APIs would cause a runtime exception. But that doesn't appear to be the case (since the tests sill passes without any product changes).

eerhardt · 2021-09-21T20:58:37Z

src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializer.cs

@@ -534,6 +546,16 @@ public virtual bool CanDeserialize(XmlReader xmlReader)
            {
                return _tempAssembly.CanRead(_mapping, xmlReader);
            }
+            else if (ShouldUseReflectionBasedSerialization(_mapping) || _isReflectionBasedSerializer)


@HongGit @StephenMolloy - one XmlSerializer\ReflectionOnly test was failing with the other changes because CanDeserialize was returning false when _primitiveType == null and _tempAssembly == null. So I wrote this logic to handle that case. Can you take a look?

I plan on porting this PR back to 6.0, so it would be good to get eyes on it.

@MichalStrehovsky - any thoughts on why that XmlSerializer\ReflectionOnly test doesn't fail in runtimelab's NativeAOT branch?

We don't run those tests in the NativeAOT branch. The set of fixes we have in the NativeAOT branch comes from manually porting the UWP ifdefs from an older version of the source code.

Looks like I missed this path.

This is how it looked like:

https://github.com/dotnet/corefx/blob/e31a2b7f3bdf985f56798df530daa4c06fd80376/src/System.Private.Xml/src/System/Xml/Serialization/XmlSerializer.cs#L682-L700

Thanks, @MichalStrehovsky. I took the UAP logic (using ReflectionMethodEnabled) and brought it into this PR.

Logically speaking, the conditions for Serialize()/Deserialize() to try the reflection-based route are exactly what is in this 'else' statement. And we check this before checking for _tempAssembly, so I would move this condition ahead in the pecking order, between _primitiveType and _tempAssembly.

I would also note that when matching this condition, we will try to do reflection-based serialization, period. No fallback. So I might consider just returning true, since returning ReflectionMethodEnabled does not account for SOAP mappings. My only concern would be that I don't know how robust the reflection-based serialization will be, and we might perhaps be saying "yes we can deserialize" when in fact we will fail when trying reflection-based serialization. (Of course, it looks like we were probably still saying 'true' for all reflection-based scenarios before, so ¯\_ (ツ)_/¯ ) @mconnew, any thoughts on this?

so I would move this condition ahead in the pecking order

I can switch the if conditions around so they match Deserialize. That makes sense to me. Originally I did it this way "just in case" it doesn't break someone when _tempAssembly is available. But it should logically match Deserialize.

I would also note that when matching this condition, we will try to do reflection-based serialization, period. No fallback. So I might consider just returning true, since returning ReflectionMethodEnabled

Honestly - the Mode property in this code is confusing/convoluted. Does anyone actually set it to CodeGenOnly or PreGenOnly? I don't see any code (other than tests) doing that.

Since Mode can never be anything other than ReflectionOnly or ReflectionAsBackup, I can just return true here. But if we did want to respect Mode for CodeGenOnly or PreGenOnly, I think it should logically use ReflectionMethodEnabled. You are right that if you had a SOAP mapping, and call Deserialize with Mode == CodeGenOnly | PreGenOnly, nothing is checking the Mode, so it should work.

Well... um... yeah. Like I said, XmlSerializer has been cobbled together from many different iterations on previous platforms. So it's kind of a mess. Reflection-only is a .Net Core thing I believe - just kind of stapled on to the mess that was already XmlSerializer moving through various runtimes. As we've hinted at previously, we do hope to clean this all up in 7.0 as we try to move to reflection-based serialization as the primary mode of operation rather than a niche alternative.

StephenMolloy

This looks mostly fine to me... but XmlSerializer has been cobbled together from so many different product iterations over the years, and the reflection-based implementation is relatively new. I'm worried that there might be unintended consequences here that aren't immediately obvious, like the issue with the linker sealing non-derived classes you mentioned. Does this meet the bar for 6.0 this late in the cycle?

StephenMolloy · 2021-09-22T03:42:41Z

src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializer.cs

@@ -270,7 +279,10 @@ public XmlSerializer(Type type, XmlAttributeOverrides? overrides, Type[]? extraT
            DefaultNamespace = defaultNamespace;
            _rootType = type;
            _mapping = GenerateXmlTypeMapping(type, overrides, extraTypes, root, defaultNamespace);
-            _tempAssembly = GenerateTempAssembly(_mapping, type, defaultNamespace, location);
+            if (Mode != SerializationMode.ReflectionOnly)


Nit, because we want to restructure this RefEmit/Reflection-Based dichotomy in 7.0... but SOAP mappings also trigger using reflection-based serialization. Perhaps consider using if (!ShouldUseReflectionBasedSerialization(_mapping)) in these three constructors?

Since we are late in 6.0, I wanted to make the least risk change that fixes the issue. The least risk (IMO) is to use what worked in UAP and in the NativeAOT branch.

This suggestion would affect SOAP in "normal" applications, not just when IsDynamicCodeSupported=false. I can log an issue for someone to investigate taking this approach in 7.0.

StephenMolloy · 2021-09-22T03:53:45Z

src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializer.cs

@@ -534,6 +546,16 @@ public virtual bool CanDeserialize(XmlReader xmlReader)
            {
                return _tempAssembly.CanRead(_mapping, xmlReader);
            }
+            else if (ShouldUseReflectionBasedSerialization(_mapping) || _isReflectionBasedSerializer)


Logically speaking, the conditions for Serialize()/Deserialize() to try the reflection-based route are exactly what is in this 'else' statement. And we check this before checking for _tempAssembly, so I would move this condition ahead in the pecking order, between _primitiveType and _tempAssembly.

I would also note that when matching this condition, we will try to do reflection-based serialization, period. No fallback. So I might consider just returning true, since returning ReflectionMethodEnabled does not account for SOAP mappings. My only concern would be that I don't know how robust the reflection-based serialization will be, and we might perhaps be saying "yes we can deserialize" when in fact we will fail when trying reflection-based serialization. (Of course, it looks like we were probably still saying 'true' for all reflection-based scenarios before, so ¯\_ (ツ)_/¯ ) @mconnew, any thoughts on this?

rolfbjarne · 2021-09-22T08:32:13Z

I would have thought that since on iOS IsDynamicCodeSupported == false, that calling any Reflection.Emit APIs would cause a runtime exception. But that doesn't appear to be the case (since the tests sill passes without any product changes).

I believe that just calling Reflection.Emit when IsDynamicCodeSupported=false is not a problem, it's perfectly possible to create an assembly and save it to disk.

The problem is that you can't JIT/execute that assembly (because there's no JIT).

And I believe this is exactly what's happening here - I had a deeper look, and the test case actually runs successfully when the linker is disabled, because I think that while the XmlSerializer generates the temp assembly, it's not used at runtime, the reflection-based implementation will be used instead:

runtime/src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializer.cs

Line 475 in 2d80343

return DeserializeUsingReflection(xmlReader, encodingStyle, events);

In the end it seems the real issue is that the XmlSerializer has dead code that is incompatible with a linker optimization... and if that's the case, actually executing a test in a FullAOT environment won't ensure it's fixed (because it works at runtime, you need the linker to execute to break the dead code). Maybe a linker test that verifies that the GenerateTempAssembly method has been linked away would work?

MichalStrehovsky · 2021-09-22T08:43:58Z

it's perfectly possible to create an assembly and save it to disk.

It's tangential, but the Save APIs only exist in .NET Framework. They were not brought over to .NET Core and there are no plans to bring them back in this form. The Mono CoreLib might get size savings by stubbing all of the Emit APIs out with a throw when interpreter/JIT is disabled because they cannot be used for anything useful - it will lead to more predictable behaviors.

eerhardt · 2021-09-22T16:20:43Z

Does this meet the bar for 6.0 this late in the cycle?

@marek-safar requested I look at it for 6.0 in #59167 (comment). I'll leave it up to tactics if they want to take the change or not.

… when IsDynamicCodeSupported==false.

eerhardt · 2021-09-22T22:49:41Z

@mconnew @StephenMolloy - any more thoughts here? I'd like to get this in so I can start the 6.0 backport process.

mconnew · 2021-09-23T00:11:24Z

I'm okay with these changes. Thanks for putting in the comment about the trade off of complexity of change and risk of false negative vs false positive. It will help refresh memory when this is revisited later.

eerhardt · 2021-09-23T01:12:24Z

/backport to release/6.0-rc2

github-actions · 2021-09-23T01:12:38Z

Started backporting to release/6.0-rc2: https://github.com/dotnet/runtime/actions/runs/1264095479

eerhardt added the area-Serialization label Sep 21, 2021

eerhardt requested review from MaximLipnin, mconnew, HongGit, MichalStrehovsky and StephenMolloy September 21, 2021 00:07

Fix a bug in XmlSerializer.CanDeserialize when in ReflectionOnly mode.

ccb3aa0

eerhardt commented Sep 21, 2021

View reviewed changes

Port UAP code for CanDeserialize

cdd1d14

StephenMolloy reviewed Sep 22, 2021

View reviewed changes

eerhardt added 2 commits September 22, 2021 11:41

PR feedback

f1b0cac

Add a linker test to ensure linker option '--enable-opt sealer' works…

d6fb135

… when IsDynamicCodeSupported==false.

mconnew approved these changes Sep 23, 2021

View reviewed changes

StephenMolloy approved these changes Sep 23, 2021

View reviewed changes

eerhardt merged commit 9114f77 into dotnet:main Sep 23, 2021

eerhardt deleted the Fix59167 branch September 23, 2021 01:10

github-actions bot mentioned this pull request Sep 23, 2021

[release/6.0-rc2] XmlSerializer support for IsDynamicCodeSupported=false #59507

Merged

ghost locked as resolved and limited conversation to collaborators Nov 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XmlSerializer support for IsDynamicCodeSupported=false #59386

XmlSerializer support for IsDynamicCodeSupported=false #59386

eerhardt commented Sep 21, 2021

steveisok commented Sep 21, 2021

steveisok commented Sep 21, 2021

eerhardt commented Sep 21, 2021

eerhardt Sep 21, 2021

eerhardt Sep 21, 2021

MichalStrehovsky Sep 21, 2021

eerhardt Sep 21, 2021

StephenMolloy Sep 22, 2021

eerhardt Sep 22, 2021

StephenMolloy Sep 23, 2021

StephenMolloy left a comment

StephenMolloy Sep 22, 2021

eerhardt Sep 22, 2021

StephenMolloy Sep 22, 2021

rolfbjarne commented Sep 22, 2021

MichalStrehovsky commented Sep 22, 2021

eerhardt commented Sep 22, 2021

eerhardt commented Sep 22, 2021

mconnew commented Sep 23, 2021

eerhardt commented Sep 23, 2021

github-actions bot commented Sep 23, 2021

XmlSerializer support for IsDynamicCodeSupported=false #59386

XmlSerializer support for IsDynamicCodeSupported=false #59386

Conversation

eerhardt commented Sep 21, 2021

steveisok commented Sep 21, 2021

steveisok commented Sep 21, 2021

eerhardt commented Sep 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenMolloy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolfbjarne commented Sep 22, 2021

MichalStrehovsky commented Sep 22, 2021

eerhardt commented Sep 22, 2021

eerhardt commented Sep 22, 2021

mconnew commented Sep 23, 2021

eerhardt commented Sep 23, 2021

github-actions bot commented Sep 23, 2021