SL-12889 Failed to cache image crashes

indra/newview/lltexturecache.cpp

@@ -616,6 +616,9 @@ bool LLTextureCacheRemoteWorker::doWrite()
 			if(idx >= 0)
 			{
 				// write to the fast cache.
+                // mRawImage is not entirely safe here since it is a pointer to one owned by cache worker,
+                // it could have been retrieved via getRequestFinished() and then modified.
+                // If writeToFastCache crashes, something is wrong around fetch worker.
 				if(!mCache->writeToFastCache(mID, idx, mRawImage, mRawDiscardLevel))
 				{
 					LL_WARNS() << "writeToFastCache failed" << LL_ENDL;
@@ -2155,8 +2158,8 @@ bool LLTextureCache::writeToFastCache(LLUUID image_id, S32 id, LLPointer<LLImage
 		h >>= i;
 		if(w * h *c > 0) //valid
 		{
-			//make a duplicate to keep the original raw image untouched.
-
+            // Make a duplicate to keep the original raw image untouched.
+            // Might be good idea to do a copy during writeToCache() call instead of here
             try
             {
 #if LL_WINDOWS

indra/newview/lltexturefetch.cpp

@@ -1977,6 +1977,11 @@ bool LLTextureFetchWorker::doWork(S32 param)
 		setState(WAIT_ON_WRITE);
 		++mCacheWriteCount;
 		CacheWriteResponder* responder = new CacheWriteResponder(mFetcher, mID);
+        // This call might be under work mutex, but mRawImage is not nessesary safe here.
+        // If something retrieves it via getRequestFinished() and modifies, image won't
+        // be protected by work mutex and won't be safe to use here nor in cache worker.
+        // So make sure users of getRequestFinished() does not attempt to modify image while
+        // fetcher is working
 		mCacheWriteHandle = mFetcher->mTextureCache->writeToCache(mID, cache_priority,
 																  mFormattedImage->getData(), datasize,
 																  mFileSize, mRawImage, mDecodedDiscard, responder);

indra/newview/lltexturefetch.h

@@ -92,6 +92,7 @@ class LLTextureFetch : public LLWorkerThread
 	void deleteAllRequests();
 
 	// Threads:  T*
+	// keep in mind that if fetcher isn't done, it still might need original raw image
 	bool getRequestFinished(const LLUUID& id, S32& discard_level,
 							LLPointer<LLImageRaw>& raw, LLPointer<LLImageRaw>& aux,
 							LLCore::HttpStatus& last_http_get_status);

indra/newview/llviewertexture.cpp

@@ -1238,6 +1238,8 @@ void LLViewerFetchedTexture::loadFromFastCache()
 		{
             if (mBoostLevel == LLGLTexture::BOOST_ICON)
             {
+                // Shouldn't do anything usefull since texures in fast cache are 16x16,
+                // it is here in case fast cache changes.
                 S32 expected_width = mKnownDrawWidth > 0 ? mKnownDrawWidth : DEFAULT_ICON_DIMENTIONS;
                 S32 expected_height = mKnownDrawHeight > 0 ? mKnownDrawHeight : DEFAULT_ICON_DIMENTIONS;
                 if (mRawImage && (mRawImage->getWidth() > expected_width || mRawImage->getHeight() > expected_height))
@@ -1485,7 +1487,8 @@ BOOL LLViewerFetchedTexture::createTexture(S32 usename/*= 0*/)
 		mOrigWidth = mRawImage->getWidth();
 		mOrigHeight = mRawImage->getHeight();
 
-			
+        // This is only safe because it's a local image and fetcher doesn't use raw data
+        // from local images, but this might become unsafe in case of changes to fetcher
 		if (mBoostLevel == BOOST_PREVIEW)
 		{ 
 			mRawImage->biasedScaleToPowerOfTwo(1024);
@@ -1989,6 +1992,7 @@ bool LLViewerFetchedTexture::updateFetch()
 		
 		if (mRawImage.notNull()) sRawCount--;
 		if (mAuxRawImage.notNull()) sAuxCount--;
+		// keep in mind that fetcher still might need raw image, don't modify original
 		bool finished = LLAppViewer::getTextureFetch()->getRequestFinished(getID(), fetch_discard, mRawImage, mAuxRawImage,
 																		   mLastHttpGetStatus);
 		if (mRawImage.notNull()) sRawCount++;
@@ -2048,7 +2052,8 @@ bool LLViewerFetchedTexture::updateFetch()
                     if (mRawImage && (mRawImage->getWidth() > expected_width || mRawImage->getHeight() > expected_height))
                     {
                         // scale oversized icon, no need to give more work to gl
-                        mRawImage->scale(expected_width, expected_height);
+                        // since we got mRawImage from thread worker and image may be in use (ex: writing cache), make a copy
+                        mRawImage = mRawImage->scaled(expected_width, expected_height);
                     }
                 }