From 3513f67d2c5628eac2ce9aa632a8491bb215a9e8 Mon Sep 17 00:00:00 2001 From: Brad Linden Date: Mon, 17 Apr 2023 10:24:23 -0700 Subject: Attempt at fixing thread safety possibly related to SL-19648 --- indra/newview/llgltfmateriallist.cpp | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/indra/newview/llgltfmateriallist.cpp b/indra/newview/llgltfmateriallist.cpp index 151d7fa969..7d677626ce 100644 --- a/indra/newview/llgltfmateriallist.cpp +++ b/indra/newview/llgltfmateriallist.cpp @@ -219,7 +219,7 @@ public: struct ReturnData { public: - LLPointer mMaterial; + LLGLTFMaterial mMaterial; S32 mSide; bool mSuccess; }; @@ -239,20 +239,16 @@ public: std::unordered_map::const_iterator end = object_override.mSides.end(); while (iter != end) { - LLPointer override_data = new LLGLTFMaterial(); std::string warn_msg, error_msg; - bool success = override_data->fromJSON(iter->second, warn_msg, error_msg); - ReturnData result; + + bool success = result.mMaterial.fromJSON(iter->second, warn_msg, error_msg); + result.mSuccess = success; result.mSide = iter->first; - if (success) - { - result.mMaterial = override_data; - } - else + if (!success) { LL_WARNS("GLTF") << "failed to parse GLTF override data. errors: " << error_msg << " | warnings: " << warn_msg << LL_ENDL; } @@ -271,23 +267,27 @@ public: { std::unordered_set side_set; - for (int i = 0; i < results.size(); ++i) + for (auto const & result : results) { - if (results[i].mSuccess) + S32 side = result.mSide; + if (result.mSuccess) { + // copy to heap here because LLTextureEntry is going to take ownership with an LLPointer + LLGLTFMaterial * material = new LLGLTFMaterial(result.mMaterial); + // flag this side to not be nulled out later - side_set.insert(results[i].mSide); + side_set.insert(side); if (obj) { - obj->setTEGLTFMaterialOverride(results[i].mSide, results[i].mMaterial); + obj->setTEGLTFMaterialOverride(side, material); } } // unblock material editor - if (obj && obj->getTE(results[i].mSide) && obj->getTE(results[i].mSide)->isSelected()) + if (obj && obj->getTE(side) && obj->getTE(side)->isSelected()) { - doSelectionCallbacks(object_override.mObjectId, results[i].mSide); + doSelectionCallbacks(object_override.mObjectId, side); } } -- cgit v1.2.3 From 6a812fa1ba9170ba34706d303913cc5aa5f187ac Mon Sep 17 00:00:00 2001 From: Brad Linden Date: Wed, 3 May 2023 13:23:05 -0700 Subject: Improved fix for SL-19675 crash. How about just don't refer to data when you don't need it --- indra/llprimitive/llgltfmaterial.h | 1 + indra/newview/llgltfmateriallist.cpp | 18 +++++++++--------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/indra/llprimitive/llgltfmaterial.h b/indra/llprimitive/llgltfmaterial.h index 0bd65fadef..ad7784f6d1 100644 --- a/indra/llprimitive/llgltfmaterial.h +++ b/indra/llprimitive/llgltfmaterial.h @@ -167,6 +167,7 @@ public: // set the contents of this LLGLTFMaterial from the given json // returns true if successful + // if unsuccessful, the contents of this LLGLTFMaterial should be left unchanged and false is returned // json - the json text to load from // warn_msg - warning message from TinyGLTF if any // error_msg - error_msg from TinyGLTF if any diff --git a/indra/newview/llgltfmateriallist.cpp b/indra/newview/llgltfmateriallist.cpp index 7d677626ce..1e49ea2eba 100644 --- a/indra/newview/llgltfmateriallist.cpp +++ b/indra/newview/llgltfmateriallist.cpp @@ -227,16 +227,16 @@ public: // fromJson() is performance heavy offload to a thread. main_queue->postTo( general_queue, - [object_override]() // Work done on general queue + [sides=object_override.mSides]() // Work done on general queue { std::vector results; - if (!object_override.mSides.empty()) + if (!sides.empty()) { - results.reserve(object_override.mSides.size()); + results.reserve(sides.size()); // parse json - std::unordered_map::const_iterator iter = object_override.mSides.begin(); - std::unordered_map::const_iterator end = object_override.mSides.end(); + std::unordered_map::const_iterator iter = sides.begin(); + std::unordered_map::const_iterator end = sides.end(); while (iter != end) { std::string warn_msg, error_msg; @@ -259,9 +259,9 @@ public: } return results; }, - [object_override, this](std::vector results) // Callback to main thread + [object_id=object_override.mObjectId, this](std::vector results) // Callback to main thread { - LLViewerObject * obj = gObjectList.findObject(object_override.mObjectId); + LLViewerObject * obj = gObjectList.findObject(object_id); if (results.size() > 0 ) { @@ -287,7 +287,7 @@ public: // unblock material editor if (obj && obj->getTE(side) && obj->getTE(side)->isSelected()) { - doSelectionCallbacks(object_override.mObjectId, side); + doSelectionCallbacks(object_id, side); } } @@ -300,7 +300,7 @@ public: obj->setTEGLTFMaterialOverride(i, nullptr); if (obj->getTE(i) && obj->getTE(i)->isSelected()) { - doSelectionCallbacks(object_override.mObjectId, i); + doSelectionCallbacks(object_id, i); } } } -- cgit v1.2.3 From 5465594a34ec65c1f4eeb9181be190e47e6527ad Mon Sep 17 00:00:00 2001 From: Brad Linden Date: Wed, 3 May 2023 13:28:29 -0700 Subject: Cleanup with SL-19675 fix. lifted empty check outside the workqueue post and cleaned up indentation --- indra/newview/llgltfmateriallist.cpp | 98 ++++++++++++++++++------------------ 1 file changed, 49 insertions(+), 49 deletions(-) diff --git a/indra/newview/llgltfmateriallist.cpp b/indra/newview/llgltfmateriallist.cpp index 1e49ea2eba..ed16a1cf7a 100644 --- a/indra/newview/llgltfmateriallist.cpp +++ b/indra/newview/llgltfmateriallist.cpp @@ -224,15 +224,15 @@ public: bool mSuccess; }; - // fromJson() is performance heavy offload to a thread. - main_queue->postTo( - general_queue, - [sides=object_override.mSides]() // Work done on general queue + if (!object_override.mSides.empty()) { - std::vector results; - - if (!sides.empty()) + // fromJson() is performance heavy offload to a thread. + main_queue->postTo( + general_queue, + [sides=object_override.mSides]() // Work done on general queue { + std::vector results; + results.reserve(sides.size()); // parse json std::unordered_map::const_iterator iter = sides.begin(); @@ -256,68 +256,68 @@ public: results.push_back(result); iter++; } - } - return results; - }, + return results; + }, [object_id=object_override.mObjectId, this](std::vector results) // Callback to main thread { - LLViewerObject * obj = gObjectList.findObject(object_id); - - if (results.size() > 0 ) - { - std::unordered_set side_set; + LLViewerObject * obj = gObjectList.findObject(object_id); - for (auto const & result : results) + if (results.size() > 0 ) { - S32 side = result.mSide; - if (result.mSuccess) + std::unordered_set side_set; + + for (auto const & result : results) { - // copy to heap here because LLTextureEntry is going to take ownership with an LLPointer - LLGLTFMaterial * material = new LLGLTFMaterial(result.mMaterial); + S32 side = result.mSide; + if (result.mSuccess) + { + // copy to heap here because LLTextureEntry is going to take ownership with an LLPointer + LLGLTFMaterial * material = new LLGLTFMaterial(result.mMaterial); - // flag this side to not be nulled out later - side_set.insert(side); + // flag this side to not be nulled out later + side_set.insert(side); - if (obj) + if (obj) + { + obj->setTEGLTFMaterialOverride(side, material); + } + } + + // unblock material editor + if (obj && obj->getTE(side) && obj->getTE(side)->isSelected()) { - obj->setTEGLTFMaterialOverride(side, material); + doSelectionCallbacks(object_id, side); } } - - // unblock material editor - if (obj && obj->getTE(side) && obj->getTE(side)->isSelected()) - { - doSelectionCallbacks(object_id, side); - } - } - if (obj && side_set.size() != obj->getNumTEs()) - { // object exists and at least one texture entry needs to have its override data nulled out - for (int i = 0; i < obj->getNumTEs(); ++i) - { - if (side_set.find(i) == side_set.end()) + if (obj && side_set.size() != obj->getNumTEs()) + { // object exists and at least one texture entry needs to have its override data nulled out + for (int i = 0; i < obj->getNumTEs(); ++i) { - obj->setTEGLTFMaterialOverride(i, nullptr); - if (obj->getTE(i) && obj->getTE(i)->isSelected()) + if (side_set.find(i) == side_set.end()) { - doSelectionCallbacks(object_id, i); + obj->setTEGLTFMaterialOverride(i, nullptr); + if (obj->getTE(i) && obj->getTE(i)->isSelected()) + { + doSelectionCallbacks(object_id, i); + } } } } } - } - else if (obj) - { // override list was empty or an error occurred, null out all overrides for this object - for (int i = 0; i < obj->getNumTEs(); ++i) - { - obj->setTEGLTFMaterialOverride(i, nullptr); - if (obj->getTE(i) && obj->getTE(i)->isSelected()) + else if (obj) + { // override list was empty or an error occurred, null out all overrides for this object + for (int i = 0; i < obj->getNumTEs(); ++i) { - doSelectionCallbacks(obj->getID(), i); + obj->setTEGLTFMaterialOverride(i, nullptr); + if (obj->getTE(i) && obj->getTE(i)->isSelected()) + { + doSelectionCallbacks(obj->getID(), i); + } } } - } - }); + }); + } } private: -- cgit v1.2.3