VBufStorage: do not delete reference nodes prematurely when replacing…

… subtrees (#10188)

* VBufStorage_buffer_t::replaceSubtrees: move all referenced nodes in a separate loop, before removing the replaced nodes from the backend. Otherwise, some referenced nodes might be prematurely deleted.

* VBufBackend_t::reuseExistingNodeInRender: remove the limitation that a node must be part of the same parent to be reused. This code did not handle descendants and the true underlying cause of crashes has been dealt with in vbufStorage_buffer_t::replaceSubtrees now.

* VBufBackend_t::reuseExistingNodeInRender: correct comment.

* VBufStorage_buffer_t::replaceSubtrees: rename some variables for better understanding and to avoid shadowing variables from an outer scope.

nvdaHelper/vbufBase/backend.cpp

@@ -276,18 +276,12 @@ VBufStorage_controlFieldNode_t* VBufBackend_t::reuseExistingNodeInRender(VBufSto
 		LOG_DEBUG(L"Existing node refuses to be reused");
 		return nullptr;
 	}
-	// We only allow reusing nodes that share the same parent. 
-	// I.e. we don't allow reusing a node that existed in an entirely different part of the tree. 
-	// If we did, this could possibly cause corruption in the virtualBuffer as a node might move between two unrelated updates.
+	// Don't reuse the node if it has no parent (is the root of the buffer).
 	auto existingParent=existingNode->getParent();
 	if(!existingParent) {
 		LOG_DEBUG(L"existing node has no parent. Not reusing.");
 		return nullptr;
 	}
-	if(existingParent->identifier!=parent->identifier) {
-		LOG_DEBUG(L"Cannot reuse a node moved from within another parent.");
-		return nullptr;
-	}
 	if(existingNode->denyReuseIfPreviousSiblingsChanged) {
 		// This node is not allowed to be reused if any of its previous siblings have changed.
 		// We work this out by walking back to the previous controlFieldNode in its siblings, and ensuring that it is a reference node that references the existing node's first previous controlFieldNode.

nvdaHelper/vbufBase/storage.cpp

@@ -642,9 +642,25 @@ bool VBufStorage_buffer_t::replaceSubtrees(map<VBufStorage_fieldNode_t*,VBufStor
 			for(previous=parent->previous;previous!=NULL;relativeSelectionStart+=previous->length,previous=previous->previous);
 		}
 	}
+	// For each buffer in the map,
+	// Reverse iterate over all reference nodes, replacing them with the existing nodes in the original buffer they point to.
+	// We must iterate in reverse as new nodes are always inserted using parent and previous as the location,
+	// iterating forward would cause a future reference node's previous to be come invalid as it had been replaced. 
+	for(auto subtreeEntryIter=m.cbegin();subtreeEntryIter!=m.cend();++subtreeEntryIter) {
+		auto node=subtreeEntryIter->first;
+		auto buffer=subtreeEntryIter->second;
+		for(auto referenceNodeIter=buffer->referenceNodes.rbegin();referenceNodeIter!=buffer->referenceNodes.rend();++referenceNodeIter) {
+			auto parent=(*referenceNodeIter)->parent;
+			auto previous=(*referenceNodeIter)->previous;
+			auto referenced=(*referenceNodeIter)->referenceNode;
+			buffer->removeFieldNode(*referenceNodeIter);
+			this->unlinkFieldNode(referenced);
+			buffer->insertNode(parent,previous,referenced);
+		}
+	}
 	//For each node in the map,
 	//Replace the node on this buffer, with the content of the buffer in the map for that node
-	//Note that controlField info will automatically be removed, but not added again
+	//Note that controlField info will automatically be removed, but not added again.
 	bool failedBuffers=false;
 	for(map<VBufStorage_fieldNode_t*,VBufStorage_buffer_t*>::iterator i=m.begin();i!=m.end();) {
 		VBufStorage_fieldNode_t* node=i->first;
@@ -655,17 +671,6 @@ bool VBufStorage_buffer_t::replaceSubtrees(map<VBufStorage_fieldNode_t*,VBufStor
 			m.erase(i++);
 			continue;
 		}
-		// Reverse iterate over all reference nodes, replacing them with the existing nodes in the original buffer they point to.
-		// We must iterate in reverse as new nodes are always inserted using parent and previous as the location,
-		 // iterating forward would cause a future reference node's previous to be come invalid as it had been replaced. 
-		for(auto i=buffer->referenceNodes.rbegin();i!=buffer->referenceNodes.rend();++i) {
-			VBufStorage_controlFieldNode_t* parent=(*i)->parent;
-			VBufStorage_fieldNode_t* previous=(*i)->previous;
-			VBufStorage_controlFieldNode_t* referenced=(*i)->referenceNode;
-			buffer->removeFieldNode(*i);
-			this->unlinkFieldNode(referenced);
-			buffer->insertNode(parent,previous,referenced);
-		}
 		parent=node->parent;
 		previous=node->previous;
 		if(!this->removeFieldNode(node)) {