From db161b6f85a266ee9d883e3180874de898ccda0e Mon Sep 17 00:00:00 2001 From: Andrey Kleshchev Date: Tue, 5 Jan 2021 18:20:30 +0200 Subject: SL-14074 Resolved potential cause of conversation crashes Models (listeners) belong to folder-views, deleting all of them means that not only proper cleanup procedure won't be followed, and view might try clean dead pointer. --- indra/newview/llconversationmodel.cpp | 3 +++ 1 file changed, 3 insertions(+) (limited to 'indra/newview/llconversationmodel.cpp') diff --git a/indra/newview/llconversationmodel.cpp b/indra/newview/llconversationmodel.cpp index 4aa74a550c..8293dc6922 100644 --- a/indra/newview/llconversationmodel.cpp +++ b/indra/newview/llconversationmodel.cpp @@ -341,6 +341,9 @@ void LLConversationItemSession::removeParticipant(const LLUUID& participant_id) void LLConversationItemSession::clearParticipants() { + // clearParticipants function potentially is malfunctioning since it only cleans children of models, + // it does nothing to views that own those models (listeners) + // probably needs to post some kind of 'remove all participants' event clearChildren(); mIsLoaded = false; mNeedsRefresh = true; -- cgit v1.3 From f86014ef151c7af64de4a08dc4c320e1743fb34b Mon Sep 17 00:00:00 2001 From: Andrey Kleshchev Date: Wed, 6 Jan 2021 00:45:07 +0200 Subject: SL-14270 Crash on participant's updateName Session was deleted before viewer had a chance to create view for listener, so listener ended up not deleted and avaiting an uptade, then tryed to call for deleted session. --- indra/newview/llconversationmodel.cpp | 14 +++++++++ indra/newview/llconversationmodel.h | 1 + indra/newview/llconversationview.cpp | 51 +++++++++++++++++++++++++++++++++ indra/newview/llconversationview.h | 2 ++ indra/newview/llfloaterimcontainer.cpp | 2 ++ indra/newview/llfloaterimsessiontab.cpp | 3 +- 6 files changed, 71 insertions(+), 2 deletions(-) (limited to 'indra/newview/llconversationmodel.cpp') diff --git a/indra/newview/llconversationmodel.cpp b/indra/newview/llconversationmodel.cpp index 8293dc6922..fbdf08d8aa 100644 --- a/indra/newview/llconversationmodel.cpp +++ b/indra/newview/llconversationmodel.cpp @@ -349,6 +349,20 @@ void LLConversationItemSession::clearParticipants() mNeedsRefresh = true; } + +void LLConversationItemSession::deleteParticipantModels() +{ + // Make sure that no views exist before use and that view-owned items were removed! + // + // Normally we are not supposed to delete models directly, they should be + // owned by views and this action will result in crashes, but LLParticipantList + // creates models separately from views (it probably shouldn't) and then those + // models wait for idle cycles to be assigned to view. + // this code is meant to delete 'waiting' models + std::for_each(mChildren.begin(), mChildren.end(), DeletePointer()); + mChildren.clear(); +} + LLConversationItemParticipant* LLConversationItemSession::findParticipant(const LLUUID& participant_id) { // This is *not* a general tree parsing algorithm. It assumes that a session contains only diff --git a/indra/newview/llconversationmodel.h b/indra/newview/llconversationmodel.h index 30c7481864..e30bfbb759 100644 --- a/indra/newview/llconversationmodel.h +++ b/indra/newview/llconversationmodel.h @@ -165,6 +165,7 @@ public: void removeParticipant(LLConversationItemParticipant* participant); void removeParticipant(const LLUUID& participant_id); void clearParticipants(); + void deleteParticipantModels(); // do not use while there are existing participant views LLConversationItemParticipant* findParticipant(const LLUUID& participant_id); void setParticipantIsMuted(const LLUUID& participant_id, bool is_muted); diff --git a/indra/newview/llconversationview.cpp b/indra/newview/llconversationview.cpp index 25971a7d52..71346b4b43 100644 --- a/indra/newview/llconversationview.cpp +++ b/indra/newview/llconversationview.cpp @@ -103,6 +103,57 @@ LLConversationViewSession::~LLConversationViewSession() mFlashTimer->unset(); } +void LLConversationViewSession::destroyView() +{ + // Chat can create and parent models(listeners) to session's model before creating + // coresponding views, such participant's models normally will wait for idle cycles + // but since we are deleting session and won't be processing any more events, make + // sure unowned models are removed as well. + // Might be good idea to just have an LLPointer list somewhere in LLConversationItemSession + + LLConversationItemSession* vmi = dynamic_cast(getViewModelItem()); + + // CONV_SESSION_1_ON_1 stores participants as two models that belong to views independent + // from session (nasty! These views are widgets in LLFloaterIMSessionTab, see buildConversationViewParticipant) + if (vmi && vmi->getType() != LLConversationItem::CONV_SESSION_1_ON_1) + { + // Destroy existing views + while (!mItems.empty()) + { + LLFolderViewItem *itemp = mItems.back(); + mItems.pop_back(); + + LLFolderViewModelItem* item_vmi = itemp->getViewModelItem(); + if (item_vmi) // supposed to exist + { + // unparent to remove from child list + vmi->removeChild(item_vmi); + } + itemp->destroyView(); + } + + // Not needed in scope of sessions, but just in case + while (!mFolders.empty()) + { + LLFolderViewFolder *folderp = mFolders.back(); + mFolders.pop_back(); + + LLFolderViewModelItem* folder_vmi = folderp->getViewModelItem(); + if (folder_vmi) + { + vmi->removeChild(folder_vmi); + } + folderp->destroyView(); + } + + // Now everything that is left in model(listener) is unowned, + // it is safe to remove + vmi->deleteParticipantModels(); + } + + LLFolderViewFolder::destroyView(); +} + void LLConversationViewSession::setFlashState(bool flash_state) { if (flash_state && !mFlashStateOn) diff --git a/indra/newview/llconversationview.h b/indra/newview/llconversationview.h index 9762d56d57..0932d24dfe 100644 --- a/indra/newview/llconversationview.h +++ b/indra/newview/llconversationview.h @@ -67,6 +67,8 @@ protected: public: virtual ~LLConversationViewSession(); + /*virtual*/ void destroyView(); + /*virtual*/ BOOL postBuild(); /*virtual*/ void draw(); /*virtual*/ BOOL handleMouseDown( S32 x, S32 y, MASK mask ); diff --git a/indra/newview/llfloaterimcontainer.cpp b/indra/newview/llfloaterimcontainer.cpp index 61b0fda18f..86c79791b8 100644 --- a/indra/newview/llfloaterimcontainer.cpp +++ b/indra/newview/llfloaterimcontainer.cpp @@ -1822,6 +1822,8 @@ bool LLFloaterIMContainer::removeConversationListItem(const LLUUID& uuid, bool c { new_selection = mConversationsRoot->getPreviousFromChild(widget, FALSE); } + + // Will destroy views and delete models that are not assigned to any views widget->destroyView(); } diff --git a/indra/newview/llfloaterimsessiontab.cpp b/indra/newview/llfloaterimsessiontab.cpp index fd3f8b21ce..e7f428c06a 100644 --- a/indra/newview/llfloaterimsessiontab.cpp +++ b/indra/newview/llfloaterimsessiontab.cpp @@ -534,8 +534,7 @@ void LLFloaterIMSessionTab::removeConversationViewParticipant(const LLUUID& part LLFolderViewItem* widget = get_ptr_in_map(mConversationsWidgets,participant_id); if (widget) { - mConversationsRoot->extractItem(widget); - delete widget; + widget->destroyView(); } mConversationsWidgets.erase(participant_id); } -- cgit v1.3 From 18323f019f51a0ca27470ec770232fc4618fcd4b Mon Sep 17 00:00:00 2001 From: Andrey Kleshchev Date: Wed, 31 Mar 2021 21:45:35 +0300 Subject: SL-15061 Crash deleting non-zero reference in LLConversationItemSession --- indra/newview/llconversationmodel.cpp | 28 ++++++++++++++++++---------- indra/newview/llconversationmodel.h | 2 +- indra/newview/llconversationview.cpp | 9 ++++----- 3 files changed, 23 insertions(+), 16 deletions(-) (limited to 'indra/newview/llconversationmodel.cpp') diff --git a/indra/newview/llconversationmodel.cpp b/indra/newview/llconversationmodel.cpp index fbdf08d8aa..4cfde21e32 100644 --- a/indra/newview/llconversationmodel.cpp +++ b/indra/newview/llconversationmodel.cpp @@ -350,16 +350,24 @@ void LLConversationItemSession::clearParticipants() } -void LLConversationItemSession::deleteParticipantModels() -{ - // Make sure that no views exist before use and that view-owned items were removed! - // - // Normally we are not supposed to delete models directly, they should be - // owned by views and this action will result in crashes, but LLParticipantList - // creates models separately from views (it probably shouldn't) and then those - // models wait for idle cycles to be assigned to view. - // this code is meant to delete 'waiting' models - std::for_each(mChildren.begin(), mChildren.end(), DeletePointer()); +void LLConversationItemSession::clearAndDeparentModels() +{ + std::for_each(mChildren.begin(), mChildren.end(), + [](LLFolderViewModelItem* c) + { + if (c->getNumRefs() == 0) + { + // LLConversationItemParticipant can be created but not assigned to any view, + // it was waiting for an "add_participant" event to be processed + delete c; + } + else + { + // Model is still assigned to some view/widget + c->setParent(NULL); + } + } + ); mChildren.clear(); } diff --git a/indra/newview/llconversationmodel.h b/indra/newview/llconversationmodel.h index e30bfbb759..787deeb594 100644 --- a/indra/newview/llconversationmodel.h +++ b/indra/newview/llconversationmodel.h @@ -165,7 +165,7 @@ public: void removeParticipant(LLConversationItemParticipant* participant); void removeParticipant(const LLUUID& participant_id); void clearParticipants(); - void deleteParticipantModels(); // do not use while there are existing participant views + void clearAndDeparentModels(); // will delete unowned models and deparent owned ones LLConversationItemParticipant* findParticipant(const LLUUID& participant_id); void setParticipantIsMuted(const LLUUID& participant_id, bool is_muted); diff --git a/indra/newview/llconversationview.cpp b/indra/newview/llconversationview.cpp index 71346b4b43..df16868132 100644 --- a/indra/newview/llconversationview.cpp +++ b/indra/newview/llconversationview.cpp @@ -108,8 +108,7 @@ void LLConversationViewSession::destroyView() // Chat can create and parent models(listeners) to session's model before creating // coresponding views, such participant's models normally will wait for idle cycles // but since we are deleting session and won't be processing any more events, make - // sure unowned models are removed as well. - // Might be good idea to just have an LLPointer list somewhere in LLConversationItemSession + // sure unowned LLConversationItemParticipant models are removed as well. LLConversationItemSession* vmi = dynamic_cast(getViewModelItem()); @@ -146,9 +145,9 @@ void LLConversationViewSession::destroyView() folderp->destroyView(); } - // Now everything that is left in model(listener) is unowned, - // it is safe to remove - vmi->deleteParticipantModels(); + // Now everything that is left in model(listener) is not owned by views, + // only by sessions, deparent so it won't point to soon to be dead model + vmi->clearAndDeparentModels(); } LLFolderViewFolder::destroyView(); -- cgit v1.3