Browse Source

Sharing style sheet containers is dangerous, clean up accordingly.

Michael Ragazzon 4 years ago
parent
commit
f0a0480e9b

+ 4 - 3
Include/RmlUi/Core/ElementDocument.h

@@ -88,9 +88,6 @@ public:
 	const StyleSheet* GetStyleSheet() const override;
 	const StyleSheet* GetStyleSheet() const override;
 	/// Sets the style sheet this document, and all of its children, uses.
 	/// Sets the style sheet this document, and all of its children, uses.
 	void SetStyleSheetContainer(SharedPtr<StyleSheetContainer> style_sheet);
 	void SetStyleSheetContainer(SharedPtr<StyleSheetContainer> style_sheet);
-	/// Returns the active style sheet container for this element.
-	/// @return The element's style sheet container.
-	const SharedPtr<StyleSheetContainer>& GetStyleSheetContainer() const;
 	/// Reload the document's style sheet from source files.
 	/// Reload the document's style sheet from source files.
 	/// Styles will be reloaded from <style> tags and external style sheets, but not inline 'style' attributes.
 	/// Styles will be reloaded from <style> tags and external style sheets, but not inline 'style' attributes.
 	/// @note The source url originally used to load the document must still be a valid RML document.
 	/// @note The source url originally used to load the document must still be a valid RML document.
@@ -154,6 +151,10 @@ private:
 	/// Searches forwards or backwards for a focusable element in the given substree
 	/// Searches forwards or backwards for a focusable element in the given substree
 	Element* SearchFocusSubtree(Element* element, bool forward);
 	Element* SearchFocusSubtree(Element* element, bool forward);
 
 
+	/// Returns the active style sheet container for this element.
+	/// @warning Shared style sheet containers should be used with care, mainly used for proxy elements in the same context.
+	const SharedPtr<StyleSheetContainer>& GetStyleSheetContainer() const;
+
 	/// Sets the dirty flag on the layout so the document will format its children before the next render.
 	/// Sets the dirty flag on the layout so the document will format its children before the next render.
 	void DirtyLayout() override;
 	void DirtyLayout() override;
 	/// Returns true if the document has been marked as needing a re-layout.
 	/// Returns true if the document has been marked as needing a re-layout.

+ 12 - 14
Source/Core/Context.cpp

@@ -65,10 +65,8 @@ Context::Context(const String& name) : name(name), dimensions(0, 0), density_ind
 
 
 	cursor_proxy = Factory::InstanceElement(nullptr, documents_base_tag, documents_base_tag, XMLAttributes());
 	cursor_proxy = Factory::InstanceElement(nullptr, documents_base_tag, documents_base_tag, XMLAttributes());
 	ElementDocument* cursor_proxy_document = rmlui_dynamic_cast< ElementDocument* >(cursor_proxy.get());
 	ElementDocument* cursor_proxy_document = rmlui_dynamic_cast< ElementDocument* >(cursor_proxy.get());
-	if (cursor_proxy_document)
-		cursor_proxy_document->context = this;
-	else
-		cursor_proxy.reset();
+	RMLUI_ASSERT(cursor_proxy_document);
+	cursor_proxy_document->context = this;
 		
 		
 	enable_cursor = true;
 	enable_cursor = true;
 
 
@@ -96,10 +94,10 @@ Context::~Context()
 
 
 	ReleaseUnloadedDocuments();
 	ReleaseUnloadedDocuments();
 
 
-	cursor_proxy.reset();
-
 	root.reset();
 	root.reset();
 
 
+	cursor_proxy.reset();
+
 	instancer = nullptr;
 	instancer = nullptr;
 
 
 	render_interface = nullptr;
 	render_interface = nullptr;
@@ -206,8 +204,8 @@ bool Context::Render()
 
 
 	ElementUtilities::SetClippingRegion(nullptr, this);
 	ElementUtilities::SetClippingRegion(nullptr, this);
 
 
-	// Render the cursor proxy so any elements attached the cursor will be rendered below the cursor.
-	if (cursor_proxy)
+	// Render the cursor proxy so that any attached drag clone will be rendered below the cursor.
+	if (drag_clone)
 	{
 	{
 		static_cast<ElementDocument&>(*cursor_proxy).UpdateDocument();
 		static_cast<ElementDocument&>(*cursor_proxy).UpdateDocument();
 		cursor_proxy->SetOffset(Vector2f((float)Math::Clamp(mouse_position.x, 0, dimensions.x),
 		cursor_proxy->SetOffset(Vector2f((float)Math::Clamp(mouse_position.x, 0, dimensions.x),
@@ -350,6 +348,7 @@ void Context::UnloadDocument(ElementDocument* _document)
 	if (drag && drag->GetOwnerDocument() == document)
 	if (drag && drag->GetOwnerDocument() == document)
 	{
 	{
 		drag = nullptr;
 		drag = nullptr;
+		ReleaseDragClone();
 	}
 	}
 
 
 	if (drag_hover && drag_hover->GetOwnerDocument() == document)
 	if (drag_hover && drag_hover->GetOwnerDocument() == document)
@@ -1179,11 +1178,7 @@ Element* Context::GetElementAtPoint(Vector2f point, const Element* ignore_elemen
 // Creates the drag clone from the given element.
 // Creates the drag clone from the given element.
 void Context::CreateDragClone(Element* element)
 void Context::CreateDragClone(Element* element)
 {
 {
-	if (!cursor_proxy)
-	{
-		Log::Message(Log::LT_ERROR, "Unable to create drag clone, no cursor proxy document.");
-		return;
-	}
+	RMLUI_ASSERTMSG(cursor_proxy, "Unable to create drag clone, no cursor proxy document.");
 
 
 	ReleaseDragClone();
 	ReleaseDragClone();
 
 
@@ -1201,8 +1196,10 @@ void Context::CreateDragClone(Element* element)
 	cursor_proxy->AppendChild(std::move(element_drag_clone));
 	cursor_proxy->AppendChild(std::move(element_drag_clone));
 
 
 	// Set the style sheet on the cursor proxy.
 	// Set the style sheet on the cursor proxy.
-	if(ElementDocument* document = element->GetOwnerDocument())
+	if (ElementDocument* document = element->GetOwnerDocument())
+	{
 		static_cast<ElementDocument&>(*cursor_proxy).SetStyleSheetContainer(document->GetStyleSheetContainer());
 		static_cast<ElementDocument&>(*cursor_proxy).SetStyleSheetContainer(document->GetStyleSheetContainer());
+	}
 
 
 	// Set all the required properties and pseudo-classes on the clone.
 	// Set all the required properties and pseudo-classes on the clone.
 	drag_clone->SetPseudoClass("drag", true);
 	drag_clone->SetPseudoClass("drag", true);
@@ -1218,6 +1215,7 @@ void Context::ReleaseDragClone()
 	{
 	{
 		cursor_proxy->RemoveChild(drag_clone);
 		cursor_proxy->RemoveChild(drag_clone);
 		drag_clone = nullptr;
 		drag_clone = nullptr;
+		static_cast<ElementDocument&>(*cursor_proxy).SetStyleSheetContainer(nullptr);
 	}
 	}
 }
 }
 
 

+ 2 - 2
Source/Core/ElementDocument.cpp

@@ -102,7 +102,7 @@ void ElementDocument::ProcessHeader(const DocumentHeader* document_header)
 	{
 	{
 		if (rcss.is_inline)
 		if (rcss.is_inline)
 		{
 		{
-			UniquePtr<StyleSheetContainer> inline_sheet = MakeUnique<StyleSheetContainer>();
+			auto inline_sheet = MakeShared<StyleSheetContainer>();
 			auto stream = MakeUnique<StreamMemory>((const byte*)rcss.content.c_str(), rcss.content.size());
 			auto stream = MakeUnique<StreamMemory>((const byte*)rcss.content.c_str(), rcss.content.size());
 			stream->SetSourceURL(rcss.path);
 			stream->SetSourceURL(rcss.path);
 
 
@@ -118,7 +118,7 @@ void ElementDocument::ProcessHeader(const DocumentHeader* document_header)
 		}
 		}
 		else
 		else
 		{
 		{
-			SharedPtr<const StyleSheetContainer> sub_sheet = StyleSheetFactory::GetStyleSheetContainer(rcss.path);
+			const StyleSheetContainer* sub_sheet = StyleSheetFactory::GetStyleSheetContainer(rcss.path);
 			if (sub_sheet)
 			if (sub_sheet)
 			{
 			{
 				if (new_style_sheet)
 				if (new_style_sheet)

+ 11 - 9
Source/Core/StyleSheetFactory.cpp

@@ -79,22 +79,24 @@ void StyleSheetFactory::Shutdown()
 	instance.reset();
 	instance.reset();
 }
 }
 
 
-SharedPtr<const StyleSheetContainer> StyleSheetFactory::GetStyleSheetContainer(const String& sheet_name)
+const StyleSheetContainer* StyleSheetFactory::GetStyleSheetContainer(const String& sheet_name)
 {
 {
 	// Look up the sheet definition in the cache
 	// Look up the sheet definition in the cache
 	auto it = instance->stylesheets.find(sheet_name);
 	auto it = instance->stylesheets.find(sheet_name);
 	if (it != instance->stylesheets.end())
 	if (it != instance->stylesheets.end())
-		return it->second;
+		return it->second.get();
 
 
 	// Don't currently have the sheet, attempt to load it
 	// Don't currently have the sheet, attempt to load it
-	SharedPtr<const StyleSheetContainer> sheet = instance->LoadStyleSheetContainer(sheet_name);
+	UniquePtr<const StyleSheetContainer> sheet = instance->LoadStyleSheetContainer(sheet_name);
 	if (!sheet)
 	if (!sheet)
 		return nullptr;
 		return nullptr;
 
 
-	// Add it to the cache, and add a reference count so the cache will keep hold of it.
-	instance->stylesheets[sheet_name] = sheet;
+	const StyleSheetContainer* result = sheet.get();
 
 
-	return sheet;
+	// Add it to the cache.
+	instance->stylesheets[sheet_name] = std::move(sheet);
+
+	return result;
 }
 }
 
 
 // Clear the style sheet cache.
 // Clear the style sheet cache.
@@ -182,15 +184,15 @@ StructuralSelector StyleSheetFactory::GetSelector(const String& name)
 	return StructuralSelector(it->second.get(), a, b);
 	return StructuralSelector(it->second.get(), a, b);
 }
 }
 
 
-SharedPtr<const StyleSheetContainer> StyleSheetFactory::LoadStyleSheetContainer(const String& sheet)
+UniquePtr<const StyleSheetContainer> StyleSheetFactory::LoadStyleSheetContainer(const String& sheet)
 {
 {
-	SharedPtr<StyleSheetContainer> new_style_sheet;
+	UniquePtr<StyleSheetContainer> new_style_sheet;
 
 
 	// Open stream, construct new sheet and pass the stream into the sheet
 	// Open stream, construct new sheet and pass the stream into the sheet
 	auto stream = MakeUnique<StreamFile>();
 	auto stream = MakeUnique<StreamFile>();
 	if (stream->Open(sheet))
 	if (stream->Open(sheet))
 	{
 	{
-		new_style_sheet = MakeShared<StyleSheetContainer>();
+		new_style_sheet = MakeUnique<StyleSheetContainer>();
 		if (!new_style_sheet->LoadStyleSheetContainer(stream.get()))
 		if (!new_style_sheet->LoadStyleSheetContainer(stream.get()))
 		{
 		{
 			new_style_sheet = nullptr;
 			new_style_sheet = nullptr;

+ 5 - 4
Source/Core/StyleSheetFactory.h

@@ -53,9 +53,10 @@ public:
 	/// Shutdown style manager
 	/// Shutdown style manager
 	static void Shutdown();
 	static void Shutdown();
 
 
-	/// Gets the named sheet, retrieving it from the cache if its already been loaded
+	/// Gets the named sheet, retrieving it from the cache if its already been loaded.
 	/// @param sheet name of sheet to load
 	/// @param sheet name of sheet to load
-	static SharedPtr<const StyleSheetContainer> GetStyleSheetContainer(const String& sheet);
+	/// @lifetime Returned pointer is valid until the next call to ClearStyleSheetCache or Shutdown, it should not be stored around.
+	static const StyleSheetContainer* GetStyleSheetContainer(const String& sheet);
 
 
 	/// Clear the style sheet cache.
 	/// Clear the style sheet cache.
 	static void ClearStyleSheetCache();
 	static void ClearStyleSheetCache();
@@ -69,10 +70,10 @@ private:
 	StyleSheetFactory();
 	StyleSheetFactory();
 
 
 	// Loads an individual style sheet
 	// Loads an individual style sheet
-	SharedPtr<const StyleSheetContainer> LoadStyleSheetContainer(const String& sheet);
+	UniquePtr<const StyleSheetContainer> LoadStyleSheetContainer(const String& sheet);
 
 
 	// Individual loaded stylesheets
 	// Individual loaded stylesheets
-	using StyleSheets = UnorderedMap<String, SharedPtr<const StyleSheetContainer>>;
+	using StyleSheets = UnorderedMap<String, UniquePtr<const StyleSheetContainer>>;
 	StyleSheets stylesheets;
 	StyleSheets stylesheets;
 
 
 	// Custom complex selectors available for style sheets.
 	// Custom complex selectors available for style sheets.