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 9b4a371cec..c82a005a77 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 &regionID, 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();