Skip to content

fix: avoid race condition crash in [RCTDataRequestHandler invalidate]#2383

Merged
Saadnajmi merged 2 commits intomicrosoft:mainfrom
Saadnajmi:fix-crash
Feb 27, 2025
Merged

fix: avoid race condition crash in [RCTDataRequestHandler invalidate]#2383
Saadnajmi merged 2 commits intomicrosoft:mainfrom
Saadnajmi:fix-crash

Conversation

@Saadnajmi
Copy link
Copy Markdown
Collaborator

Summary:

Upstreaming a fix by @ntre that fixes a crash we saw internally related to [_queue cancelAllOperations].

Calling [_queue cancelAllOperations] will release all references to any active operations.
If the blocks of those operations have a reference to itself, it will result in dangling pointers, which could conceptually trigger a later crash if there's a race between the operation completing and it being pulled out of the queue.

Add explicit strong reference while block is running.
For good measure, fix same pattern also in RCTFileRequestHandler.

Note: separately, that this code is passing the op itself as a requestToken to [delegate URLRequest:] methods is suspect. That delegate can retain said token.

Test Plan:

Tested internally.

@Saadnajmi Saadnajmi enabled auto-merge (squash) February 27, 2025 08:43
Saadnajmi added a commit that referenced this pull request Feb 27, 2025
@Saadnajmi Saadnajmi merged commit c541c8c into microsoft:main Feb 27, 2025
12 checks passed
Saadnajmi added a commit that referenced this pull request Feb 27, 2025
Saadnajmi added a commit that referenced this pull request Feb 27, 2025
Saadnajmi added a commit that referenced this pull request Feb 27, 2025
Saadnajmi added a commit that referenced this pull request Apr 8, 2025
…aRequestHandler invalidate]" (#2450)

Cherry pick facebook#50342 , which
fixes a bug with our fix from #2383

I chose to omit the macOS tags as this is a cherry-pick.

Co-authored-by: zhongwuzw <zhongwuzw@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants