Remove all external usage of CmptPrivate. Header cleanup.

This removes usage of CmptPrivate outside of ScriptInterface.
ScriptRequest can now be used to safely recover the scriptInterface from
a JSContext instead of going through ScriptInterface, which allows more
code cleanup.

Follows 34b1920e7b

Differential Revision: https://code.wildfiregames.com/D3963
This was SVN commit r25442.

source/scriptinterface/FunctionWrapper.h

@@ -21,9 +21,13 @@
 #include "Object.h"
 #include "ScriptConversions.h"
 #include "ScriptExceptions.h"
-#include "ScriptInterface.h"
 #include "ScriptRequest.h"
 
+#include <tuple>
+#include <type_traits>
+
+class ScriptInterface;
+
 /**
  * This class introduces templates to conveniently wrap C++ functions in JSNative functions.
  * This _is_ rather template heavy, so compilation times beware.
@@ -128,14 +132,14 @@ private:
 	/**
 	 * ConvertFromJS is a wrapper around DoConvertFromJS, and serves to:
 	 *  - unwrap the tuple types as a parameter pack
-	 *  - handle specific cases for the first argument (cmptPrivate, ScriptRequest).
+	 *  - handle specific cases for the first argument (ScriptRequest, ...).
 	 *
 	 * Trick: to unpack the types of the tuple as a parameter pack, we deduce them from the function signature.
 	 * To do that, we want the tuple in the arguments, but we don't want to actually have to default-instantiate,
 	 * so we'll pass a nullptr that's static_cast to what we want.
 	 */
 	template<typename ...Types>
-	static std::tuple<Types...> ConvertFromJS(ScriptInterface::CmptPrivate*, const ScriptRequest& rq, JS::CallArgs& args, bool& went_ok, std::tuple<Types...>*)
+	static std::tuple<Types...> ConvertFromJS(const ScriptRequest& rq, JS::CallArgs& args, bool& went_ok, std::tuple<Types...>*)
 	{
 		if constexpr (sizeof...(Types) == 0)
 		{
@@ -147,23 +151,9 @@ private:
 			return DoConvertFromJS<0, Types...>(rq, args, went_ok);
 	}
 
-	// Overloads for CmptPrivate* first argument.
-	template<typename ...Types>
-	static std::tuple<ScriptInterface::CmptPrivate*, Types...> ConvertFromJS(ScriptInterface::CmptPrivate* cmptPrivate, const ScriptRequest& rq, JS::CallArgs& args, bool& went_ok, std::tuple<ScriptInterface::CmptPrivate*, Types...>*)
-	{
-		if constexpr (sizeof...(Types) == 0)
-		{
-			// GCC (at least < 9) & VS17 prints warnings if arguments are not used in some constexpr branch.
-			UNUSED2(rq); UNUSED2(args); UNUSED2(went_ok);
-			return std::forward_as_tuple(cmptPrivate);
-		}
-		else
-			return std::tuple_cat(std::forward_as_tuple(cmptPrivate), DoConvertFromJS<0, Types...>(rq, args, went_ok));
-	}
-
 	// Overloads for ScriptRequest& first argument.
 	template<typename ...Types>
-	static std::tuple<const ScriptRequest&, Types...> ConvertFromJS(ScriptInterface::CmptPrivate*, const ScriptRequest& rq, JS::CallArgs& args, bool& went_ok, std::tuple<const ScriptRequest&, Types...>*)
+	static std::tuple<const ScriptRequest&, Types...> ConvertFromJS(const ScriptRequest& rq, JS::CallArgs& args, bool& went_ok, std::tuple<const ScriptRequest&, Types...>*)
 	{
 		if constexpr (sizeof...(Types) == 0)
 		{
@@ -177,16 +167,16 @@ private:
 
 	// Overloads for ScriptInterface& first argument.
 	template<typename ...Types>
-	static std::tuple<const ScriptInterface&, Types...> ConvertFromJS(ScriptInterface::CmptPrivate* cmptPrivate, const ScriptRequest& rq, JS::CallArgs& args, bool& went_ok, std::tuple<const ScriptInterface&, Types...>*)
+	static std::tuple<const ScriptInterface&, Types...> ConvertFromJS(const ScriptRequest& rq, JS::CallArgs& args, bool& went_ok, std::tuple<const ScriptInterface&, Types...>*)
 	{
 		if constexpr (sizeof...(Types) == 0)
 		{
 			// GCC (at least < 9) & VS17 prints warnings if arguments are not used in some constexpr branch.
 			UNUSED2(rq); UNUSED2(args); UNUSED2(went_ok);
-			return std::forward_as_tuple(*cmptPrivate->pScriptInterface);
+			return std::forward_as_tuple(rq.GetScriptInterface());
 		}
 		else
-			return std::tuple_cat(std::forward_as_tuple(*cmptPrivate->pScriptInterface), DoConvertFromJS<0, Types...>(rq, args, went_ok));
+			return std::tuple_cat(std::forward_as_tuple(rq.GetScriptInterface()), DoConvertFromJS<0, Types...>(rq, args, went_ok));
 	}
 
 	///////////////////////////////////////////////////////////////////////////
@@ -289,7 +279,7 @@ public:
 	 * so that it can be called from JS and manipulated in Spidermonkey.
 	 * Most C++ functions can be directly wrapped, so long as their arguments are
 	 * convertible from JS::Value and their return value is convertible to JS::Value (or void)
-	 * The C++ function may optionally take const ScriptRequest& or CmptPrivate* as its first argument.
+	 * The C++ function may optionally take const ScriptRequest& or ScriptInterface& as its first argument.
 	 * The function may be an object method, in which case you need to pass an appropriate getter
 	 *
 	 * Optimisation note: the ScriptRequest object is created even without arguments,
@@ -303,8 +293,7 @@ public:
 		using ObjType = typename args_info<decltype(callable)>::object_type;
 
 		JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
-		ScriptInterface* scriptInterface = ScriptInterface::GetScriptInterfaceAndCBData(cx)->pScriptInterface;
-		ScriptRequest rq(*scriptInterface);
+		ScriptRequest rq(cx);
 
 		// If the callable is an object method, we must specify how to fetch the object.
 		static_assert(std::is_same_v<typename args_info<decltype(callable)>::object_type, void> || thisGetter != nullptr,
@@ -327,7 +316,7 @@ public:
 #endif
 
 		bool went_ok = true;
-		typename args_info<decltype(callable)>::arg_types outs = ConvertFromJS(ScriptInterface::GetScriptInterfaceAndCBData(cx), rq, args, went_ok, static_cast<typename args_info<decltype(callable)>::arg_types*>(nullptr));
+		typename args_info<decltype(callable)>::arg_types outs = ConvertFromJS(rq, args, went_ok, static_cast<typename args_info<decltype(callable)>::arg_types*>(nullptr));
 		if (!went_ok)
 			return false;
 
@@ -412,15 +401,6 @@ public:
 	{
 		JS_DefineFunction(cx, scope, name, &ToJSNative<callable, thisGetter>, args_info<decltype(callable)>::nb_args, flags);
 	}
-
-	/**
-	 * Convert the CmptPrivate callback data to T*
-	 */
-	template <typename T>
-	static T* ObjectFromCBData(const ScriptRequest& rq, JS::CallArgs&)
-	{
-		return static_cast<T*>(ScriptInterface::GetScriptInterfaceAndCBData(rq.cx)->pCBData);
-	}
 };
 
 #endif // INCLUDED_FUNCTIONWRAPPER

source/scriptinterface/ScriptInterface.cpp

@@ -52,7 +52,6 @@
  * directly accessing the underlying JS api.
  */
 
-
 struct ScriptInterface_impl
 {
 	ScriptInterface_impl(const char* nativeScopeName, const shared_ptr<ScriptContext>& context);
@@ -76,14 +75,21 @@ struct ScriptInterface_impl
  * Constructor for ScriptRequest - here because it needs access into ScriptInterface_impl.
  */
 ScriptRequest::ScriptRequest(const ScriptInterface& scriptInterface) :
-	cx(scriptInterface.m->m_cx), glob(scriptInterface.m->m_glob), nativeScope(scriptInterface.m->m_nativeScope)
+	cx(scriptInterface.m->m_cx),
+	glob(scriptInterface.m->m_glob),
+	nativeScope(scriptInterface.m->m_nativeScope),
+	m_ScriptInterface(scriptInterface)
 {
-	m_formerRealm = JS::EnterRealm(cx, scriptInterface.m->m_glob);
+	m_FormerRealm = JS::EnterRealm(cx, scriptInterface.m->m_glob);
 }
 
 ScriptRequest::~ScriptRequest()
 {
-	JS::LeaveRealm(cx, m_formerRealm);
+	JS::LeaveRealm(cx, m_FormerRealm);
+}
+
+ScriptRequest::ScriptRequest(JSContext* cx) : ScriptRequest(ScriptInterface::CmptPrivate::GetScriptInterface(cx))
+{
 }
 
 JS::Value ScriptRequest::globalValue() const
@@ -91,6 +97,10 @@ JS::Value ScriptRequest::globalValue() const
 	return JS::ObjectValue(*glob);
 }
 
+const ScriptInterface& ScriptRequest::GetScriptInterface() const
+{
+	return m_ScriptInterface;
+}
 
 namespace
 {
@@ -112,7 +122,7 @@ JSClass global_class = {
 bool print(JSContext* cx, uint argc, JS::Value* vp)
 {
 	JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
-	ScriptRequest rq(*ScriptInterface::GetScriptInterfaceAndCBData(cx)->pScriptInterface); \
+	ScriptRequest rq(cx);
 
 	for (uint i = 0; i < args.length(); ++i)
 	{
@@ -135,7 +145,7 @@ bool logmsg(JSContext* cx, uint argc, JS::Value* vp)
 		return true;
 	}
 
-	ScriptRequest rq(*ScriptInterface::GetScriptInterfaceAndCBData(cx)->pScriptInterface); \
+	ScriptRequest rq(cx);
 	std::wstring str;
 	if (!Script::FromJSVal(rq, args[0], str))
 		return false;
@@ -153,7 +163,7 @@ bool warn(JSContext* cx, uint argc, JS::Value* vp)
 		return true;
 	}
 
-	ScriptRequest rq(*ScriptInterface::GetScriptInterfaceAndCBData(cx)->pScriptInterface); \
+	ScriptRequest rq(cx);
 	std::wstring str;
 	if (!Script::FromJSVal(rq, args[0], str))
 		return false;
@@ -171,7 +181,7 @@ bool error(JSContext* cx, uint argc, JS::Value* vp)
 		return true;
 	}
 
-	ScriptRequest rq(*ScriptInterface::GetScriptInterfaceAndCBData(cx)->pScriptInterface); \
+	ScriptRequest rq(cx);
 	std::wstring str;
 	if (!Script::FromJSVal(rq, args[0], str))
 		return false;
@@ -271,24 +281,24 @@ static double generate_uniform_real(boost::rand48& rng, double min, double max)
 	}
 }
 
-bool Math_random(JSContext* cx, uint argc, JS::Value* vp)
-{
-	JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
-	double r;
-	if (!ScriptInterface::GetScriptInterfaceAndCBData(cx)->pScriptInterface->MathRandom(r))
-		return false;
+} // anonymous namespace
 
-	args.rval().setNumber(r);
+bool ScriptInterface::MathRandom(double& nbr) const
+{
+	if (m->m_rng == nullptr)
+		return false;
+	nbr = generate_uniform_real(*(m->m_rng), 0.0, 1.0);
 	return true;
 }
 
-} // anonymous namespace
-
-bool ScriptInterface::MathRandom(double& nbr)
+bool ScriptInterface::Math_random(JSContext* cx, uint argc, JS::Value* vp)
 {
-	if (m->m_rng == NULL)
+	JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
+	double r;
+	if (!ScriptInterface::CmptPrivate::GetScriptInterface(cx).MathRandom(r))
 		return false;
-	nbr = generate_uniform_real(*(m->m_rng), 0.0, 1.0);
+
+	args.rval().setNumber(r);
 	return true;
 }
 
@@ -356,18 +366,30 @@ ScriptInterface::~ScriptInterface()
 	}
 }
 
+const ScriptInterface& ScriptInterface::CmptPrivate::GetScriptInterface(JSContext *cx)
+{
+	CmptPrivate* pCmptPrivate = (CmptPrivate*)JS::GetRealmPrivate(JS::GetCurrentRealmOrNull(cx));
+	ENSURE(pCmptPrivate);
+	return *pCmptPrivate->pScriptInterface;
+}
+
+void* ScriptInterface::CmptPrivate::GetCBData(JSContext *cx)
+{
+	CmptPrivate* pCmptPrivate = (CmptPrivate*)JS::GetRealmPrivate(JS::GetCurrentRealmOrNull(cx));
+	return pCmptPrivate ? pCmptPrivate->pCBData : nullptr;
+}
+
 void ScriptInterface::SetCallbackData(void* pCBData)
 {
 	m_CmptPrivate.pCBData = pCBData;
 }
 
-ScriptInterface::CmptPrivate* ScriptInterface::GetScriptInterfaceAndCBData(JSContext* cx)
+template <>
+void* ScriptInterface::ObjectFromCBData<void>(const ScriptRequest& rq)
 {
-	CmptPrivate* pCmptPrivate = (CmptPrivate*)JS::GetRealmPrivate(JS::GetCurrentRealmOrNull(cx));
-	return pCmptPrivate;
+	return ScriptInterface::CmptPrivate::GetCBData(rq.cx);
 }
 
-
 bool ScriptInterface::LoadGlobalScripts()
 {
 	// Ignore this failure in tests

source/scriptinterface/ScriptInterface.h

@@ -73,6 +73,7 @@ class ScriptInterface
 	NONCOPYABLE(ScriptInterface);
 
 	friend class ScriptRequest;
+
 public:
 
 	/**
@@ -88,12 +89,36 @@ public:
 
 	struct CmptPrivate
 	{
+		friend class ScriptInterface;
+	public:
+		static const ScriptInterface& GetScriptInterface(JSContext* cx);
+		static void* GetCBData(JSContext* cx);
+	protected:
 		ScriptInterface* pScriptInterface; // the ScriptInterface object the compartment belongs to
 		void* pCBData; // meant to be used as the "this" object for callback functions
-	} m_CmptPrivate;
+	};
 
 	void SetCallbackData(void* pCBData);
-	static CmptPrivate* GetScriptInterfaceAndCBData(JSContext* cx);
+
+	/**
+	 * Convert the CmptPrivate callback data to T*
+	 */
+	template <typename T>
+	static T* ObjectFromCBData(const ScriptRequest& rq)
+	{
+		static_assert(!std::is_same_v<void, T>);
+		ScriptInterface::CmptPrivate::GetCBData(rq.cx);
+		return static_cast<T*>(ObjectFromCBData<void>(rq));
+	}
+
+	/**
+	 * Variant for the function wrapper.
+	 */
+	template <typename T>
+	static T* ObjectFromCBData(const ScriptRequest& rq, JS::CallArgs&)
+	{
+		return ObjectFromCBData<T>(rq);
+	}
 
 	/**
 	 * GetGeneralJSContext returns the context without starting a GC request and without
@@ -177,12 +202,14 @@ public:
 	template<typename T> bool Eval(const char* code, T& out) const;
 
 	/**
-	 * MathRandom (this function) calls the random number generator assigned to this ScriptInterface instance and
-	 * returns the generated number.
-	 * Math_random (with underscore, not this function) is a global function, but different random number generators can be
-	 * stored per ScriptInterface. It calls MathRandom of the current ScriptInterface instance.
+	 * Calls the random number generator assigned to this ScriptInterface instance and returns the generated number.
 	 */
-	bool MathRandom(double& nbr);
+	bool MathRandom(double& nbr) const;
+
+	/**
+	 * JSNative wrapper of the above.
+	 */
+	static bool Math_random(JSContext* cx, uint argc, JS::Value* vp);
 
 	/**
 	 * Retrieve the private data field of a JSObject that is an instance of the given JSClass.
@@ -230,6 +257,8 @@ private:
 		JSNative m_Constructor;
 	};
 
+	CmptPrivate m_CmptPrivate;
+
 	// Take care to keep this declaration before heap rooted members. Destructors of heap rooted
 	// members have to be called before the custom destructor of ScriptInterface_impl.
 	std::unique_ptr<ScriptInterface_impl> m;
@@ -237,6 +266,10 @@ private:
 	std::map<std::string, CustomType> m_CustomObjectTypes;
 };
 
+// Explicitly instantiate void* as that is used for the generic template,
+// and we want to define it in the .cpp file.
+template <> void* ScriptInterface::ObjectFromCBData(const ScriptRequest& rq);
+
 template<typename T>
 bool ScriptInterface::SetGlobal(const char* name, const T& value, bool replace, bool constant, bool enumerate)
 {

source/scriptinterface/ScriptRequest.h

@@ -71,12 +71,30 @@ public:
 	ScriptRequest(std::shared_ptr<ScriptInterface> scriptInterface) : ScriptRequest(*scriptInterface) {}
 	~ScriptRequest();
 
+	/**
+	 * Create a script request from a JSContext.
+	 * This can be used to get the script interface in a JSNative function.
+	 * In general, you shouldn't have to rely on this otherwise.
+	 */
+	ScriptRequest(JSContext* cx);
+
+	/**
+	 * Return the scriptInterface active when creating this ScriptRequest.
+	 * Note that this is multi-request safe: even if another ScriptRequest is created,
+	 * it will point to the original scriptInterface, and thus can be used to re-enter the realm.
+	 */
+	const ScriptInterface& GetScriptInterface() const;
+
 	JS::Value globalValue() const;
+
+	// Note that JSContext actually changes behind the scenes when creating another ScriptRequest for another realm,
+	// so be _very_ careful when juggling between different realms.
 	JSContext* cx;
 	JS::HandleObject glob;
 	JS::HandleObject nativeScope;
 private:
-	JS::Realm* m_formerRealm;
+	const ScriptInterface& m_ScriptInterface;
+	JS::Realm* m_FormerRealm;
 };
 
 

source/scriptinterface/tests/test_FunctionWrapper.h

@@ -43,8 +43,8 @@ public:
 	static void _handle(JS::HandleValue) {};
 	static void _handle_2(int, JS::HandleValue, bool) {};
 
-	static void _cmpt_private(ScriptInterface::CmptPrivate*) {};
-	static int _cmpt_private_2(ScriptInterface::CmptPrivate*, int a, bool) { return a; };
+	static void _script_interface(const ScriptInterface&) {};
+	static int _script_interface_2(const ScriptInterface&, int a, bool) { return a; };
 
 	static void _script_request(const ScriptRequest&) {};
 	static int _script_request_2(const ScriptRequest&, int a, bool) { return a; };
@@ -53,8 +53,8 @@ public:
 	{
 		static_assert(std::is_same_v<decltype(&ScriptFunction::ToJSNative<&TestFunctionWrapper::_handle>), JSNative>);
 		static_assert(std::is_same_v<decltype(&ScriptFunction::ToJSNative<&TestFunctionWrapper::_handle_2>), JSNative>);
-		static_assert(std::is_same_v<decltype(&ScriptFunction::ToJSNative<&TestFunctionWrapper::_cmpt_private>), JSNative>);
-		static_assert(std::is_same_v<decltype(&ScriptFunction::ToJSNative<&TestFunctionWrapper::_cmpt_private_2>), JSNative>);
+		static_assert(std::is_same_v<decltype(&ScriptFunction::ToJSNative<&TestFunctionWrapper::_script_interface>), JSNative>);
+		static_assert(std::is_same_v<decltype(&ScriptFunction::ToJSNative<&TestFunctionWrapper::_script_interface_2>), JSNative>);
 		static_assert(std::is_same_v<decltype(&ScriptFunction::ToJSNative<&TestFunctionWrapper::_script_request>), JSNative>);
 		static_assert(std::is_same_v<decltype(&ScriptFunction::ToJSNative<&TestFunctionWrapper::_script_request_2>), JSNative>);
 	}
@@ -71,13 +71,13 @@ public:
 	void test_method_wrappers()
 	{
 		static_assert(std::is_same_v<decltype(&ScriptFunction::ToJSNative<&TestFunctionWrapper::test_method::method_1,
-											  &ScriptFunction::ObjectFromCBData<test_method>>), JSNative>);
+											  &ScriptInterface::ObjectFromCBData<test_method>>), JSNative>);
 		static_assert(std::is_same_v<decltype(&ScriptFunction::ToJSNative<&TestFunctionWrapper::test_method::method_2,
-											  &ScriptFunction::ObjectFromCBData<test_method>>), JSNative>);
+											  &ScriptInterface::ObjectFromCBData<test_method>>), JSNative>);
 		static_assert(std::is_same_v<decltype(&ScriptFunction::ToJSNative<&TestFunctionWrapper::test_method::const_method_1,
-											  &ScriptFunction::ObjectFromCBData<test_method>>), JSNative>);
+											  &ScriptInterface::ObjectFromCBData<test_method>>), JSNative>);
 		static_assert(std::is_same_v<decltype(&ScriptFunction::ToJSNative<&TestFunctionWrapper::test_method::const_method_2,
-											  &ScriptFunction::ObjectFromCBData<test_method>>), JSNative>);
+											  &ScriptInterface::ObjectFromCBData<test_method>>), JSNative>);
 	}
 
 	void test_calling()
@@ -100,7 +100,7 @@ public:
 			TS_ASSERT_EQUALS(ret, 4);
 		}
 
-		ScriptFunction::Register<&TestFunctionWrapper::_cmpt_private_2>(script, "_cmpt_private_2");
+		ScriptFunction::Register<&TestFunctionWrapper::_script_interface_2>(script, "_cmpt_private_2");
 		{
 			std::string input = "Test._cmpt_private_2(4);";
 			int ret = 0;