Use explicit iterators instead of the implicit (built-in) ones due to the thread-safety requirements.

This change affects GenericMap only, as it's used globally in a few places and we have a proven crash there.
I have a more complete solution (total cleanup of default accessors from BePlusTree and SparseBitmap) in my local tree (thanks to Dmitry Kovalenko), but it's incomplete (requires more changes inside the engine) and IMHO risky (~20 core modules are affected), so I'd like to defer finalization of this cleanup till v3.0.
All objects of these types seem being protected by dbb_sync, so they shouldn't cause us any problems currently.

src/common/classes/GenericMap.h

@@ -57,9 +57,32 @@ public:
 	typedef typename KeyValuePair::first_type KeyType;
 	typedef typename KeyValuePair::second_type ValueType;
 
+	typedef BePlusTree<KeyValuePair*, KeyType, MemoryPool, FirstObjectKey<KeyValuePair>, KeyComparator> ValuesTree;
+
+	class Accessor
+	{
+	public:
+		Accessor(GenericMap* map) : m_Accessor(&map->tree) {}
+
+		KeyValuePair* current() const { return m_Accessor.current(); }			
+
+		bool getFirst() { return m_Accessor.getFirst(); }
+		bool getNext() { return m_Accessor.getNext(); }
+
+	private:
+		Accessor(const Accessor&);
+		Accessor& operator=(const Accessor&);
+
+		typename ValuesTree::Accessor m_Accessor;
+	};
+
+	friend class Accessor;
+
 	GenericMap() : tree(&getPool()), mCount(0) { }
+
 	explicit GenericMap(MemoryPool& a_pool)
 		: AutoStorage(a_pool), tree(&getPool()), mCount(0) { }
+
 	~GenericMap()
 	{
 		clear();
@@ -69,23 +92,29 @@ public:
 	{
 		clear();
 
-		for (bool found = v.getFirst(); found; found = v.getNext())
+		GenericMap::Accessor accessor(&v);
+
+		for (bool found = accessor.getFirst(); found; found = accessor.getNext())
 		{
-			KeyValuePair* current_pair = v.current();
+			const KeyValuePair* const current_pair = accessor.current();
 			put(current_pair->first, current_pair->second);
 		}
 	}
 
 	void takeOwnership(GenericMap& from)
 	{
+		fb_assert(this != &from);
+
 		clear();
 
 		tree = from.tree;
 		mCount = from.mCount;
 
-		if (from.tree.getFirst()) {
+		ValuesTree::Accessor treeAccessor(&from.tree);
+
+		if (treeAccessor.getFirst()) {
 			while (true) {
-				bool haveMore = from.tree.fastRemove();
+				bool haveMore = treeAccessor.fastRemove();
 				if (!haveMore)
 					break;
 			}
@@ -97,10 +126,12 @@ public:
 	// Clear the map
 	void clear()
 	{
-		if (tree.getFirst()) {
+		ValuesTree::Accessor treeAccessor(&tree);
+
+		if (treeAccessor.getFirst()) {
 			while (true) {
-				KeyValuePair* temp = tree.current();
+				KeyValuePair* temp = treeAccessor.current();
-				bool haveMore = tree.fastRemove();
+				bool haveMore = treeAccessor.fastRemove();
 				delete temp;
 				if (!haveMore)
 					break;
@@ -113,9 +144,11 @@ public:
 	// Returns true if value existed
 	bool remove(const KeyType& key)
 	{
-		if (tree.locate(key)) {
+		ValuesTree::Accessor treeAccessor(&tree);
-			KeyValuePair* var = tree.current();
+
-			tree.fastRemove();
+		if (treeAccessor.locate(key)) {
+			KeyValuePair* var = treeAccessor.current();
+			treeAccessor.fastRemove();
 			delete var;
 			mCount--;
 			return true;
@@ -127,8 +160,10 @@ public:
 	// Returns true if value existed previously
 	bool put(const KeyType& key, const ValueType& value)
 	{
-		if (tree.locate(key)) {
+		ValuesTree::Accessor treeAccessor(&tree);
-			tree.current()->second = value;
+
+		if (treeAccessor.locate(key)) {
+			treeAccessor.current()->second = value;
 			return true;
 		}
 
@@ -141,8 +176,10 @@ public:
 	// Returns pointer to the added empty value or null when key already exists
 	ValueType* put(const KeyType& key)
 	{
-		if (tree.locate(key)) {
+		ValuesTree::Accessor treeAccessor(&tree);
-			return 0;
+
+		if (treeAccessor.locate(key)) {
+			return NULL;
 		}
 
 		KeyValuePair* var = FB_NEW(getPool()) KeyValuePair(getPool());
@@ -155,32 +192,35 @@ public:
 	// Returns true if value is found
 	bool get(const KeyType& key, ValueType& value)
 	{
-		if (tree.locate(key)) {
+		ValuesTree::Accessor treeAccessor(&tree);
-			value = tree.current()->second;
+
+		if (treeAccessor.locate(key)) {
+			value = treeAccessor.current()->second;
 			return true;
 		}
 
 		return false;
 	}
 
-	bool getFirst() { return tree.getFirst(); }
+	// Returns pointer to the found value or null otherwise
+	ValueType* get(const KeyType& key)
+	{
+		ValuesTree::Accessor treeAccessor(&tree);
 
-	bool getLast() { return tree.getLast(); }
+		if (treeAccessor.locate(key)) {
+			return &treeAccessor.current()->second;
+		}
 
-	bool getNext() { return tree.getNext(); }
+		return NULL;
-
+	}
-	bool getPrev() { return tree.getPrev(); }
-
-	KeyValuePair* current() const { return tree.current(); }
 
 	bool exist(const KeyType& key)
 	{
-		return tree.locate(key);
+		return ValuesTree::Accessor(&tree).locate(key);
 	}
 
 	size_t count() const { return mCount; }
 
-	typedef BePlusTree<KeyValuePair*, KeyType, MemoryPool, FirstObjectKey<KeyValuePair>, KeyComparator> ValuesTree;
 private:
 	ValuesTree tree;
 	size_t mCount;

src/jrd/DatabaseSnapshot.cpp

@@ -1210,7 +1210,9 @@ void DatabaseSnapshot::putContextVars(Firebird::StringMap& variables,
 									  Firebird::ClumpletWriter& writer,
 									  int object_id, bool is_attachment)
 {
-	for (bool found = variables.getFirst(); found; found = variables.getNext())
+	StringMap::Accessor accessor(&variables);
+
+	for (bool found = accessor.getFirst(); found; found = accessor.getNext())
 	{
 		writer.insertByte(TAG_RECORD, rel_mon_ctx_vars);
 
@@ -1219,8 +1221,8 @@ void DatabaseSnapshot::putContextVars(Firebird::StringMap& variables,
 		else
 			writer.insertInt(f_mon_ctx_var_tra_id, object_id);
 
-		writer.insertString(f_mon_ctx_var_name, variables.current()->first);
+		writer.insertString(f_mon_ctx_var_name, accessor.current()->first);
-		writer.insertString(f_mon_ctx_var_value, variables.current()->second);
+		writer.insertString(f_mon_ctx_var_value, accessor.current()->second);
 	}
 }
 

src/jrd/IntlManager.cpp

@@ -58,10 +58,10 @@ namespace
 		~ModulesMap()
 		{
 			// unload modules
-			for (bool found = getFirst(); found; found = getNext())
+			Accessor accessor(this);
+			for (bool found = accessor.getFirst(); found; found = accessor.getNext())
 			{
-				ModuleLoader::Module* module = current()->second;
+				delete accessor.current()->second;
-				delete module;
 			}
 		}
 	};

src/jrd/IntlUtil.cpp

@@ -77,7 +77,9 @@ static ULONG unicodeCanonical(texttype* tt, ULONG srcLen, const UCHAR* src,
 string IntlUtil::generateSpecificAttributes(
 	Jrd::CharSet* cs, SpecificAttributesMap& map)
 {
-	bool found = map.getFirst();
+	SpecificAttributesMap::Accessor accessor(&map);
+
+	bool found = accessor.getFirst();
 	string s;
 
 	while (found)
@@ -85,7 +87,7 @@ string IntlUtil::generateSpecificAttributes(
 		UCHAR c[sizeof(ULONG)];
 		ULONG size;
 
-		SpecificAttribute* attribute = map.current();
+		SpecificAttribute* attribute = accessor.current();
 
 		s += escapeAttribute(cs, attribute->first);
 
@@ -99,7 +101,7 @@ string IntlUtil::generateSpecificAttributes(
 
 		s += escapeAttribute(cs, attribute->second);
 
-		found = map.getNext();
+		found = accessor.getNext();
 
 		if (found)
 		{
@@ -420,7 +422,9 @@ bool IntlUtil::initUnicodeCollation(texttype* tt, charset* cs, const ASCII* name
 
 	IntlUtil::SpecificAttributesMap map16;
 
-	bool found = map.getFirst();
+	SpecificAttributesMap::Accessor accessor(&map);
+
+	bool found = accessor.getFirst();
 
 	while (found)
 	{
@@ -429,20 +433,20 @@ bool IntlUtil::initUnicodeCollation(texttype* tt, charset* cs, const ASCII* name
 		ULONG errPosition;
 
 		s1.resize(cs->charset_to_unicode.csconvert_fn_convert(
-			&cs->charset_to_unicode, map.current()->first.length(), NULL, 0, NULL, &errCode, &errPosition));
+			&cs->charset_to_unicode, accessor.current()->first.length(), NULL, 0, NULL, &errCode, &errPosition));
 		s1.resize(cs->charset_to_unicode.csconvert_fn_convert(
-			&cs->charset_to_unicode, map.current()->first.length(), (UCHAR*) map.current()->first.c_str(),
+			&cs->charset_to_unicode, accessor.current()->first.length(), (UCHAR*) accessor.current()->first.c_str(),
 			s1.getCapacity(), s1.begin(), &errCode, &errPosition));
 
 		s2.resize(cs->charset_to_unicode.csconvert_fn_convert(
-			&cs->charset_to_unicode, map.current()->second.length(), NULL, 0, NULL, &errCode, &errPosition));
+			&cs->charset_to_unicode, accessor.current()->second.length(), NULL, 0, NULL, &errCode, &errPosition));
 		s2.resize(cs->charset_to_unicode.csconvert_fn_convert(
-			&cs->charset_to_unicode, map.current()->second.length(), (UCHAR*) map.current()->second.c_str(),
+			&cs->charset_to_unicode, accessor.current()->second.length(), (UCHAR*) accessor.current()->second.c_str(),
 			s2.getCapacity(), s2.begin(), &errCode, &errPosition));
 
 		map16.put(string((char*) s1.begin(), s1.getCount()), string((char*) s2.begin(), s2.getCount()));
 
-		found = map.getNext();
+		found = accessor.getNext();
 	}
 
 	UnicodeUtil::Utf16Collation* collation =

src/jrd/cmp.cpp

@@ -2031,26 +2031,27 @@ jrd_req* CMP_make_request(thread_db* tdbb, CompilerScratch* csb, bool internal_f
 	csb->csb_node = CMP_pass1(tdbb, csb, csb->csb_node);
 
 	// Copy and compile (pass1) domains DEFAULT and constraints.
-	bool found = csb->csb_map_field_info.getFirst();
-	while (found)
 	{
-		FieldInfo& fieldInfo = csb->csb_map_field_info.current()->second;
+		MapFieldInfo::Accessor accessor(&csb->csb_map_field_info);
-		UCHAR local_map[MAP_LENGTH];
 
-		AutoSetRestore<USHORT> autoRemapVariable(&csb->csb_remap_variable,
+		for (bool found = accessor.getFirst(); found; found = accessor.getNext())
-			(csb->csb_variables ? csb->csb_variables->count() : 0) + 1);
+		{
+			FieldInfo& fieldInfo = accessor.current()->second;
+			UCHAR local_map[MAP_LENGTH];
 
-		fieldInfo.defaultValue = copy(tdbb, csb, fieldInfo.defaultValue, local_map, 0, NULL, false);
+			AutoSetRestore<USHORT> autoRemapVariable(&csb->csb_remap_variable,
+				(csb->csb_variables ? csb->csb_variables->count() : 0) + 1);
 
-		csb->csb_remap_variable = (csb->csb_variables ? csb->csb_variables->count() : 0) + 1;
+			fieldInfo.defaultValue = copy(tdbb, csb, fieldInfo.defaultValue, local_map, 0, NULL, false);
 
-		fieldInfo.validation = copy(tdbb, csb, fieldInfo.validation, local_map, 0, NULL, false);
+			csb->csb_remap_variable = (csb->csb_variables ? csb->csb_variables->count() : 0) + 1;
 
-		fieldInfo.defaultValue = CMP_pass1(tdbb, csb, fieldInfo.defaultValue);
+			fieldInfo.validation = copy(tdbb, csb, fieldInfo.validation, local_map, 0, NULL, false);
-		fieldInfo.validation = CMP_pass1(tdbb, csb, fieldInfo.validation);
 
-		found = csb->csb_map_field_info.getNext();
+			fieldInfo.defaultValue = CMP_pass1(tdbb, csb, fieldInfo.defaultValue);
-	}
+			fieldInfo.validation = CMP_pass1(tdbb, csb, fieldInfo.validation);
+		}
+	} // scope
 
 	csb->csb_impure = REQ_SIZE + REQ_TAIL * MAX(csb->csb_n_stream, 1);
 	csb->csb_exec_sta.clear();
@@ -2058,16 +2059,17 @@ jrd_req* CMP_make_request(thread_db* tdbb, CompilerScratch* csb, bool internal_f
 	csb->csb_node = CMP_pass2(tdbb, csb, csb->csb_node, 0);
 
 	// Compile (pass2) domains DEFAULT and constraints
-	found = csb->csb_map_field_info.getFirst();
-	while (found)
 	{
-		FieldInfo& fieldInfo = csb->csb_map_field_info.current()->second;
+		MapFieldInfo::Accessor accessor(&csb->csb_map_field_info);
 
-		fieldInfo.defaultValue = CMP_pass2(tdbb, csb, fieldInfo.defaultValue, 0);
+		for (bool found = accessor.getFirst(); found; found = accessor.getNext())
-		fieldInfo.validation = CMP_pass2(tdbb, csb, fieldInfo.validation, 0);
+		{
+			FieldInfo& fieldInfo = accessor.current()->second;
 
-		found = csb->csb_map_field_info.getNext();
+			fieldInfo.defaultValue = CMP_pass2(tdbb, csb, fieldInfo.defaultValue, 0);
-	}
+			fieldInfo.validation = CMP_pass2(tdbb, csb, fieldInfo.validation, 0);
+		}
+	} // scope
 
 	if (csb->csb_impure > MAX_REQUEST_SIZE) {
 		IBERROR(226);			// msg 226 request size limit exceeded

src/jrd/jrd.cpp

@@ -4888,10 +4888,10 @@ static void release_attachment(thread_db* tdbb, Attachment* attachment, ISC_STAT
 	if (attachment->att_temp_pg_lock)
 		LCK_release(tdbb, attachment->att_temp_pg_lock);
 
-	for (bool getResult = attachment->att_dsql_cache.getFirst(); getResult;
+	DSqlCache::Accessor accessor(&attachment->att_dsql_cache);
-			  getResult = attachment->att_dsql_cache.getNext())
+	for (bool getResult = accessor.getFirst(); getResult; getResult = accessor.getNext())
 	{
-		LCK_release(tdbb, attachment->att_dsql_cache.current()->second.lock);
+		LCK_release(tdbb, accessor.current()->second.lock);
 	}
 #endif
 

src/jrd/jrd.h

@@ -228,6 +228,9 @@ struct DSqlCacheItem
 	bool obsolete;
 };
 
+typedef Firebird::GenericMap<Firebird::Pair<Firebird::Left<
+	Firebird::string, DSqlCacheItem> > > DSqlCache;
+
 
 //
 // the attachment block; one is created for each attachment to a database
@@ -330,8 +333,7 @@ public:
 #ifndef SUPERSERVER
 	Lock*		att_temp_pg_lock;			// temporary pagespace ID lock
 #endif
-	Firebird::GenericMap<Firebird::Pair<Firebird::Left<
+	DSqlCache att_dsql_cache;	// DSQL cache locks
-		Firebird::string, DSqlCacheItem> > > att_dsql_cache;	// DSQL cache locks
 	Firebird::SortedArray<void*> att_udf_pointers;
 	dsql_dbb* att_dsql_instance;
 	Firebird::Mutex att_mutex;				// attachment mutex

src/jrd/met.epp

@@ -4192,7 +4192,9 @@ static DSqlCacheItem* get_dsql_cache_item(thread_db* tdbb, int type, const Fireb
 		memcpy(item->lock->lck_key.lck_string, key.c_str(), key.length());
 	}
 	else
-		item = &attachment->att_dsql_cache.current()->second;
+	{
+		item = attachment->att_dsql_cache.get(key);
+	}
 
 	return item;
 }

src/jrd/unicode_util.cpp

@@ -117,6 +117,8 @@ public:
 // cache ICU module instances to not load and unload many times
 class UnicodeUtil::ICUModules
 {
+	typedef GenericMap<Pair<Left<string, ICU*> > > ModulesMap;
+
 public:
 	explicit ICUModules(MemoryPool&)
 	{
@@ -124,11 +126,12 @@ public:
 
 	~ICUModules()
 	{
-		for (bool found = modules().getFirst(); found; found = modules().getNext())
+		ModulesMap::Accessor modulesAccessor(&modules());
-			delete modules().current()->second;
+		for (bool found = modulesAccessor.getFirst(); found; found = modulesAccessor.getNext())
+			delete modulesAccessor.current()->second;
 	}
 
-	InitInstance<GenericMap<Pair<Left<string, ICU*> > > > modules;
+	InitInstance<ModulesMap> modules;
 	RWLock lock;
 };