Application.ThreadContext.FromId reads from s_contextHash while another thread may be writing

[KalleOlaviNiemitalo]

.NET version

9.0.3

Did it work in .NET Framework?

Yes

Did it work in any of the earlier releases of .NET Core or .NET 5+?

I guess it would have worked in .NET 7, because #8161 was not merged there.

Issue description

The Application.ThreadContext.FromId method reads from the s_contextHash dictionary without locking. This can cause an error if another thread writes to the dictionary at the same time.

winforms/src/System.Windows.Forms/src/System/Windows/Forms/Application.ThreadContext.cs

Lines 513 to 521 in 5bf1bff

	internal static ThreadContext? FromId(uint id)
	{
	if (!s_contextHash.TryGetValue(id, out ThreadContext? context) && id == PInvoke.GetCurrentThreadId())
	{
	context = Create();
	}
	return context;
	}

Writers do lock (s_tcInternalSyncObject) but this reader doesn't. AFAICT, the callers of Application.ThreadContext.FromId don't lock it either.

Before #8161, s_contextHash used to be a Hashtable, which is safe for one writer in parallel with multiple readers. Dictionary<TKey, TValue> is not safe for that use.

Steps to reproduce

Found by source code inspection, not reproduced.

[Tanya-Solyanik]

@Olina-Zhang - could your team please work on a unit test or a ScratchProject repro that exercises the relevant code for this issue. Please share your private branch when done.

[Olina-Zhang]

@Tanya-Solyanik Tried to repro with that relevant code from the Application.ThreadContext.FromId method, but cannot, no any error after simulating a race condition between a reader and a writer.
here is my sample app: TestSample.zip
@KalleOlaviNiemitalo Can you give us some support how to reproduce this issue? Thanks!

[KalleOlaviNiemitalo]

@Olina-Zhang why does your sample app have private static readonly ConcurrentDictionary<uint, ThreadContext> s_contextHash?

On the main branch, s_contextHash is a Dictionary<uint, ThreadContext> rather than a ConcurrentDictionary<uint, ThreadContext>:

winforms/src/System.Windows.Forms/System/Windows/Forms/Application.ThreadContext.cs

Line 25 in 8ae5137

private static readonly Dictionary<uint, ThreadContext> s_contextHash = [];

Likewise in v9.0.3:

winforms/src/System.Windows.Forms/src/System/Windows/Forms/Application.ThreadContext.cs

Line 25 in 5bf1bff

private static readonly Dictionary<uint, ThreadContext> s_contextHash = [];

[KalleOlaviNiemitalo]

I mean, ConcurrentDictionary may be a good way to fix the bug, but you cannot reproduce the error if you use that in the sample app.

[JeremyKuhne]

@KalleOlaviNiemitalo are you hitting this? Are you willing to propose a fix PR? If not, I'll look into fixing this when I've got a few cycles.

@Olina-Zhang don't worry about a repro here.

[KalleOlaviNiemitalo]

No I am not hitting this. And won't make a PR.

[JeremyKuhne]

No I am not hitting this. And won't make a PR.

Thanks for the clarification. I'll take care of it when I can.