[release/5.0] DataContractSerialization doesn't work with TrimMode - link #42911
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
eerhardt
changed the title
DataContractSerialization has some Reflection "shim" methods that the ILLinker can't see through. This causes critical methods to be trimmed and applications to fail. These methods were put in place in .NET Core 1.0 when the full Reflection API wasn't available. The fix is to remove these "shim" Reflection APIs and use Reflection directly. Fix #41525 Fix #42754
eerhardt
force-pushed
the
backport/pr-42824-to-release/5.0-rc2
branch
from
a0c79b9 to
984c6a2
Compare
eerhardt
added
area-Serialization
Servicing-consider
eerhardt
requested review from
jkotas,
mconnew,
vitek-karas,
HongGit,
MichalStrehovsky and
StephenMolloy
jkotas
approved these changes
Sep 30, 2020
marek-safar
approved these changes
Oct 1, 2020
pranavkm
reviewed
Oct 1, 2020
| @@ -392,7 +392,7 @@ internal static bool IsNonAttributedTypeValidForSerialization(Type type) | |||
| else | |||
| { | |||
| return (type.IsVisible && | |||
| type.GetConstructor(BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public, Array.Empty<Type>()) != null); | |||
| type.GetConstructor(BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public, null, Array.Empty<Type>(), null) != null); | |||
There was a problem hiding this comment.
Is it uncommon to use named parameters in this codebase?
There was a problem hiding this comment.
No it isn't uncommon, but I'm not sure it adds a ton of value in these cases.
Note I've opened #42753 to make this overload an official API, so these nulls won't be necessary in the future.
| @@ -38,7 +38,7 @@ private static MethodInfo ObjectEquals | |||
| { | |||
| if (s_objectEquals == null) | |||
| { | |||
| s_objectEquals = Globals.TypeOfObject.GetMethod("Equals", BindingFlags.Public | BindingFlags.Static); | |||
| s_objectEquals = typeof(object).GetMethod("Equals", BindingFlags.Public | BindingFlags.Static); | |||
There was a problem hiding this comment.
I realize this isn't new, but could we specify the parameter types in the GetMethod call?
There was a problem hiding this comment.
We can do that in the master branch with this follow up issue: #42861.
|
@mconnew could you please sign off? should be a direct port of what you already approved. |
mconnew
approved these changes
Oct 1, 2020
leecow
added
Servicing-approved
|
thanks! |
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport of #42824 to release/5.0
/cc @eerhardt
Customer Impact
DataContractSerializer doesn't work at all in an application that is trimmed with TrimMode=link. This includes Blazor WASM apps by default. It can also include any app that sets PublishTrimmed=true and TrimMode=link.
Customer reported, eg dotnet/aspnetcore#25909 (in Blazor) which shows up as a regression from earlier Blazor.
Testing
I tested the scenarios listed in both bugs logged for this issue, which were reading objects from a data stream:
#41525
#42754
I also tested that writing objects to a data stream works with trimming as well.
A new "trimming test" was added as well.
Risk
Low. This changes the way methods are discovered through Reflection in DataContractSerialization. Instead of doing its own filtering, it now uses the built-in Reflection APIs to find the method.