From 2938b8238c4c937b18faf2595532cb60a4071712 Mon Sep 17 00:00:00 2001 From: Andrey Kleshchev Date: Mon, 25 May 2020 23:59:33 +0300 Subject: SL-12889 Failed to cache image crashes --- indra/newview/lltexturecache.cpp | 7 +++++-- indra/newview/lltexturefetch.cpp | 5 +++++ indra/newview/lltexturefetch.h | 1 + indra/newview/llviewertexture.cpp | 9 +++++++-- 4 files changed, 18 insertions(+), 4 deletions(-) diff --git a/indra/newview/lltexturecache.cpp b/indra/newview/lltexturecache.cpp index 2e52414d71..6211d0ce3b 100644 --- a/indra/newview/lltexturecache.cpp +++ b/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>= 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 diff --git a/indra/newview/lltexturefetch.cpp b/indra/newview/lltexturefetch.cpp index fe058024f6..f64db7beb5 100644 --- a/indra/newview/lltexturefetch.cpp +++ b/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); diff --git a/indra/newview/lltexturefetch.h b/indra/newview/lltexturefetch.h index cdf8868597..2aa194e141 100644 --- a/indra/newview/lltexturefetch.h +++ b/indra/newview/lltexturefetch.h @@ -92,6 +92,7 @@ public: 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& raw, LLPointer& aux, LLCore::HttpStatus& last_http_get_status); diff --git a/indra/newview/llviewertexture.cpp b/indra/newview/llviewertexture.cpp index a2cec9a613..bd83a61e5b 100644 --- a/indra/newview/llviewertexture.cpp +++ b/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); } } -- cgit v1.2.3