Skip to content
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

[release/7.0] Make sure s_currentGenerationTable is safe for profiler attach #78937

Merged
merged 1 commit into from
Nov 29, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 28, 2022

Backport of #78841 to release/7.0

/cc @cshung

Customer Impact

Customer reported crash on profiler attach. If we hit this sequence of events, then the profiled .NET process would crash.

  • .NET Process Launched,
  • An ICorProfiler implementation that is interested in GC events is attached later that
  • An allocation happened.
  • The GC needs to create a new region, then
  • The process would crash.

Testing

The diagnostics team will run the profiler-specific tests on this commit.

Risk

This change does not impact the profiler launch scenario, and nothing can be worse than a crash in the profiler attach scenario.

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Nov 29, 2022

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #78841 to release/7.0

/cc @cshung

Customer Impact

If we hit this sequence of events, then the profiled .NET process would crash.

  • .NET Process Launched,
  • An ICorProfiler implementation that is interested in GC events is attached later that
  • An allocation happened.
  • The GC needs to create a new region, then
  • The process would crash.

Testing

The diagnostics team will run the profiler-specific tests on this commit.

Risk

This change does not impact the profiler launch scenario, and nothing can be worse than a crash in the profiler attach scenario.

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-Diagnostics-coreclr

Milestone: -

Copy link
Member

@tommcdon tommcdon left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks! This is an important diagnostics scenario and the fix looks low risk.

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. we will take for consideration in 7.0.x

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Nov 29, 2022
@jeffschwMSFT jeffschwMSFT added this to the 7.0.x milestone Nov 29, 2022
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Nov 29, 2022
@rbhanda rbhanda modified the milestones: 7.0.x, 7.0.3 Nov 29, 2022
@tommcdon tommcdon modified the milestones: 7.0.3, 7.0.2, 7.0.x Nov 29, 2022
@jeffschwMSFT jeffschwMSFT modified the milestones: 7.0.x, 7.0.2 Nov 29, 2022
@carlossanlop
Copy link
Member

Branding has been done. Final chosen milestone is 7.0.2. Signed-off by area owners. Approved by Tactics. No OOB package authoring changes needed. CI is green.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 44e68e8 into release/7.0 Nov 29, 2022
@carlossanlop carlossanlop deleted the backport/pr-78841-to-release/7.0 branch November 29, 2022 23:25
@ghost ghost locked as resolved and limited conversation to collaborators Dec 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants