Fix iterator invalidation issues in CargoPacketList actions

See: #48

src/cargoaction.cpp

@@ -215,22 +215,26 @@ bool StationCargoReroute::operator()(CargoPacket *cp)
 /**
  * Reroutes some cargo in a VehicleCargoList.
  * @param cp Packet to be rerouted.
+ * @param front_insert Front insert list.
  * @return True if the packet was completely rerouted, false if part of it was.
  */
-bool VehicleCargoReroute::operator()(CargoPacket *cp)
+bool VehicleCargoReroute::operator()(CargoPacket *cp, std::vector<CargoPacket *> &front_insert)
 {
 	CargoPacket *cp_new = this->Preprocess(cp);
 	if (cp_new == NULL) cp_new = cp;
 	if (cp_new->NextStation() == this->avoid || cp_new->NextStation() == this->avoid2) {
 		cp->SetNextStation(this->ge->GetVia(cp_new->SourceStation(), this->avoid, this->avoid2));
 	}
-	if (this->source != this->destination) {
+	if (unlikely(this->source != this->destination)) {
 		this->source->RemoveFromMeta(cp_new, VehicleCargoList::MTA_TRANSFER, cp_new->Count());
 		this->destination->AddToMeta(cp_new, VehicleCargoList::MTA_TRANSFER);
 	}
 
-	/* Legal, as front pushing doesn't invalidate iterators in std::list. */
-	this->destination->packets.push_front(cp_new);
+	if (likely(this->source == this->destination)) {
+		front_insert.push_back(cp_new);
+	} else {
+		this->destination->packets.push_front(cp_new);
+	}
 	return cp_new == cp;
 }
 

src/cargoaction.h

@@ -13,6 +13,7 @@
 #define CARGOACTION_H
 
 #include "cargopacket.h"
+#include <vector>
 
 /**
  * Abstract action of removing cargo from a vehicle or a station.
@@ -140,7 +141,7 @@ public:
 	{
 		assert(this->max_move <= source->ActionCount(VehicleCargoList::MTA_TRANSFER));
 	}
-	bool operator()(CargoPacket *cp);
+	bool operator()(CargoPacket *cp, std::vector<CargoPacket *> &front_insert);
 };
 
 #endif /* CARGOACTION_H */

src/cargopacket.cpp

@@ -445,6 +445,35 @@ void VehicleCargoList::ShiftCargo(Taction action)
 	}
 }
 
+/**
+ * Shifts cargo from the front of the packet list and applies some action to it.
+ * @tparam Taction Action class or function to be used. It should define
+ *                 "bool operator()(CargoPacket *, std::vector<CargoPacket*> &)". If true is returned the
+ *                 cargo packet will be removed from the list. Otherwise it
+ *                 will be kept and the loop will be aborted.
+ *                 The second method parameter can be appended to, to prepend items to the packet list
+ * @param action Action instance to be applied.
+ */
+template<class Taction>
+void VehicleCargoList::ShiftCargoWithFrontInsert(Taction action)
+{
+	std::vector<CargoPacket *> packets_to_front_insert;
+
+	Iterator it(this->packets.begin());
+	while (it != this->packets.end() && action.MaxMove() > 0) {
+		CargoPacket *cp = *it;
+		if (action(cp, packets_to_front_insert)) {
+			it = this->packets.erase(it);
+		} else {
+			break;
+		}
+	}
+
+	for (CargoPacket *cp : packets_to_front_insert) {
+		this->packets.push_front(cp);
+	}
+}
+
 /**
  * Pops cargo from the back of the packet list and applies some action to it.
  * @tparam Taction Action class or function to be used. It should define
@@ -732,7 +761,11 @@ uint VehicleCargoList::Reassign<VehicleCargoList::MTA_DELIVER, VehicleCargoList:
 		if (sum > this->action_counts[MTA_TRANSFER] + max_move) {
 			CargoPacket *cp_split = cp->Split(sum - this->action_counts[MTA_TRANSFER] + max_move);
 			sum -= cp_split->Count();
-			this->packets.insert(it, cp_split);
+			it = this->packets.insert(it, cp_split);
+			/* it points to the inserted value, which is just before the previous value of it.
+			 * Increment it so that it points to the same element as it did before the insert.
+			 */
+			++it;
 		}
 		cp->next_station = next_station;
 	}
@@ -818,7 +851,7 @@ uint VehicleCargoList::Truncate(uint max_move)
 uint VehicleCargoList::Reroute(uint max_move, VehicleCargoList *dest, StationID avoid, StationID avoid2, const GoodsEntry *ge)
 {
 	max_move = min(this->action_counts[MTA_TRANSFER], max_move);
-	this->ShiftCargo(VehicleCargoReroute(this, dest, max_move, avoid, avoid2, ge));
+	this->ShiftCargoWithFrontInsert(VehicleCargoReroute(this, dest, max_move, avoid, avoid2, ge));
 	return max_move;
 }
 

src/cargopacket.h

@@ -303,6 +303,9 @@ protected:
 	template<class Taction>
 	void ShiftCargo(Taction action);
 
+	template<class Taction>
+	void ShiftCargoWithFrontInsert(Taction action);
+
 	template<class Taction>
 	void PopCargo(Taction action);