EXT-2349 : Diagnose warning spam: [X] is in model and in view, but STRUCTURE flag not set

No longer allowing recursive calls to gInventory.notifyObservers.  Added debugging tools to catch when this happens.
This fixes the symptom, but we'll also need to remove whatever code is causing the recursive calls.

2 files changed, 52 insertions, 18 deletions

diff --git a/indra/newview/llinventorymodel.cpp b/indra/newview/llinventorymodel.cpp
index b41695fd34..51052a22b5 100644
--- a/indra/newview/llinventorymodel.cpp
+++ b/indra/newview/llinventorymodel.cpp
@@ -172,6 +172,7 @@ LLInventoryModel::LLInventoryModel()
 	mRootFolderID(),
 	mLibraryRootFolderID(),
 	mLibraryOwnerID(),
+	mIsNotifyObservers(FALSE),
 	mIsAgentInvUsable(false)
 {
 }
@@ -533,7 +534,10 @@ void LLInventoryModel::updateLinkedItems(const LLUUID& object_id)
 						 item_array,
 						 LLInventoryModel::INCLUDE_TRASH,
 						 is_linked_item_match);
-
+	if (cat_array.empty() && item_array.empty())
+	{
+		return;
+	}
 	for (LLInventoryModel::cat_array_t::iterator cat_iter = cat_array.begin();
 		 cat_iter != cat_array.end();
 		 cat_iter++)
@@ -635,6 +639,7 @@ U32 LLInventoryModel::updateItem(const LLViewerInventoryItem* item)
 		new_item = old_item;
 		LLUUID old_parent_id = old_item->getParentUUID();
 		LLUUID new_parent_id = item->getParentUUID();
+			
 		if(old_parent_id != new_parent_id)
 		{
 			// need to update the parent-child tree
@@ -1129,6 +1134,16 @@ BOOL LLInventoryModel::containsObserver(LLInventoryObserver* observer) const
 // The optional argument 'service_name' is used by Agent Inventory Service [DEV-20328]
 void LLInventoryModel::notifyObservers(const std::string service_name)
 {
+	if (mIsNotifyObservers)
+	{
+		// Within notifyObservers, something called notifyObservers
+		// again.  This type of recursion is unsafe because it causes items to be 
+		// processed twice, and this can easily lead to infinite loops.
+		llwarns << "Call was made to notifyObservers within notifyObservers!" << llendl;
+		return;
+	}
+	mIsNotifyObservers = TRUE;
+	llinfos << "Start process notifyObservers for " << this << llendl;
 	for (observer_list_t::iterator iter = mObservers.begin();
 		 iter != mObservers.end(); )
 	{
@@ -1150,12 +1165,21 @@ void LLInventoryModel::notifyObservers(const std::string service_name)
 
 	mModifyMask = LLInventoryObserver::NONE;
 	mChangedItemIDs.clear();
+	mIsNotifyObservers = FALSE;
 }
 
 // store flag for change
 // and id of object change applies to
 void LLInventoryModel::addChangedMask(U32 mask, const LLUUID& referent) 
 { 
+	if (mIsNotifyObservers)
+	{
+		// Something marked an item for change within a call to notifyObservers
+		// (which is in the process of processing the list of items marked for change).
+		// This means the change may fail to be processed.
+		llwarns << "Adding changed mask within notify observers!  Change will likely be lost." << llendl;
+	}
+	
 	mModifyMask |= mask; 
 	if (referent.notNull())
 	{
@@ -1824,13 +1848,13 @@ void LLInventoryModel::addItem(LLViewerInventoryItem* item)
 {
 	//llinfos << "LLInventoryModel::addItem()" << llendl;
 
-	
-	// This can happen if assettype enums change.  This can be a backwards compatibility issue 
-	// in some viewer prototypes prior to when the AT_LINK enum changed from 23 to 24.
+	// This can happen if assettype enums from llassettype.h ever change.
+	// For example, there is a known backwards compatibility issue in some viewer prototypes prior to when 
+	// the AT_LINK enum changed from 23 to 24.
 	if ((item->getType() == LLAssetType::AT_NONE)
 		|| LLAssetType::lookup(item->getType()) == LLAssetType::badLookup())
 	{
-		llwarns << "Got bad asset type for item ( name: " << item->getName() << " type: " << item->getType() << " inv-type: " << item->getInventoryType() << " ), ignoring." << llendl;
+		llwarns << "Got bad asset type for item [ name: " << item->getName() << " type: " << item->getType() << " inv-type: " << item->getInventoryType() << " ], ignoring." << llendl;
 		return;
 	}
 	if(item)
@@ -1839,7 +1863,7 @@ void LLInventoryModel::addItem(LLViewerInventoryItem* item)
 		// The item will show up as a broken link.
 		if (item->getIsBrokenLink())
 		{
-			llinfos << "Adding broken link ( name: " << item->getName() << " itemID: " << item->getUUID() << " assetID: " << item->getAssetUUID() << " )  parent: " << item->getParentUUID() << llendl;
+			llinfos << "Adding broken link [ name: " << item->getName() << " itemID: " << item->getUUID() << " assetID: " << item->getAssetUUID() << " )  parent: " << item->getParentUUID() << llendl;
 		}
 		mItemMap[item->getUUID()] = item;
 	}
@@ -3263,6 +3287,12 @@ void LLInventoryModel::processInventoryDescendents(LLMessageSystem* msg,void**)
 	for(i = 0; i < count; ++i)
 	{
 		titem->unpackMessage(msg, _PREHASH_ItemData, i);
+		// If the item has already been added (e.g. from link prefetch), then it doesn't need to be re-added.
+		if (gInventory.getItem(titem->getUUID()))
+		{
+			llinfos << "Skipping prefetched item [ Name: " << titem->getName() << " | Type: " << titem->getActualType() << " | ItemUUID: " << titem->getUUID() << " ] " << llendl;
+			continue;
+		}
 		gInventory.updateItem(titem);
 	}
 
diff --git a/indra/newview/llinventorymodel.h b/indra/newview/llinventorymodel.h
index aba0a619db..5f51408bcf 100644
--- a/indra/newview/llinventorymodel.h
+++ b/indra/newview/llinventorymodel.h
@@ -472,23 +472,12 @@ protected:
 	cat_array_t* getUnlockedCatArray(const LLUUID& id);
 	item_array_t* getUnlockedItemArray(const LLUUID& id);
 	
-protected:
+private:
 	// Variables used to track what has changed since the last notify.
 	U32 mModifyMask;
 	typedef std::set<LLUUID> changed_items_t;
 	changed_items_t mChangedItemIDs;
 
-	// Information for tracking the actual inventory. We index this
-	// information in a lot of different ways so we can access
-	// the inventory using several different identifiers.
-	// mInventory member data is the 'master' list of inventory, and
-	// mCategoryMap and mItemMap store uuid->object mappings. 
-	typedef std::map<LLUUID, LLPointer<LLViewerInventoryCategory> > cat_map_t;
-	typedef std::map<LLUUID, LLPointer<LLViewerInventoryItem> > item_map_t;
-	//inv_map_t mInventory;
-	cat_map_t mCategoryMap;
-	item_map_t mItemMap;
-
 	std::map<LLUUID, bool> mCategoryLock;
 	std::map<LLUUID, bool> mItemLock;
 	
@@ -521,6 +510,21 @@ protected:
 	// This flag is used to handle an invalid inventory state.
 	bool mIsAgentInvUsable;
 
+private:
+	// Information for tracking the actual inventory. We index this
+	// information in a lot of different ways so we can access
+	// the inventory using several different identifiers.
+	// mInventory member data is the 'master' list of inventory, and
+	// mCategoryMap and mItemMap store uuid->object mappings. 
+	typedef std::map<LLUUID, LLPointer<LLViewerInventoryCategory> > cat_map_t;
+	typedef std::map<LLUUID, LLPointer<LLViewerInventoryItem> > item_map_t;
+	//inv_map_t mInventory;
+	cat_map_t mCategoryMap;
+	item_map_t mItemMap;
+
+	// Flag set when notifyObservers is being called, to look for bugs
+	// where it's called recursively.
+	BOOL mIsNotifyObservers;
 public:
 	// *NOTE: DEBUG functionality
 	void dumpInventory() const;