Skip to content

Fix System.Net.Security.Unit.Tests crashes due to stripped Dictionary IL - #132125

Closed
rzikm with Copilot wants to merge 2 commits into
mainfrom
copilot/tvos-coreclr-fix-dictionary-il-stripping
Closed

Fix System.Net.Security.Unit.Tests crashes due to stripped Dictionary IL#132125
rzikm with Copilot wants to merge 2 commits into
mainfrom
copilot/tvos-coreclr-fix-dictionary-il-stripping

Conversation

Copilot AI commented Aug 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • Inspect repository state and CI failure context
  • Locate the affected test project and matching Apple-mobile mitigation
  • Apply the smallest project configuration change
  • Validate the project configuration and review the final diff
  • Run automated review and security checks

Copilot AI lite review requested due to automatic review settings August 11, 2026 09:27
Copilot AI reviewed Aug 11, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

@azure-pipelines

azure-pipelines Bot commented Aug 11, 2026

Copy link
Copy Markdown
Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI linked an issue Aug 11, 2026 that may be closed by this pull request
[tvOS CoreCLR] System.Net.Security.Unit.Tests crashes because required Dictionary IL was stripped from R2R image #131922
Closed
@rzikm
Disable R2R IL stripping for Net.Security unit tests
dcff2d0 
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 11, 2026 09:32
Copilot AI reviewed Aug 11, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@dotnet-policy-service

dotnet-policy-service Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Copilot AI requested a review from rzikm August 11, 2026 09:41
@build-analysis build-analysis Bot mentioned this pull request Aug 11, 2026
Open
@rzikm
rzikm marked this pull request as ready for review August 12, 2026 08:13
@rzikm
rzikm requested review from a team, kotlarmilos and matouskozak August 12, 2026 08:14
@azure-pipelines

azure-pipelines Bot commented Aug 12, 2026

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@rzikm rzikm changed the title [WIP] Fix System.Net.Security.Unit.Tests crashes due to stripped Dictionary IL Fix System.Net.Security.Unit.Tests crashes due to stripped Dictionary IL Aug 12, 2026
@rzikm
rzikm enabled auto-merge (squash) August 12, 2026 08:52
@matouskozak

matouskozak commented Aug 12, 2026

Copy link
Copy Markdown
Member

We're trying to reproduce this locally to see if we come up with a better solution than disabling stripping. So far we we're unable to reproduce it on iPhone. cc: @vitek-karas @davidnguyen-tech

@davidnguyen-tech

davidnguyen-tech commented Aug 12, 2026

Copy link
Copy Markdown
Member

I will try to reproduce the issue locally on tvos by tomorrow

@rzikm
rzikm disabled auto-merge August 12, 2026 13:43
@BrzVlad

BrzVlad commented Aug 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

We have a few library suites that add PublishReadyToRunStripILBodies which is not good behavior, given we should be testing the configuration that we ship by default to user. If there are some tests that we particularly don't care about for iOS, then it might make more sense to disable them individually instead. Normally, I would disable this optimization for .net11 until we figure it out, but on maui sample it would lead to 8% size increase, so it is not a good option either.

@rzikm

rzikm commented Aug 13, 2026

Copy link
Copy Markdown
Member

I am also fine with just disabling the test on non-desktop platforms, I don't think the additional coverage provides much benefit.

@rzikm rzikm mentioned this pull request Aug 13, 2026
Merged
rzikm added a commit that referenced this pull request Aug 14, 2026
@rzikm
Compile UpdateOptions_ServerCertificateContextProvided test on deskto…
48fa6ef 
…p only (#132277)

Alternative to #131929 and #132125 for #131922.

`System.Net.Security.Unit.Tests` crashes during runtime init on
Apple-mobile CoreCLR (e.g. tvOS), before test discovery runs. The single
test
`UpdateOptions_ServerCertificateContextProvided_DoesNotDisposeCallerContext`
exercises `SslStreamCertificateContext.Create` with intermediate
certificates, which pulls in a generic `Dictionary` instantiation whose
IL body is stripped from the Apple-mobile CoreCLR ReadyToRun composite
image, crashing the whole app at startup. Because the app crashes before
test discovery, per-test `[Fact]`/attribute-based skipping can't help.

## Approach

Rather than disabling the entire test project on non-desktop platforms
(as the alternatives do), this keeps the rest of the suite running on
mobile and only excludes the one problematic test from the mobile build:

- Make `SslAuthenticationOptionsTests` a `partial` class.
- Move
`UpdateOptions_ServerCertificateContextProvided_DoesNotDisposeCallerContext`
verbatim into a new `SslAuthenticationOptionsTests.Desktop.cs`.
- Register that file with an MSBuild `Compile` condition limited to
desktop platforms (`windows`/`unix`/`osx`). An MSBuild condition is used
rather than a `#if` because the `TARGET_*` C# defines aren't reliably
present in this test project.

`TargetFrameworks` and the existing `IgnoreForCI` line are untouched, so
the app still builds and all other unit tests still run on mobile.

## Comparison to alternatives

- #131929 changes the test to also supply the root cert so Android's
chain build succeeds. That addresses the Android
`PlatformNotSupportedException` but not the tvOS R2R startup crash,
which is unrelated to the test's certificate inputs.
- #132125 disables the whole `System.Net.Security.Unit.Tests` project in
CI on non-desktop platforms, losing all mobile coverage of these unit
tests.

This PR is the most surgical option: it drops only the one test on
mobile and preserves the remaining coverage.

## Validation

Test-only change; no product code touched. Not built locally (test
project requires the full libraries baseline build in this environment);
the change is a mechanical move plus a build-condition addition. CI will
exercise the desktop and mobile legs.

> [!NOTE]
> This PR was generated with the assistance of GitHub Copilot.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@rzikm

rzikm commented Aug 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

replaced by #132277

@rzikm rzikm closed this Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@kotlarmilos kotlarmilos kotlarmilos approved these changes
@rzikm rzikm Awaiting requested review from rzikm
@matouskozak matouskozak Awaiting requested review from matouskozak

Assignees

@rzikm rzikm

Copilot code review Copilot

Labels

area-System.Net.Security

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[tvOS CoreCLR] System.Net.Security.Unit.Tests crashes because required Dictionary IL was stripped from R2R image

7 participants

@matouskozak @davidnguyen-tech @BrzVlad @rzikm @kotlarmilos