DRTVWR-476, SL-12204: Fix crash in Marketplace Listings.

The observed crash was due to sharing a stateful global resource (the global
LLMessageSystem instance) between different tasks. Specifically, a coroutine
sets its mMessageReader one way, expecting that value to persist until it's
done with message parsing, but another coroutine sneaks in at a suspension
point and sets it differently.

Introduce LockMessageReader and LockMessageChecker classes, which must be
instantiated by a consumer of the resource. The constructor of each locks a
coroutine-aware mutex, so that for the lifetime of the lock object no other
coroutine can instantiate another.

Refactor the code so that LLMessageSystem::mMessageReader can only be modified
by LockMessageReader, not by direct assignment. mMessageReader is now an
instance of LLMessageReaderPointer, which supports dereferencing and
comparison but not assignment. Only LockMessageReader can change its value.

LockMessageReader addresses the use case in which the specific mMessageReader
value need only persist for the duration of a single method call. Add an
instance in LLMessageHandlerBridge::post().

LockMessageChecker is a subclass of LockMessageReader: both lock the same
mutex. LockMessageChecker addresses the use case in which the specific
mMessageReader value must persist across multiple method calls. Modify the
methods in question to require a LockMessageChecker instance. Provide
LockMessageChecker forwarding methods to facilitate calling the underlying
LLMessageSystem methods via the LockMessageChecker instance.

Add LockMessageChecker instances to LLAppViewer::idleNetwork(), a couple cases
in idle_startup() and LLMessageSystem::establishBidirectionalTrust().

1 files changed, 26 insertions, 23 deletions

diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp
index 65bfcec8c4..70d3c10524 100644
--- a/indra/newview/llappviewer.cpp
+++ b/indra/newview/llappviewer.cpp
@@ -5250,37 +5250,40 @@ void LLAppViewer::idleNetwork()
 		const S64 frame_count = gFrameCount;  // U32->S64
 		F32 total_time = 0.0f;
 
-		while (gMessageSystem->checkAllMessages(frame_count, gServicePump))
 		{
-			if (gDoDisconnect)
+			LockMessageChecker lmc(gMessageSystem);
+			while (lmc.checkAllMessages(frame_count, gServicePump))
 			{
-				// We're disconnecting, don't process any more messages from the server
-				// We're usually disconnecting due to either network corruption or a
-				// server going down, so this is OK.
-				break;
-			}
+				if (gDoDisconnect)
+				{
+					// We're disconnecting, don't process any more messages from the server
+					// We're usually disconnecting due to either network corruption or a
+					// server going down, so this is OK.
+					break;
+				}
 
-			total_decoded++;
-			gPacketsIn++;
+				total_decoded++;
+				gPacketsIn++;
 
-			if (total_decoded > MESSAGE_MAX_PER_FRAME)
-			{
-				break;
-			}
+				if (total_decoded > MESSAGE_MAX_PER_FRAME)
+				{
+					break;
+				}
 
 #ifdef TIME_THROTTLE_MESSAGES
-			// Prevent slow packets from completely destroying the frame rate.
-			// This usually happens due to clumps of avatars taking huge amount
-			// of network processing time (which needs to be fixed, but this is
-			// a good limit anyway).
-			total_time = check_message_timer.getElapsedTimeF32();
-			if (total_time >= CheckMessagesMaxTime)
-				break;
+				// Prevent slow packets from completely destroying the frame rate.
+				// This usually happens due to clumps of avatars taking huge amount
+				// of network processing time (which needs to be fixed, but this is
+				// a good limit anyway).
+				total_time = check_message_timer.getElapsedTimeF32();
+				if (total_time >= CheckMessagesMaxTime)
+					break;
 #endif
-		}
+			}
 
-		// Handle per-frame message system processing.
-		gMessageSystem->processAcks(gSavedSettings.getF32("AckCollectTime"));
+			// Handle per-frame message system processing.
+			lmc.processAcks(gSavedSettings.getF32("AckCollectTime"));
+		}
 
 #ifdef TIME_THROTTLE_MESSAGES
 		if (total_time >= CheckMessagesMaxTime)