Avoid dead lock in LLQueuedThread::generateHandle / LLTextureFetchWorker::callbackDecoded

Thead 1:

indra/llcommon/llqueuedthread.cpp:456:

452                     if (complete)
453                     {
454                             lockData();             // This locks LLThread::mRunCondition
455                             req->setStatus(STATUS_COMPLETE);
456                             req->finishRequest(true);

LLImageDecodeThread::ImageRequest::finishRequest calls:
    mResponder->completed(success, mDecodedImageRaw, mDecodedImageAux);

LLTextureFetchWorker::DecodeResponder::completed calls:
    worker->callbackDecoded(success, raw, aux);

LLTextureFetchWorker::callbackDecoded calls:
    LLMutexLock lock(&mWorkMutex);                      // This locks LLTextureFetchWorker::mWorkMutex

Thread 2:

LLTextureFetchWorker::doWork calls:
    LLMutexLock lock(&mWorkMutex);                      // This locks LLTextureFetchWorker::mWorkMutex
    .
    .
    .
    mDecodeHandle = mFetcher->mImageDecodeThread->decodeImage(mFormattedImage, image_priority, discard, mNeedsAux, new DecodeResponder(mFetcher, mID, this));

LLImageDecodeThread::decodeImage calls:
    handle_t handle = generateHandle();

LLQueuedThread::generateHandle calls:
    lockData();                                         // This locks LLThread::mRunCondition

indra/llcommon/llqueuedthread.cpp

@@ -264,7 +264,7 @@ bool LLQueuedThread::waitForResult(LLQueuedThread::handle_t handle, bool auto_co
 		{
 			done = true; // request does not exist
 		}
-		else if (req->getStatus() == STATUS_COMPLETE)
+		else if (req->getStatus() == STATUS_COMPLETE && !(req->getFlags() & FLAG_LOCKED))
 		{
 			res = true;
 			if (auto_complete)
@@ -372,9 +372,17 @@ bool LLQueuedThread::completeRequest(handle_t handle)
 #if _DEBUG
 // 		llinfos << llformat("LLQueuedThread::Completed req [%08d]",handle) << llendl;
 #endif
-		mRequestHash.erase(handle);
+		if (!(req->getFlags() & FLAG_LOCKED))
-		req->deleteRequest();
+		{
-// 		check();
+			mRequestHash.erase(handle);
+			req->deleteRequest();
+// 			check();
+ 		}
+ 		else
+ 		{
+			// Cause deletion immediately after FLAG_LOCKED is released.
+ 			req->setFlags(FLAG_AUTO_COMPLETE);
+ 		}
 		res = true;
 	}
 	unlockData();
@@ -421,12 +429,44 @@ S32 LLQueuedThread::processNextRequest()
 		if ((req->getFlags() & FLAG_ABORT) || (mStatus == QUITTING))
 		{
 			req->setStatus(STATUS_ABORTED);
+			// Unlock, because we can't call finishRequest() while keeping this lock:
+			// generateHandle() takes this lock too and is called while holding a lock
+			// (ie LLTextureFetchWorker::mWorkMutex) that finishRequest will lock too,
+			// causing a dead lock.
+			// Although a complete rewrite of LLQueuedThread is in order because it's
+			// far from robust the way it handles it's locking; the following rationale
+			// at least makes plausible that releasing the lock here SHOULD work if
+			// the original coder didn't completely fuck up: if before the QueuedRequest
+			// req was only accessed while keeping the lock, then it still should
+			// never happen that another thread is waiting for this lock in order to
+			// access the QueuedRequest: a few lines lower we delete it.
+			// In other words, if directly after releasing this lock another thread
+			// would access req, then that can only happen by finding it again in
+			// either mRequestQueue or mRequestHash. We already deleted it from the
+			// first, so this access would have to happen by finding it in mRequestHash.
+			// Such access happens in the following functions:
+			// 1) LLQueuedThread::shutdown -- but it does that anyway, as it doesn't use any locking.
+			// 2) LLQueuedThread::generateHandle -- might skip our handle while before it would block until we deleted it. Skipping it is actually better.
+			// 3) LLQueuedThread::waitForResult -- this now doesn't touch the req when it has the flag FLAG_LOCKED set.
+			// 4) LLQueuedThread::getRequest -- whereever this is used, FLAG_LOCKED is tested before using the req.
+			// 5) LLQueuedThread::getRequestStatus -- this is a read-only operation on the status, which should never be changed from finishRequest().
+			// 6) LLQueuedThread::abortRequest -- it doesn't seem to hurt to add flags (if this happens at all), while calling finishRequest().
+			// 7) LLQueuedThread::setFlags -- same.
+			// 8) LLQueuedThread::setPriority -- doesn't access req with status STATUS_ABORTED, STATUS_COMPLETE or STATUS_INPROGRESS.
+			// 9) LLQueuedThread::completeRequest -- now sets FLAG_AUTO_COMPLETE instead of deleting the req, if FLAG_LOCKED is set, so that deletion happens here when finishRequest returns.
+			req->setFlags(FLAG_LOCKED);
+			unlockData();
 			req->finishRequest(false);
-			if (req->getFlags() & FLAG_AUTO_COMPLETE)
+			lockData();
+			req->resetFlags(FLAG_LOCKED);
+			if ((req->getFlags() & FLAG_AUTO_COMPLETE))
 			{
+				req->resetFlags(FLAG_AUTO_COMPLETE);
 				mRequestHash.erase(req);
-				req->deleteRequest();
 // 				check();
+				unlockData();
+				req->deleteRequest();
+				lockData();
 			}
 			continue;
 		}
@@ -453,14 +493,23 @@ S32 LLQueuedThread::processNextRequest()
 		{
 			lockData();
 			req->setStatus(STATUS_COMPLETE);
-			req->finishRequest(true);
+			req->setFlags(FLAG_LOCKED);
-			if (req->getFlags() & FLAG_AUTO_COMPLETE)
-			{
-				mRequestHash.erase(req);
-				req->deleteRequest();
-// 				check();
-			}
 			unlockData();
+			req->finishRequest(true);
+			if ((req->getFlags() & FLAG_AUTO_COMPLETE))
+			{
+				lockData();
+				req->resetFlags(FLAG_AUTO_COMPLETE);
+				mRequestHash.erase(req);
+// 				check();
+				req->resetFlags(FLAG_LOCKED);
+				unlockData();
+				req->deleteRequest();
+			}
+			else
+			{
+				req->resetFlags(FLAG_LOCKED);
+			}
 		}
 		else
 		{

indra/llcommon/llqueuedthread.h

@@ -72,7 +72,8 @@ public:
 	enum flags_t {
 		FLAG_AUTO_COMPLETE = 1,
 		FLAG_AUTO_DELETE = 2, // child-class dependent
-		FLAG_ABORT = 4
+		FLAG_ABORT = 4,
+		FLAG_LOCKED = 8
 	};
 
 	typedef U32 handle_t;
@@ -122,6 +123,10 @@ public:
 			// NOTE: flags are |'d
 			mFlags |= flags;
 		}
+		void resetFlags(U32 flags)
+		{
+			mFlags &= ~flags;
+		}
 		
 		virtual bool processRequest() = 0; // Return true when request has completed
 		virtual void finishRequest(bool completed); // Always called from thread after request has completed or aborted

indra/llcommon/llworkerthread.cpp

@@ -226,7 +226,8 @@ LLWorkerClass::~LLWorkerClass()
 			llerrs << "LLWorkerClass destroyed with stale work handle" << llendl;
 		}
 		if (workreq->getStatus() != LLWorkerThread::STATUS_ABORTED &&
-			workreq->getStatus() != LLWorkerThread::STATUS_COMPLETE)
+			workreq->getStatus() != LLWorkerThread::STATUS_COMPLETE &&
+			!(workreq->getFlags() & LLWorkerThread::FLAG_LOCKED))
 		{
 			llerrs << "LLWorkerClass destroyed with active worker! Worker Status: " << workreq->getStatus() << llendl;
 		}
@@ -350,14 +351,10 @@ bool LLWorkerClass::checkWork(bool aborting)
 		}
 
 		LLQueuedThread::status_t status = workreq->getStatus();
-		if (status == LLWorkerThread::STATUS_ABORTED)
+		if (status == LLWorkerThread::STATUS_COMPLETE || status == LLWorkerThread::STATUS_ABORTED)
 		{
-			complete = true;
+			complete = !(workreq->getFlags() & LLWorkerThread::FLAG_LOCKED);
-			abort = true;
+			abort = status == LLWorkerThread::STATUS_ABORTED;
-		}
-		else if (status == LLWorkerThread::STATUS_COMPLETE)
-		{
-			complete = true;
 		}
 		else
 		{

indra/llvfs/llvfile.cpp

@@ -175,7 +175,7 @@ BOOL LLVFile::isReadComplete()
 	{
 		LLVFSThread::Request* req = (LLVFSThread::Request*)sVFSThread->getRequest(mHandle);
 		LLVFSThread::status_t status = req->getStatus();
-		if (status == LLVFSThread::STATUS_COMPLETE)
+		if (status == LLVFSThread::STATUS_COMPLETE && !(req->getFlags() & LLVFSThread::FLAG_LOCKED))
 		{
 			mBytesRead = req->getBytesRead();
 			mPosition += mBytesRead;