Skip to content

[Blazor] Workaround linker regression #45028

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

Merged
merged 1 commit into from
Nov 14, 2022

Conversation

javiercn
Copy link
Member

@javiercn javiercn commented Nov 11, 2022

The linker seems to be incorrectly trimming the getters for one of our types. This PR includes a descriptor to ensure that the members of the exchange type are never serialized out and avoids using a serialization context as that breaks serialization for user provided data types.

Description

The linker is linking out the getters for one of our exchange types. This is the link output in 7.0
MicrosoftTeams-image

This is the link output in 6.0
MicrosoftTeams-image (1)

Fixes #44973

Customer Impact

Breaks the ability to return to the original page after the user is redirected for authentication purposes as well as the ability to pass options to control the login process.

Workaround

Add this to your csproj

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

And this is TrimmerRootDescriptor.xml:

<?xml version="1.0" encoding="UTF-8" ?>
<linker>
	<assembly fullname="Microsoft.Authentication.WebAssembly.Msal" preserve="all" />
	<assembly fullname="Microsoft.AspNetCore.Components.WebAssembly.Authentication" preserve="all" />
</linker>

Regression?

  • Yes
  • No

6.0

Risk

  • High
  • Medium
  • Low

The fix uses a linker descriptor to explicitly prevent the members for that type from being trimmed away.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

When servicing release/2.1

  • Make necessary changes in eng/PatchConfig.props

@javiercn javiercn requested a review from a team as a code owner November 11, 2022 16:43
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Nov 11, 2022
@javiercn javiercn added Servicing-consider Shiproom approval is required for the issue and removed area-blazor Includes: Blazor, Razor Components labels Nov 11, 2022
@ghost
Copy link

ghost commented Nov 11, 2022

Hi @javiercn. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@mkArtakMSFT mkArtakMSFT added the area-blazor Includes: Blazor, Razor Components label Nov 11, 2022
@javiercn
Copy link
Member Author

The associated linker issue in the runtime is dotnet/linker#3110

@mkArtakMSFT mkArtakMSFT added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Nov 11, 2022
@ghost
Copy link

ghost commented Nov 11, 2022

Hi @javiercn. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed.

@javiercn
Copy link
Member Author

@dotnet/aspnet-build can I get some help here? Seems that one leg is failing to restore

@dougbu
Copy link
Member

dougbu commented Nov 12, 2022

Should be fixed in the target branch. Right @wtgodbe

@dougbu
Copy link
Member

dougbu commented Nov 12, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@javiercn
Copy link
Member Author

@wtgodbe can you help so that this does not miss the train?

@wtgodbe
Copy link
Member

wtgodbe commented Nov 14, 2022

##[error].dotnet/sdk/7.0.100/NuGet.targets(132,5): error MSB4018: (NETCORE_ENGINEERING_TELEMETRY=Restore) System.Threading.AbandonedMutexException: The wait completed due to an abandoned mutex. [/tmp/d043a9d2-cb34-439f-a93f-c7e1e4b6d110/restore.csproj]

@mmitche we're still seeing the AbandonedMutexException despite azp run-ing after the Arcade update: #44972

@wtgodbe wtgodbe merged commit 4fcc0f0 into release/7.0 Nov 14, 2022
@wtgodbe wtgodbe deleted the javiercn/workaround-linker-regression branch November 14, 2022 18:05
@wtgodbe
Copy link
Member

wtgodbe commented Nov 14, 2022

I'm just force-merging this since the failure is unrelated

@wtgodbe
Copy link
Member

wtgodbe commented Nov 14, 2022

If these failures are persistent we should un-revert the init nuget stuff from https://github.com/dotnet/aspnetcore/pull/44972/commits

[JsonSerializable(typeof(JsonElement))]
internal partial class InteractiveRequestOptionsSerializerContext : JsonSerializerContext
{
public IEnumerable<string> Scopes { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Microsoft framework guidelines say you should use ICollection instead of IEnumerable.


public InteractionType Interaction { get; set; }

public Dictionary<string, object> AdditionalRequestParameters { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use IDictionary instead of Dictionary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants