Allow observer pointers after Rml::Shutdown, but be more assertive ab…

…out elements

It is rather innocent for users to keep objects derived from Rml::EventListener around after shut down, and it can be a hassle to work around that. So instead, allow that situation and rather make observer pointers garbage collect themselves if they are destroyed after Rml::Shutdown.

On the other hand, keeping an Rml::Element around after Rml::Shutdown will most likely lead to a crash. Be more assertive when detecting this and make it an error.

Source/Core/ElementMeta.cpp

@@ -46,7 +46,11 @@ void ElementMetaPool::Shutdown()
 	}
 	else
 	{
-		Log::Message(Log::LT_WARNING, "Element meta pool not empty on shutdown, %d object(s) leaked.", num_objects);
+		Log::Message(Log::LT_ERROR,
+			"Element meta pool not empty on shutdown, %d object(s) leaked. This will likely lead to a crash when element is destroyed. Ensure that "
+			"no Rml::Element objects are kept alive in user space at the end of Rml::Shutdown.",
+			num_objects);
+		RMLUI_ERROR;
 		element_meta_pool.Leak();
 	}
 }

Source/Core/ObserverPtr.cpp

@@ -34,6 +34,7 @@
 namespace Rml {
 
 struct ObserverPtrData {
+	bool is_shutdown = false;
 	Pool<Detail::ObserverPtrBlock> block_pool{128, true};
 };
 static ControlledLifetimeResource<ObserverPtrData> observer_ptr_data;
@@ -44,11 +45,16 @@ void Detail::DeallocateObserverPtrBlockIfEmpty(ObserverPtrBlock* block)
 	if (block->num_observers == 0 && block->pointed_to_object == nullptr)
 	{
 		observer_ptr_data->block_pool.DestroyAndDeallocate(block);
+		if (observer_ptr_data->is_shutdown && observer_ptr_data->block_pool.GetNumAllocatedObjects() == 0)
+		{
+			observer_ptr_data.Shutdown();
+		}
 	}
 }
 void Detail::InitializeObserverPtrPool()
 {
 	observer_ptr_data.InitializeIfEmpty();
+	observer_ptr_data->is_shutdown = false;
 }
 
 void Detail::ShutdownObserverPtrPool()
@@ -61,12 +67,11 @@ void Detail::ShutdownObserverPtrPool()
 	else
 	{
 		// This pool must outlive all other global variables that derive from EnableObserverPtr. This even includes user
-		// variables which we have no control over. So if there are any objects still alive, let the pool leak.
-		Log::Message(Log::LT_WARNING,
-			"Observer pointers still alive on shutdown, %d object(s) leaked. "
-			"Please ensure that no RmlUi objects are retained in user space at the end of Rml::Shutdown.",
-			num_objects);
-		observer_ptr_data.Leak();
+		// variables which we have no control over. So if there are any objects still alive, let the pool garbage
+		// collect itself when all references to it are gone. It is somewhat unreasonable to expect that no observer
+		// pointers remain, particularly because that means no objects derived from Rml::EventListener can be alive in
+		// user space, which can be a hassle to ensure and is otherwise pretty innocent.
+		observer_ptr_data->is_shutdown = true;
 	}
 }
 

Tests/Source/UnitTests/Core.cpp

@@ -309,11 +309,14 @@ TEST_CASE("core.observer_ptr")
 	ObserverPtr<Element> observer_ptr = document->GetObserverPtr();
 	document->Close();
 
-	// We expect a warning about the observer pointer being held in user space, preventing its memory pool from shutting down.
-	TestsSystemInterface* system_interface = TestsShell::GetTestsSystemInterface();
-	system_interface->SetNumExpectedWarnings(1);
-
+	// Keeping the observer pointer alive should prevent the memory pool from shutting down.
 	TestsShell::ShutdownShell();
+
+	// For that reason, the observer pointer can still be used as normal without crashing.
+	REQUIRE(!observer_ptr);
+
+	// Resetting the observer pointer (or destroying it) should not cause any crashes, and should release the memory pool.
+	observer_ptr.reset();
 }
 
 TEST_CASE("core.RemoveContext")

Tests/Source/UnitTests/ElementDocument.cpp

@@ -260,31 +260,28 @@ TEST_CASE("Load")
 	Context* context = TestsShell::GetContext();
 	REQUIRE(context);
 
-	{
-		MockEventListener mockEventListener;
-		MockEventListenerInstancer mockEventListenerInstancer;
+	MockEventListener mockEventListener;
+	MockEventListenerInstancer mockEventListenerInstancer;
 
-		tl::sequence sequence;
-		REQUIRE_CALL(mockEventListenerInstancer, InstanceEventListener("something", tl::_))
-			.WITH(_2->GetTagName() == BODY_TAG)
-			.IN_SEQUENCE(sequence)
-			.LR_RETURN(&mockEventListener);
+	tl::sequence sequence;
+	REQUIRE_CALL(mockEventListenerInstancer, InstanceEventListener("something", tl::_))
+		.WITH(_2->GetTagName() == BODY_TAG)
+		.IN_SEQUENCE(sequence)
+		.LR_RETURN(&mockEventListener);
 
-		ALLOW_CALL(mockEventListener, OnAttach(tl::_));
-		ALLOW_CALL(mockEventListener, OnDetach(tl::_));
+	ALLOW_CALL(mockEventListener, OnAttach(tl::_));
+	ALLOW_CALL(mockEventListener, OnDetach(tl::_));
 
-		REQUIRE_CALL(mockEventListener, ProcessEvent(tl::_))
-			.WITH(_1.GetId() == EventId::Load && _1.GetTargetElement()->GetTagName() == BODY_TAG)
-			.IN_SEQUENCE(sequence);
+	REQUIRE_CALL(mockEventListener, ProcessEvent(tl::_))
+		.WITH(_1.GetId() == EventId::Load && _1.GetTargetElement()->GetTagName() == BODY_TAG)
+		.IN_SEQUENCE(sequence);
 
-		Factory::RegisterEventListenerInstancer(&mockEventListenerInstancer);
+	Factory::RegisterEventListenerInstancer(&mockEventListenerInstancer);
 
-		ElementDocument* document = context->LoadDocumentFromMemory(document_focus_rml);
-		REQUIRE(document);
+	ElementDocument* document = context->LoadDocumentFromMemory(document_focus_rml);
+	REQUIRE(document);
 
-		document->Close();
-		context->Update();
-	}
+	document->Close();
 
 	TestsShell::ShutdownShell();
 }

Tests/Source/UnitTests/ElementFormControlSelect.cpp

@@ -568,8 +568,6 @@ TEST_CASE("form.select.event.change")
 	CHECK(listener->num_events_processed == 3);
 
 	document->Close();
-	context->Update();
-	listener.reset();
 
 	TestsShell::ShutdownShell();
 }