Better handling of disposed components - fixes #175 (#184)

* Updated changelog with pharry22's docs contributions

* Fixes #175

* updated changelog

Author: egil

CHANGELOG.md

@@ -6,6 +6,8 @@ All notable changes to **bUnit** will be documented in this file. The project ad
 
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 
+Thanks to [pharry22](https://github.com/pharry22) for submitting fixes and improvements to the documentation.
+
 ### Added
 List of new features.
 
@@ -16,6 +18,8 @@ List of new features.
 List of changes in existing functionality.
 
 - Moved `InvokeAsync()`, `Render()` and `SetParametersAndRender()` methods out of `IRenderedComponentBase<TComponent>` into extension methods. By [@JeroenBos](https://github.com/JeroenBos) in [#151](https://github.com/egil/bUnit/pull/177).
+- Accessing `Markup`, `Nodes` and related methods on a rendered fragment whose underlying component has been removed from the render tree (disposed) now throws a `ComponentDisposedException`. By [@egil](https://github.com/egil) in [#184](https://github.com/egil/bUnit/pull/184).
+
 
 ### Deprecated
 List of soon-to-be removed features.
@@ -26,6 +30,8 @@ List of now removed features.
 ### Fixed
 List of any bug fixes.
 
+- Fixes [#175](https://github.com/egil/bUnit/issues/175): When a component referenced in a test, e.g. through the `FindComponent()` method was removed from the render tree, accessing the reference could caused bUnit to look for updates to it in the renderer, causing a exception to be thrown. By [@egil](https://github.com/egil) in [#184](https://github.com/egil/bUnit/pull/184).
+
 ### Security
 List of fixed security vulnerabilities.
 

src/bunit.core/Rendering/ComponentDisposedException.cs

@@ -0,0 +1,23 @@
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace Bunit.Rendering
+{
+	/// <summary>
+	/// Represents an exception that is thrown when a <see cref="IRenderedFragmentBase"/>'s
+	/// properties is accessed after the underlying component has been dispsoed by the renderer.
+	/// </summary>
+	public class ComponentDisposedException : Exception
+	{
+		/// <summary>
+		/// Creates an instance of the <see cref="ComponentDisposedException"/>.
+		/// </summary>
+		/// <param name="componentId">Id of the disposed component.</param>
+		public ComponentDisposedException(int componentId)
+			: base($"The component has been removed from the render tree by the renderer and is no longer available for inspection. ComponentId = {componentId}.")
+		{
+
+		}
+	}
+}

src/bunit.core/Rendering/RenderEvent.cs

@@ -1,3 +1,4 @@
+using System;
 using Microsoft.AspNetCore.Components.RenderTree;
 
 namespace Bunit.Rendering
@@ -67,8 +68,8 @@ bool HasChangedToChildren(ArrayRange<RenderTreeFrame> componentRenderTreeFrames)
 		/// <returns>True if component was disposed, false otherwise.</returns>
 		public bool DidComponentDispose(int componentId)
 		{
-			for (var i = 0; i < _renderBatch.DisposedEventHandlerIDs.Count; i++)
-				if (_renderBatch.DisposedEventHandlerIDs.Array[i].Equals(componentId))
+			for (var i = 0; i < _renderBatch.DisposedComponentIDs.Count; i++)
+				if (_renderBatch.DisposedComponentIDs.Array[i].Equals(componentId))
 					return true;
 
 			return false;
@@ -93,18 +94,21 @@ bool DidComponentRenderRoot(int componentId)
 						return true;
 				}
 
-				return DidChildComponentRender(_renderer.GetCurrentRenderTreeFrames(componentId));
+				return DidChildComponentRender(componentId);
 			}
 
-			bool DidChildComponentRender(ArrayRange<RenderTreeFrame> componentRenderTreeFrames)
+			bool DidChildComponentRender(int componentId)
 			{
-				for (var i = 0; i < componentRenderTreeFrames.Count; i++)
+				var frames = _renderer.GetCurrentRenderTreeFrames(componentId);
+
+				for (var i = 0; i < frames.Count; i++)
 				{
-					ref var frame = ref componentRenderTreeFrames.Array[i];
+					ref var frame = ref frames.Array[i];
 					if (frame.FrameType == RenderTreeFrameType.Component)
 						if (DidComponentRenderRoot(frame.ComponentId))
 							return true;
 				}
+
 				return false;
 			}
 		}

src/bunit.core/Rendering/TestRenderer.cs

@@ -148,10 +148,21 @@ private Task PublishRenderEvent(in RenderBatch renderBatch)
 			{
 				0 => Task.CompletedTask,
 				1 => _renderEventHandlers[0].Handle(renderEvent),
-				_ => Task.WhenAll(_renderEventHandlers.Select(x => x.Handle(renderEvent)))
+				_ => NotifyEventHandlers(renderEvent)
 			};
 		}
 
+		private Task NotifyEventHandlers(RenderEvent renderEvent)
+		{
+			// copy to new array since _renderEventHandlers might be modified by the
+			// Handle method on event handlers if component is disposed.
+			var handleTasks = _renderEventHandlers
+				.ToArray()
+				.Select(x => x.Handle(renderEvent));
+
+			return Task.WhenAll(handleTasks);
+		}
+
 		private void AssertNoUnhandledExceptions()
 		{
 			if (_unhandledException is { } unhandled)

src/bunit.testassets/SampleComponents/ToggleChildComponent.razor

@@ -0,0 +1,8 @@
+@if (ShowChild)
+{
+	<Simple1 Header="Helle World" />
+}
+@code {
+	[Parameter]
+	public bool ShowChild { get; set; }
+}

src/bunit.web.tests/Rendering/RenderedComponentTest.cs

@@ -1,4 +1,5 @@
 using System;
+using Bunit.Rendering;
 using Bunit.TestAssets.SampleComponents;
 using Bunit.TestDoubles.JSInterop;
 using Shouldly;
@@ -53,5 +54,17 @@ public void Test003()
 			Should.Throw<InvalidOperationException>(() => cut.SetParametersAndRender(ps => ps.Add(p => p.UnnamedCascadingValue, 42)));
 			Should.Throw<InvalidOperationException>(() => cut.SetParametersAndRender(ps => ps.Add(p => p.NamedCascadingValue, 1337)));
 		}
+
+		[Fact(DisplayName = "Getting Instance from a RenderedComponent based on a disposed component throws")]
+		public void Test020()
+		{
+			var cut = RenderComponent<ToggleChildComponent>(ps => ps.Add(p => p.ShowChild, true));
+			var target = cut.FindComponent<Simple1>();
+
+			// Disposes of <Simple1 />
+			cut.SetParametersAndRender(ps => ps.Add(p => p.ShowChild, false));
+
+			Should.Throw<ComponentDisposedException>(() => target.Instance);
+		}
 	}
 }

src/bunit.web.tests/Rendering/RenderedFragmentTest.cs

@@ -1,3 +1,4 @@
+using System;
 using Bunit.Rendering;
 using Bunit.TestAssets.SampleComponents;
 
@@ -173,5 +174,17 @@ public void Test010()
 			first.RenderCount.ShouldBe(2);
 			second.RenderCount.ShouldBe(2);
 		}
+
+		[Fact(DisplayName = "Getting Markup from a RenderedFragment based on a disposed component throws")]
+		public void Test020()
+		{
+			var cut = RenderComponent<ToggleChildComponent>(ps => ps.Add(p => p.ShowChild, true));
+			var target = cut.FindComponent<Simple1>();
+
+			// Disposes of <Simple1 />
+			cut.SetParametersAndRender(ps => ps.Add(p => p.ShowChild, false));
+
+			Should.Throw<ComponentDisposedException>(() => target.Markup);
+		}
 	}
 }

src/bunit.web/Rendering/RenderedComponent.cs

@@ -9,12 +9,21 @@ namespace Bunit.Rendering
 	/// <inheritdoc/>
 	internal class RenderedComponent<TComponent> : RenderedFragment, IRenderedComponent<TComponent> where TComponent : IComponent
 	{
+		private readonly TComponent _instance;
+
 		/// <inheritdoc/>
-		public TComponent Instance { get; }
+		public TComponent Instance
+		{
+			get
+			{
+				EnsureComponentNotDisposed();
+				return _instance;
+			}
+		}
 
 		public RenderedComponent(IServiceProvider services, int componentId, TComponent component) : base(services, componentId)
 		{
-			Instance = component;
-		}		
+			_instance = component;
+		}
 	}
 }

src/bunit.web/Rendering/RenderedFragment.cs

@@ -19,13 +19,14 @@ namespace Bunit.Rendering
 	/// </summary>
 	public class RenderedFragment : IRenderedFragment, IRenderEventHandler
 	{
-		private readonly object _lock = new object();
+		private readonly object _markupAccessLock = new object();
 		private readonly ILogger<RenderedFragment> _logger;
 		private string? _snapshotMarkup;
 		private INodeList? _firstRenderNodes;
 		private INodeList? _latestRenderNodes;
 		private INodeList? _snapshotNodes;
 		private string _markup;
+		private bool _componentDisposed = false;
 
 		private HtmlParser HtmlParser { get; }
 
@@ -36,7 +37,7 @@ public class RenderedFragment : IRenderedFragment, IRenderEventHandler
 
 		/// <inheritdoc/>
 		public ITestRenderer Renderer { get; }
-		
+
 		/// <inheritdoc/>
 		public IServiceProvider Services { get; }
 
@@ -48,9 +49,11 @@ public string Markup
 		{
 			get
 			{
+				EnsureComponentNotDisposed();
+
 				// The lock ensures that we cannot read the _markup and _latestRenderNodes
 				// field while it is being updated
-				lock (_lock)
+				lock (_markupAccessLock)
 				{
 					return Volatile.Read(ref _markup);
 				}
@@ -62,8 +65,10 @@ public INodeList Nodes
 		{
 			get
 			{
+				EnsureComponentNotDisposed();
+
 				// The lock ensures that latest nodes is always based on the latest rendered markup.
-				lock (_lock)
+				lock (_markupAccessLock)
 				{
 					if (_latestRenderNodes is null)
 						_latestRenderNodes = HtmlParser.Parse(Markup);
@@ -131,26 +136,30 @@ public IReadOnlyList<IDiff> GetChangesSinceFirstRender()
 
 		Task IRenderEventHandler.Handle(RenderEvent renderEvent)
 		{
-			HandleComponentRender(renderEvent);
+			if (renderEvent.DidComponentDispose(ComponentId))
+			{
+				HandleComponentDisposed();
+			}
+			else if (renderEvent.DidComponentRender(ComponentId))
+			{
+				HandleComponentRender(renderEvent);
+			}
 			return Task.CompletedTask;
 		}
 
 		private void HandleComponentRender(RenderEvent renderEvent)
 		{
-			if (renderEvent.DidComponentRender(ComponentId))
-			{
-				_logger.LogDebug(new EventId(1, nameof(HandleComponentRender)), $"Received a new render where component {ComponentId} did render.");
+			_logger.LogDebug(new EventId(1, nameof(HandleComponentRender)), $"Received a new render where component {ComponentId} did render.");
 
-				RenderCount++;
+			RenderCount++;
 
-				// First notify derived types, e.g. queried AngleSharp collections or elements
-				// that the markup has changed and they should rerun their queries.
-				HandleChangesToMarkup(renderEvent);
+			// First notify derived types, e.g. queried AngleSharp collections or elements
+			// that the markup has changed and they should rerun their queries.
+			HandleChangesToMarkup(renderEvent);
 
-				// Then it is safe to tell anybody waiting on updates or changes to the rendered fragment
-				// that they can redo their assertions or continue processing.
-				OnAfterRender?.Invoke();
-			}
+			// Then it is safe to tell anybody waiting on updates or changes to the rendered fragment
+			// that they can redo their assertions or continue processing.
+			OnAfterRender?.Invoke();
 		}
 
 		private void HandleChangesToMarkup(RenderEvent renderEvent)
@@ -160,19 +169,31 @@ private void HandleChangesToMarkup(RenderEvent renderEvent)
 				_logger.LogDebug(new EventId(1, nameof(HandleChangesToMarkup)), $"Received a new render where the markup of component {ComponentId} changed.");
 
 				// The lock ensures that latest nodes is always based on the latest rendered markup.
-				lock (_lock)
+				lock (_markupAccessLock)
 				{
 					_latestRenderNodes = null;
 					_markup = RetrieveLatestMarkupFromRenderer();
 				}
 
 				OnMarkupUpdated?.Invoke();
 			}
-			else if (renderEvent.DidComponentDispose(ComponentId))
-			{
-				_logger.LogDebug(new EventId(1, nameof(HandleChangesToMarkup)), $"Received a new render where the component {ComponentId} was disposed.");
-				Renderer.RemoveRenderEventHandler(this);
-			}
+		}
+
+		private void HandleComponentDisposed()
+		{
+			_logger.LogDebug(new EventId(1, nameof(HandleChangesToMarkup)), $"Received a new render where the component {ComponentId} was disposed.");
+			_componentDisposed = true;
+			Renderer.RemoveRenderEventHandler(this);
+		}
+
+		/// <summary>
+		/// Ensures that the underlying component behind the
+		/// fragment has not been removed from the render tree.
+		/// </summary>
+		protected void EnsureComponentNotDisposed()
+		{
+			if (_componentDisposed)
+				throw new ComponentDisposedException(ComponentId);
 		}
 	}
 }