Fix various edge cases in extensible synchronisation.

- Fix not forwarding the accountid extensible if it is set.
- Rename the variadic Set() overload to SetFwd().
- Re-add the `const T&` overload of Set().
- Move `bool synced` to SimpleExtItem from StringExtItem.
- Only sync extensibles if their instance is marked as syncable.

10 files changed, 34 insertions, 30 deletions

diff --git a/include/extensible.h b/include/extensible.h
index a416f91a4..0e86f8272 100644
--- a/include/extensible.h
+++ b/include/extensible.h
@@ -191,14 +191,20 @@ template <typename T, typename Del = std::default_delete<T>>
 class SimpleExtItem
 	: public ExtensionItem
 {
+ protected:
+	/** Whether to sync this StringExtItem across the network. */
+	bool synced;
+
  public:
 	/** Initializes an instance of the SimpleExtItem class.
 	 * @param parent The module which created this SimpleExtItem.
 	 * @param Key The name of the extension item (e.g. foo_bar).
 	 * @param exttype The type of Extensible that this SimpleExtItem applies to.
+	 * @param sync Whether this SimpleExtItem should be broadcast to other servers.
 	 */
-	SimpleExtItem(Module* parent, const std::string& Key, ExtensionType exttype)
+	SimpleExtItem(Module* parent, const std::string& Key, ExtensionType exttype, bool sync = false)
 		: ExtensionItem(parent, Key, exttype)
+		, synced(sync)
 	{
 	}
 
@@ -211,20 +217,28 @@ class SimpleExtItem
 	{
 		T* old = static_cast<T*>(SetRaw(container, value));
 		Delete(container, old);
-		if (sync)
+		if (sync && synced)
 			Sync(container, value);
 	}
 
+	inline void Set(Extensible* container, const T& value, bool sync = true)
+	{
+		Set(container, new T(value), sync);
+	}
+
 	template <typename... Args>
-	inline void Set(Extensible* container, Args&&... args)
+	inline void SetFwd(Extensible* container, Args&&... args)
 	{
-		Set(container, new T(std::forward<Args>(args)...));
+		// Forwarded arguments are for complex types which are assumed to not
+		// be synced across the network. You can manually call Sync() if this
+		// is not the case.
+		Set(container, new T(std::forward<Args>(args)...), false);
 	}
 
 	inline void Unset(Extensible* container, bool sync = true)
 	{
 		Delete(container, UnsetRaw(container));
-		if (sync)
+		if (synced && sync)
 			Sync(container, nullptr);
 	}
 
@@ -239,10 +253,6 @@ class SimpleExtItem
 class CoreExport StringExtItem
 	: public SimpleExtItem<std::string>
 {
- protected:
-	/** Whether to sync this StringExtItem across the network. */
-	bool synced;
-
  public:
 	/** Initializes an instance of the StringExtItem class.
 	 * @param owner The module which created this StringExtItem.
@@ -300,7 +310,7 @@ class CoreExport IntExtItem
 	/** Sets a value for this IntExtItem.
 	 * @param container A container that the IntExtItem should be set on.
 	 * @param value The new value for this IntExtItem.
-	 * @param sync Whether to sync this value to other servers.
+	 * @param sync If syncable then whether to sync this value to other servers.
 	 */
 	void Set(Extensible* container, intptr_t value, bool sync = true);
 
@@ -312,7 +322,7 @@ class CoreExport IntExtItem
 
 	/** Removes the value for this IntExtItem.
 	 * @param container A container the ExtensionItem should be removed from.
-	 * @param sync Whether to sync this unset to the network.
+	 * @param sync If syncable then whether to sync this unset to the network.
 	 */
 	void Unset(Extensible* container, bool sync = true);
 };
@@ -360,13 +370,13 @@ class CoreExport BoolExtItem
 
 	/** Sets a value for this BoolExtItem.
 	 * @param container A container that the BoolExtItem should be set on.
-	 * @param sync Whether to sync this set to the network.
+	 * @param sync If syncable then whether to sync this set to the network.
 	 */
 	void Set(Extensible* container, bool sync = true);
 
 	/** Removes the value for this BoolExtItem.
 	 * @param container A container the ExtensionItem should be removed from.
-	 * @param sync Whether to sync this unset to the network.
+	 * @param sync If syncable then whether to sync this unset to the network.
 	 */
 	void Unset(Extensible* container, bool sync = true);
 };
diff --git a/src/extensible.cpp b/src/extensible.cpp
index 641d62337..1e02fc526 100644
--- a/src/extensible.cpp
+++ b/src/extensible.cpp
@@ -232,14 +232,14 @@ bool BoolExtItem::Get(const Extensible* container) const
 void BoolExtItem::Set(Extensible* container, bool sync)
 {
 	SetRaw(container, reinterpret_cast<void*>(1));
-	if (sync)
+	if (sync && synced)
 		Sync(container, reinterpret_cast<void*>(1));
 }
 
 void BoolExtItem::Unset(Extensible* container, bool sync)
 {
 	UnsetRaw(container);
-	if (sync)
+	if (sync && synced)
 		Sync(container, reinterpret_cast<void*>(0));
 }
 
@@ -299,8 +299,7 @@ void IntExtItem::Unset(Extensible* container, bool sync)
 }
 
 StringExtItem::StringExtItem(Module* owner, const std::string& key, ExtensionType exttype, bool sync)
-	: SimpleExtItem(owner, key, exttype)
-	, synced(sync)
+	: SimpleExtItem(owner, key, exttype, sync)
 {
 }
 
diff --git a/src/modules/m_anticaps.cpp b/src/modules/m_anticaps.cpp
index 1f6ff0747..3d8f2c8ba 100644
--- a/src/modules/m_anticaps.cpp
+++ b/src/modules/m_anticaps.cpp
@@ -120,7 +120,7 @@ class AntiCapsMode final
 			return MODEACTION_DENY;
 		}
 
-		ext.Set(channel, method, minlen, percent);
+		ext.SetFwd(channel, method, minlen, percent);
 		return MODEACTION_ALLOW;
 	}
 
diff --git a/src/modules/m_chanhistory.cpp b/src/modules/m_chanhistory.cpp
index ab95da200..32768a0b2 100644
--- a/src/modules/m_chanhistory.cpp
+++ b/src/modules/m_chanhistory.cpp
@@ -125,7 +125,7 @@ class HistoryMode final
 		}
 		else
 		{
-			ext.Set(channel, len, time);
+			ext.SetFwd(channel, len, time);
 		}
 		return MODEACTION_ALLOW;
 	}
diff --git a/src/modules/m_conn_join.cpp b/src/modules/m_conn_join.cpp
index fdf20ab24..4185c7987 100644
--- a/src/modules/m_conn_join.cpp
+++ b/src/modules/m_conn_join.cpp
@@ -112,7 +112,7 @@ class ModuleConnJoin final
 		if (!chandelay)
 			JoinChannels(localuser, chanlist);
 		else
-			ext.Set(localuser, localuser, ext, chanlist, chandelay);
+			ext.SetFwd(localuser, localuser, ext, chanlist, chandelay);
 	}
 };
 
diff --git a/src/modules/m_joinflood.cpp b/src/modules/m_joinflood.cpp
index ae29811e3..2fff26377 100644
--- a/src/modules/m_joinflood.cpp
+++ b/src/modules/m_joinflood.cpp
@@ -123,7 +123,7 @@ class JoinFlood final
 			return MODEACTION_DENY;
 		}
 
-		ext.Set(channel, nsecs, njoins);
+		ext.SetFwd(channel, nsecs, njoins);
 		return MODEACTION_ALLOW;
 	}
 
diff --git a/src/modules/m_messageflood.cpp b/src/modules/m_messageflood.cpp
index a71497f70..65d7dfba7 100644
--- a/src/modules/m_messageflood.cpp
+++ b/src/modules/m_messageflood.cpp
@@ -97,7 +97,7 @@ class MsgFlood final
 			return MODEACTION_DENY;
 		}
 
-		ext.Set(channel, ban, nsecs, nlines);
+		ext.SetFwd(channel, ban, nsecs, nlines);
 		return MODEACTION_ALLOW;
 	}
 
diff --git a/src/modules/m_nickflood.cpp b/src/modules/m_nickflood.cpp
index 9475d8077..14c238947 100644
--- a/src/modules/m_nickflood.cpp
+++ b/src/modules/m_nickflood.cpp
@@ -112,7 +112,7 @@ class NickFlood final
 			return MODEACTION_DENY;
 		}
 
-		ext.Set(channel, nsecs, nnicks);
+		ext.SetFwd(channel, nsecs, nnicks);
 		return MODEACTION_ALLOW;
 	}
 
diff --git a/src/modules/m_sasl.cpp b/src/modules/m_sasl.cpp
index 05653a927..122b34b43 100644
--- a/src/modules/m_sasl.cpp
+++ b/src/modules/m_sasl.cpp
@@ -345,7 +345,7 @@ class CommandAuthenticate final
 
 			SaslAuthenticator *sasl = authExt.Get(user);
 			if (!sasl)
-				authExt.Set(user, user, parameters[0], sslapi);
+				authExt.SetFwd(user, user, parameters[0], sslapi);
 			else if (sasl->SendClientMessage(parameters) == false)	// IAL abort extension --nenolod
 			{
 				sasl->AnnounceState();
diff --git a/src/modules/m_services_account.cpp b/src/modules/m_services_account.cpp
index 8a97a83eb..b5d5f22a6 100644
--- a/src/modules/m_services_account.cpp
+++ b/src/modules/m_services_account.cpp
@@ -103,11 +103,6 @@ class AccountExtItemImpl final
 	{
 	}
 
-	void FromInternal(Extensible* container, const std::string& value) noexcept override
-	{
-		StringExtItem::FromInternal(container, value);
-	}
-
 	void FromNetwork(Extensible* container, const std::string& value) noexcept override
 	{
 		StringExtItem::FromNetwork(container, value);
@@ -201,7 +196,7 @@ class ModuleServicesAccount final
 		, regdeafmode(this, "regdeaf", 'R')
 		, chanregmode(this)
 		, userregmode(this)
-		, accountid(this, "accountid", ExtensionType::USER)
+		, accountid(this, "accountid", ExtensionType::USER, true)
 		, accountname(this)
 		, accountextban(this, accountname)
 		, unauthedextban(this, accountname)