Fix pure virtual call crash in BackgroundTransportCurl destructor#51
Draft
mliberman wants to merge 1 commit intocompnerd/swiftfrom
Draft
Fix pure virtual call crash in BackgroundTransportCurl destructor#51mliberman wants to merge 1 commit intocompnerd/swiftfrom
BackgroundTransportCurl destructor#51mliberman wants to merge 1 commit intocompnerd/swiftfrom
Conversation
In the async path, SignalTransferComplete() was called before CompleteOperation(). This unblocked the waiting thread (via WaitForAllTransfersToComplete), allowing it to destroy the Request and Response objects. When CompleteOperation() then called request_->MarkCompleted() / response_->MarkFailed(), these were dangling pointers whose vtables had been overwritten during destruction, causing _purecall -> abort(). The fix is to always call CompleteOperation() before SignalTransferComplete(), which is what the synchronous path already did. This ensures request_/response_ are still alive when their virtual methods are called. The old comment worried that MarkCompleted/MarkFailed "could end up attempting to tear down TransportCurl", but these methods only set a boolean flag and cannot trigger destruction. Even if the complete_ callback somehow triggered TransportCurl teardown, ~TransportCurl calls WaitForAllTransfersToComplete() which would block until SignalTransferComplete() is called, so the ordering is self-protecting. This crash manifests as BROWSER-WIN-2SRG in Sentry with 19k+ events: EXCEPTION_ACCESS_VIOLATION_WRITE via _purecall in BackgroundTransportCurl::~BackgroundTransportCurl.
mliberman
changed the title
BackgroundTransportCurl destructor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a race condition in the curl HTTP transport destructor that causes a crash in Arc on Windows (BROWSER-WIN-2SRG).
Context
When an async HTTP request finishes,
~BackgroundTransportCurldoes two things: signals the calling thread that it can proceed (SignalTransferComplete()), and marks the request/response as completed (CompleteOperation()).The problem is the async path does these in the wrong order.
SignalTransferComplete()fires first, which wakes up the calling thread (blocked inWaitForAllTransfersToComplete()), and it immediately starts destroying the request and response objects. ThenCompleteOperation()tries to callMarkCompleted()on an object that's mid-destruction; the vtable has already been rewound to the base class (Transfer) whereMarkCompleted()is pure virtual, so we get a_purecallcrash.The synchronous path already had the correct order; the async path had a different order because of a concern (noted in a comment) that
CompleteOperation()might somehow trigger destruction of theTransportCurlthat owns the signal. But this isn't actually possible:MarkCompleted()andMarkFailed()just set a boolean flag (completed_ = true). And even if thecomplete_callback did somehow triggerTransportCurldestruction,~TransportCurlcallsWaitForAllTransfersToComplete()which blocks on the semaphore, so it would deadlock waiting for theSignalTransferComplete()that hasn't happened yet.Changes
~BackgroundTransportCurlto callCompleteOperation()beforeSignalTransferComplete(), matching the synchronous path.Testing
I can't repro this crash, so this fix is slightly speculative.