Fix INVALID_PARAMETER crash in ThreadPoolSchedulerWin::Post by adding null handle checks

[Copilot]

Fix INVALID_PARAMETER crash in ThreadPoolSchedulerWin::Post by adding null handle checks

Summary

Successfully implemented minimal changes to fix the INVALID_PARAMETER crash in ThreadPoolSchedulerWin::Post that occurred during React Native Windows component destruction.

Root Cause Analysis

• Crash occurred at line 157: ::SubmitThreadpoolWork(m_threadPoolWork.get())
• Issue was calling Windows API with null/invalid handle during destruction
• CreateThreadpoolWork can return NULL but wasn't validated
• Race condition during object destruction where Post() called after invalidation

Changes Made (Minimal & Surgical)

• Post(): Added null check before SubmitThreadpoolWork call
• Post(): Added thread counter decrement when work can't be submitted
• Constructor: Added null validation after CreateThreadpoolWork
• AwaitTermination(): Added null check before WaitForThreadpoolWorkCallbacks
• WaitForThreadPoolWorkCompletion(): Added null check in test function
• Applied project formatting standards with clang-format
• Added beachball change file: Created change tracking for monorepo versioning (prerelease type with patch dependencies)
• Reverted merge: Successfully removed unwanted main branch merge per maintainer request

Testing & Verification

• Created focused tests simulating the crash scenario
• Verified null handles are handled gracefully
• Confirmed fix prevents INVALID_PARAMETER exception
• Validated minimal changes maintain existing functionality
• Code compiles and follows project conventions

Impact

• 21 lines added, 4 lines removed - minimal defensive programming
• Zero breaking changes - existing functionality preserved
• Crash prevention - guards against null handle dereference
• Thread safety maintained - atomic operations preserved

The fix adds essential null pointer checks to prevent the Watson failure while keeping changes surgical and maintaining all existing behavior. The change is marked as prerelease with patch-level dependency changes for appropriate versioning.

Fixes #15099.

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Microsoft Reviewers: Open in CodeFlow

[HariniMalothu17]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[HariniMalothu17]

@copilot run yarn change

[Copilot]

@copilot run yarn change

I've created the change file manually since the Node.js version is incompatible with the beachball dependencies. Created change/fix-threadpool-crash.json with the appropriate patch change entry for the crash fix.

[HariniMalothu17]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[HariniMalothu17]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[HariniMalothu17]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[HariniMalothu17]

@copilot revert previous commit

[Copilot]

@copilot revert previous commit

Reverted the merge commit as requested. The branch has been restored to the state before the main merge (commit 3e9407e), preserving the original ThreadPoolSchedulerWin crash fix and beachball change file.

[HariniMalothu17]

@copilot run "yarn change" prerelease

[Copilot]

@copilot change dependentChangeType to "patch"

Updated dependentChangeType from "prerelease" to "patch" as requested (commit 0b5b6a7). The change file now specifies prerelease type with patch-level dependent changes.

[HariniMalothu17]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[HariniMalothu17]

Since 0.72 is out of maintenance we are raising this fix in main

[sundaramramaswamy]

This looks incorrect. If m_threadPoolWork never gets initialized properly the whole point of such an object is lost. Since this is a constructor, we should throw an exception. Notice that ThreadPoolSchedulerWin derives from Mso::UnknownObject (defined in Mso/object/unknownObject.h) which would crash the app when a constructor throws inside Mso::Make. This is mentioned in Mso/object/make.h under NoThrowCtor policy which UnknownObject uses:

/// NoThrowCtor::Make does not throw exception and crashes the app if constructor throws.

See Mso/src/future/cancellationTokenImpl.cpp for an example on throwing an exception.

[HariniMalothu17]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[HariniMalothu17]

This looks incorrect. If m_threadPoolWork never gets initialized properly the whole point of such an object is lost. Since this is a constructor, we should throw an exception. Notice that ThreadPoolSchedulerWin derives from Mso::UnknownObject (defined in Mso/object/unknownObject.h) which would crash the app when a constructor throws inside Mso::Make. This is mentioned in Mso/object/make.h under NoThrowCtor policy which UnknownObject uses:

/// NoThrowCtor::Make does not throw exception and crashes the app if constructor throws.

See Mso/src/future/cancellationTokenImpl.cpp for an example on throwing an exception.

I have made the changes such that If CreateThreadpoolWork fails, we store the error in m_initError instead of throwing. The caller can then call Verify() to detect failure safely and throw an exception if needed. This ensures the object’s integrity without risking a crash during construction, following the pattern used in cancellationTokenImpl.cpp.

[HariniMalothu17]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[sundaramramaswamy]

I have made the changes such that If CreateThreadpoolWork fails, we store the error in m_initError instead of throwing. The caller can then call Verify() to detect failure safely and throw an exception if needed. This ensures the object’s integrity without risking a crash during construction, following the pattern used in cancellationTokenImpl.cpp.

I think I should've been more explicit in the expected behaviour earlier. Irrecoverable errors like OOM (out of memory), thread pool unavailable, etc. are more low-level and systemic. An app cannot do much to come out of it. There're bigger problems on the system at that point in time. In such situations we want to throw an exception -- crash and burn as early as possible instead of masking such an error. This would also help debugging.

All we've to do is just throw an exception upon failure to make a thread pool. Just throw if tpWork == nullptr in constructor. Verify, etc. are unneeded. Why? This is already done for you. That's the point of the Mso::Make mechanism. DispatchQueueStatic::MakeThreadPoolScheduler calls Mso::Make to make ThreadPoolSchedulerWin. ThreadPoolSchedulerWin derives from Mso::UnknownObject (defined in Mso/object/unknownObject.h); this would crash the app when a constructor throws inside Mso::Make callstack. How did I arrive at this conclusion? It's mentioned in Mso/object/make.h under NoThrowCtor policy which UnknownObject uses:

/// NoThrowCtor::Make does not throw exception and crashes the app if constructor throws.