From d4efc4f2aef0065f4238cc9b224eae807588ab62 Mon Sep 17 00:00:00 2001 From: Roxie Linden Date: Wed, 12 Aug 2026 14:29:29 -0700 Subject: p#682 Fix shutdown crash and cut WebRTC teardown time terminate() released the thread unique_ptrs on the timeout path while the detached shutdown thread was still running the lambda that reads them, so a slow shutdown became a null deref at the next BlockingCall. Leave the members alone and report the timeout instead, so the caller leaks this object rather than deleting it out from under the detached thread. Reduce how often that timeout is reached at all: - Post the audio device stop instead of blocking on it. StopRecording and StopPlayout each wait up to 2s on a WASAPI thread, spending much of the 10s budget before the connections were touched. The device stop has to stay on the worker thread (AudioDeviceBuffer is guarded by a sequence checker bound to it), but the queue is FIFO, so ForceTerminate still runs after it while the signaling close gets on with its network-thread work. - Drain the peer connections in LLWebRTCVoiceClient::terminate() before handing off to llwebrtc::terminate(), bounded to 3s. An estate session can hold ten live connections -- the current region plus up to eight neighbours -- and closing them inline and serially under the shutdown budget is what made it tight. Draining also quiets the stats poll, so PeerConnection::Close() no longer blocks in WaitForPendingRequest() on an in-flight GetStats. Connections reaped by the drain are destroyed while webrtc is still fully alive, so they must unregister and release their peer connections normally; key the destructor's early-out off a new sWebRTCTerminated instead of isShuttingDown(). Notify signaling observers even when there was no peer connection to close, or a connection shut down before it finished initializing parks in VOICE_STATE_WAIT_FOR_CLOSE, which has no timeout of its own. Co-Authored-By: Claude Opus 5 (1M context) --- indra/llwebrtc/llwebrtc.cpp | 80 +++++++++++++++++++------- indra/llwebrtc/llwebrtc_impl.h | 6 +- indra/newview/llvoicewebrtc.cpp | 124 ++++++++++++++++++++++++++++++++++++---- indra/newview/llvoicewebrtc.h | 20 +++++++ 4 files changed, 198 insertions(+), 32 deletions(-) diff --git a/indra/llwebrtc/llwebrtc.cpp b/indra/llwebrtc/llwebrtc.cpp index bdb629172d..3b5d338896 100644 --- a/indra/llwebrtc/llwebrtc.cpp +++ b/indra/llwebrtc/llwebrtc.cpp @@ -396,7 +396,7 @@ void LLWebRTCImpl::init() } -void LLWebRTCImpl::terminate() +bool LLWebRTCImpl::terminate() { // Run all blocking WebRTC shutdown calls on a separate thread so that a // hung BlockingCall cannot block the viewer shutdown indefinitely. @@ -413,7 +413,27 @@ void LLWebRTCImpl::terminate() std::thread shutdown_thread( [this, connections = std::move(connections), done_promise]() mutable { - mWorkerThread->BlockingCall( + // Stop the capture/render devices alongside the connection teardown + // below rather than ahead of it. Both of these calls end in a + // WaitForSingleObject on a WASAPI thread with a 2s timeout apiece + // (AudioDeviceWindowsCore::StopRecording / StopPlayout), so blocking on + // them here can spend most of the shutdown budget before the + // connections have been touched at all -- and after an OS sleep they + // tend to hit the full timeout. + // + // This work has to stay on the worker thread: the device module was + // created there and its AudioDeviceBuffer is guarded by a sequence + // checker bound to that thread. Posting instead of blocking lets the + // signaling close below get on with its network-thread work (data + // channel close, transport teardown) while the device stop is still + // waiting on WASAPI. + // + // No explicit join is needed: Thread's task queue is FIFO and + // BlockingCall posts through it, so the ForceTerminate call at the end + // of this lambda can't run until this task has finished. Any + // worker-thread work the signaling close does is likewise ordered + // after it, so nothing sees the device module half torn down. + mWorkerThread->PostTask( [this]() { if (mDeviceModule) @@ -474,18 +494,23 @@ void LLWebRTCImpl::terminate() " Detaching — some WebRTC resources will be leaked."; shutdown_thread.detach(); - // Release the unique_ptrs WITHOUT joining/deleting: the detached thread - // may still be using these thread objects. - // The raw pointers are intentionally leaked — the process is exiting anyway - // and our priority is saving cache and personal data. - (void)mNetworkThread.release(); - (void)mWorkerThread.release(); - (void)mSignalingThread.release(); - + // Leave every member exactly as it is. The detached thread is still + // running the lambda above, which reads mSignalingThread, mWorkerThread, + // mDeviceModule and mPeerConnectionFactory through `this` -- clearing or + // releasing them here would pull them out from under it mid-shutdown + // (a null mSignalingThread is an immediate segfault at the next + // BlockingCall). Instead we report the failure so the caller leaks this + // object rather than deleting it; the process is exiting anyway and our + // priority is saving cache and personal data. + // // mPeerConnections is already empty -- the detached thread owns the // connections now and must be left to finish with them. + // + // The log sink is unhooked here (and deliberately not deleted, since the + // detached thread may still log) because the viewer-side log callback + // behind it doesn't outlive this call. webrtc::LogMessage::RemoveLogToStream(mLogSink); - return; + return false; } shutdown_thread.join(); @@ -497,6 +522,7 @@ void LLWebRTCImpl::terminate() mSignalingThread = nullptr; webrtc::LogMessage::RemoveLogToStream(mLogSink); + return true; } @@ -1124,17 +1150,23 @@ void LLWebRTCPeerConnectionImpl::closeOnSignalingThread() mLocalStream = nullptr; } mPeerConnection = nullptr; + } - for (auto &observer : mSignalingObserverList) - { - observer->OnPeerConnectionClosed(); - } + // Notify unconditionally, even if there was no peer connection to close -- + // a connection can be shut down before it ever finished initializing, and + // the caller is still waiting to hear that the close is done. Withholding + // this leaves the viewer's connection state machine parked in + // VOICE_STATE_WAIT_FOR_CLOSE, which has no timeout of its own. + for (auto &observer : mSignalingObserverList) + { + observer->OnPeerConnectionClosed(); } - // Nothing may call back into the viewer past this point. On shutdown the - // viewer's connection objects are torn down as soon as llwebrtc::terminate() - // returns and they deliberately don't unset themselves as observers, so any - // late callback would be reaching into freed memory. + // Nothing may call back into the viewer past this point. Connections + // closed while the viewer is still running unset themselves as observers + // when they're destroyed, but any that are left for llwebrtc::terminate() + // to close deliberately don't -- they're torn down as soon as it returns, + // so a late callback would be reaching into freed memory. mSignalingObserverList.clear(); mDataObserverList.clear(); } @@ -1931,8 +1963,14 @@ void terminate() { if (gWebRTCImpl) { - gWebRTCImpl->terminate(); - delete gWebRTCImpl; + if (gWebRTCImpl->terminate()) + { + delete gWebRTCImpl; + } + // Otherwise shutdown timed out and was left to a detached thread that is + // still using this object -- and the webrtc threads it owns -- so it's + // intentionally leaked. Deleting it would hand that thread a freed + // object to finish shutting down with. gWebRTCImpl = nullptr; } } diff --git a/indra/llwebrtc/llwebrtc_impl.h b/indra/llwebrtc/llwebrtc_impl.h index cee42cf19a..8d1288c87b 100644 --- a/indra/llwebrtc/llwebrtc_impl.h +++ b/indra/llwebrtc/llwebrtc_impl.h @@ -413,7 +413,11 @@ class LLWebRTCImpl : public LLWebRTCDeviceInterface, public webrtc::AudioDeviceO } void init(); - void terminate(); + // Returns true if shutdown completed cleanly and this object may be + // destroyed. Returns false if it timed out: a detached thread is still + // using this object and its webrtc threads, so it must be leaked, not + // deleted. + bool terminate(); // // LLWebRTCDeviceInterface diff --git a/indra/newview/llvoicewebrtc.cpp b/indra/newview/llvoicewebrtc.cpp index 126d22924b..476ab18338 100644 --- a/indra/newview/llvoicewebrtc.cpp +++ b/indra/newview/llvoicewebrtc.cpp @@ -208,6 +208,7 @@ LLSD LLVoiceWebRTCStats::read() /////////////////////////////////////////////////////////////////////////////////////////////// bool LLWebRTCVoiceClient::sShuttingDown = false; +bool LLWebRTCVoiceClient::sWebRTCTerminated = false; LLWebRTCVoiceClient::LLWebRTCVoiceClient() : mHidden(false), @@ -233,6 +234,7 @@ LLWebRTCVoiceClient::LLWebRTCVoiceClient() : mWebRTCDeviceInterface(nullptr) { sShuttingDown = false; + sWebRTCTerminated = false; mSpeakerVolume = 0.0; @@ -301,11 +303,81 @@ void LLWebRTCVoiceClient::terminate() mVoiceEnabled = false; sShuttingDown = true; // so that coroutines won't post more work. + + drainConnections(); + + sWebRTCTerminated = true; llwebrtc::terminate(); mWebRTCDeviceInterface = nullptr; } +// Close the live peer connections before handing control to +// llwebrtc::terminate(). +// +// Anything still open when terminate() runs gets closed inline and serially on +// the signaling thread, under a single 10s budget that is also paying for the +// audio device shutdown. An estate session can hold ten live connections -- +// the current region plus up to eight neighbours, plus any group or ad-hoc +// session -- each needing a full DTLS/SCTP teardown, so that budget is not +// generous. Closing them here lets the teardown proceed asynchronously on the +// signaling thread while this thread keeps pumping. +// +// It also quiets the per-connection stats poll before terminate() runs. A +// GetStats request left in flight makes PeerConnection::Close() block in +// RTCStatsCollector::WaitForPendingRequest(), which waits on the network thread +// with no timeout at all. +// +// Best effort: whatever hasn't closed by the deadline is left to +// llwebrtc::terminate(), exactly as before. +void LLWebRTCVoiceClient::drainConnections() +{ + // Marks every session and connection as shutting down. This also stops + // estateSessionState::processConnectionStates() from spinning up + // replacement connections to neighbouring regions while we drain. + sessionState::for_each(boost::bind(predShutdownSession, _1)); + + // Long enough for a local close to complete, short enough that a wedged + // connection doesn't noticeably delay quitting. The remaining budget in + // llwebrtc::terminate() is the real backstop. + constexpr F32 DRAIN_TIMEOUT_SECONDS = 3.0f; + constexpr U32 DRAIN_POLL_MS = 10; + + // Wait on the peer connections being closed rather than on the sessions + // being reaped. A connection that still has an HTTP coroutine in flight + // holds its session alive until mOutstandingRequests unwinds, and those + // coroutines don't run from here -- but its peer connection has already + // been closed by then, which is all terminate() cares about. + LLTimer timer; + while (!sessionState::allSessionsClosed() && timer.getElapsedTimeF32() < DRAIN_TIMEOUT_SECONDS) + { + // OnPeerConnectionClosed comes back through the main queue, so it has + // to be pumped or the state machines never see connections finish. + if (auto main_queue = mMainQueue.lock()) + { + main_queue->runFor(std::chrono::milliseconds(DRAIN_POLL_MS)); + } + sessionState::processSessionStates(); + + if (!sessionState::allSessionsClosed()) + { + ms_sleep(DRAIN_POLL_MS); + } + } + + if (!sessionState::allSessionsClosed()) + { + LL_WARNS("Voice") << "Timed out draining voice connections after " + << DRAIN_TIMEOUT_SECONDS + << "s; leaving the rest to llwebrtc::terminate()." << LL_ENDL; + } + else + { + LL_INFOS("Voice") << "Voice connections drained in " + << timer.getElapsedTimeF32() << "s." << LL_ENDL; + } +} + //--------------------------------------------------- void LLWebRTCVoiceClient::cleanUp() @@ -1890,6 +1962,7 @@ void LLWebRTCVoiceClient::userAuthorized(const std::string& user_id, const LLUUI if (sShuttingDown) { sShuttingDown = false; // was terminated, restart + sWebRTCTerminated = false; initWebRTC(); } } @@ -2173,6 +2246,30 @@ void LLWebRTCVoiceClient::sessionState::processSessionStates() } } +bool LLWebRTCVoiceClient::sessionState::allConnectionsClosed() const +{ + for (const auto &connection : mWebRTCConnections) + { + if (!connection->isClosed()) + { + return false; + } + } + return true; +} + +bool LLWebRTCVoiceClient::sessionState::allSessionsClosed() +{ + for (const auto &session : sSessions) + { + if (session.second && !session.second->allConnectionsClosed()) + { + return false; + } + } + return true; +} + // process the states on each connection associated with a session. bool LLWebRTCVoiceClient::sessionState::processConnectionStates() { @@ -2425,12 +2522,18 @@ LLVoiceWebRTCConnection::LLVoiceWebRTCConnection(const LLUUID ®ionID, const s LLVoiceWebRTCConnection::~LLVoiceWebRTCConnection() { - if (LLWebRTCVoiceClient::isShuttingDown()) + if (LLWebRTCVoiceClient::isWebRTCTerminated()) { - // peer connection and observers will be cleaned up - // by llwebrtc::terminate() on shutdown. + // peer connection and observers have already been cleaned up + // by llwebrtc::terminate(). return; } + // Note this is deliberately keyed off isWebRTCTerminated() rather than + // isShuttingDown(): connections drained by drainConnections() are destroyed + // while the webrtc library is still fully alive, and must unregister + // themselves and release the peer connection like any other close. Leaving + // a freed observer registered would hand llwebrtc::terminate() a dangling + // pointer to call OnPeerConnectionClosed() on. mWebRTCPeerConnectionInterface->unsetSignalingObserver(this); llwebrtc::freePeerConnection(mWebRTCPeerConnectionInterface); } @@ -3092,8 +3195,10 @@ bool LLVoiceWebRTCConnection::connectionStateMachine() } else { - // llwebrtc::terminate() is already shuting down the connection. - setVoiceConnectionState(VOICE_STATE_WAIT_FOR_CLOSE); + // Shutting down: skip the courtesy logout to the sim (the HTTP + // round trip would just delay quitting) and go straight to + // dropping the webrtc connection. + setVoiceConnectionState(VOICE_STATE_SESSION_EXIT); } break; @@ -3104,11 +3209,10 @@ bool LLVoiceWebRTCConnection::connectionStateMachine() { setVoiceConnectionState(VOICE_STATE_WAIT_FOR_CLOSE); mOutstandingRequests++; - if (!LLWebRTCVoiceClient::isShuttingDown()) - { - mWebRTCPeerConnectionInterface->shutdownConnection(); - } - // else was already posted by llwebrtc::terminate(). + // Always drop the connection ourselves, including during shutdown: + // drainConnections() runs before llwebrtc::terminate(), so nothing + // else has posted the close yet. + mWebRTCPeerConnectionInterface->shutdownConnection(); break; } diff --git a/indra/newview/llvoicewebrtc.h b/indra/newview/llvoicewebrtc.h index 818bce5bc3..ec01b8c08c 100644 --- a/indra/newview/llvoicewebrtc.h +++ b/indra/newview/llvoicewebrtc.h @@ -79,6 +79,12 @@ public: static bool isShuttingDown() { return sShuttingDown; } + // True once llwebrtc::terminate() has been entered. Between + // isShuttingDown() and this, the webrtc library is still fully alive and + // connections must still release their peer connections normally -- see + // drainConnections() and ~LLVoiceWebRTCConnection(). + static bool isWebRTCTerminated() { return sWebRTCTerminated; } + const LLVoiceVersionInfo& getVersion() override; void updateVersion(); @@ -306,6 +312,9 @@ public: bool isEmpty() { return mWebRTCConnections.empty(); } + bool allConnectionsClosed() const; + static bool allSessionsClosed(); + virtual bool isSpatial() = 0; virtual bool isEstate() = 0; virtual bool isCallbackPossible() = 0; @@ -455,6 +464,10 @@ private: /// Clean up objects created during a voice session. void cleanUp(); + /// Close the live peer connections before handing off to + /// llwebrtc::terminate(). Bounded and best effort. + void drainConnections(); + LL::WorkQueue::weak_t mMainQueue; F32 mTuningMicGain; @@ -539,6 +552,7 @@ private: // These variables can last longer than WebRTC in coroutines so we need them as static static bool sShuttingDown; + static bool sWebRTCTerminated; LLEventMailDrop mWebRTCPump; @@ -643,6 +657,12 @@ class LLVoiceWebRTCConnection : return mShutDown; } + // True once the webrtc peer connection has finished closing. The + // connection object can outlive this while it waits for outstanding + // requests to unwind, so this -- not reaping -- is what drainConnections() + // waits on. + bool isClosed() const { return mVoiceConnectionState == VOICE_STATE_CLOSED; } + void OnVoiceConnectionRequestSuccess(const LLSD &body); void resetConnectionStats(); -- cgit v1.3