New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove circular reference from ShapeDrawable #19347
Changes from 2 commits
36702b1
e172f6f
4ef9488
d46911d
2b77991
aae067e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,50 @@ | ||
using System; | ||
using System.Threading.Tasks; | ||
using Microsoft.Maui.Graphics; | ||
using Microsoft.Maui.Graphics.Platform; | ||
using Microsoft.Maui.Handlers; | ||
using Microsoft.Maui.Platform; | ||
using Microsoft.Maui.Controls; | ||
using System.Reflection; | ||
using Xunit; | ||
|
||
namespace Microsoft.Maui.DeviceTests | ||
{ | ||
public partial class BoxViewTests | ||
{ | ||
MauiShapeView GetNativeBoxView(ShapeViewHandler boxViewHandler) => | ||
boxViewHandler.PlatformView; | ||
|
||
[Fact(DisplayName = "ShapeView Parts Keep Around")] | ||
public async Task ShapeViewPartsKeepAround() | ||
{ | ||
var boxView = new BoxView() | ||
{ | ||
HeightRequest = 100, | ||
WidthRequest = 200 | ||
}; | ||
|
||
await AttachAndRun<ShapeViewHandler>(boxView, async handler => | ||
{ | ||
var shapeView = GetNativeBoxView(handler); | ||
var renderer = (DirectRenderer)shapeView.Renderer; | ||
|
||
GC.Collect(); | ||
GC.WaitForPendingFinalizers(); | ||
await Task.Yield(); | ||
|
||
GC.Collect(); | ||
GC.WaitForPendingFinalizers(); | ||
await Task.Yield(); | ||
|
||
Assert.NotNull(shapeView.Renderer); | ||
Assert.NotNull(shapeView.Drawable); | ||
Assert.NotNull(renderer.Drawable); | ||
|
||
var flags = BindingFlags.NonPublic | BindingFlags.Instance; | ||
var graphicsView = renderer.GetType().GetField("_graphicsView", flags)?.GetValue(renderer) as PlatformGraphicsView; | ||
Assert.NotNull(graphicsView); | ||
}); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -8,9 +8,9 @@ namespace Microsoft.Maui.Graphics.Platform | |||
[Register(nameof(PlatformGraphicsView))] | ||||
public class PlatformGraphicsView : UIView | ||||
{ | ||||
private WeakReference<IGraphicsRenderer> _renderer; | ||||
private IGraphicsRenderer _renderer; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might still be an issue because PlatformGraphicsView references IGraphicsRenderer which has a reference back to PlatformGraphicsView via the DirectRenderer.GraphicsView property. Setting PlatformGraphicsView.Renderer sets the DirectRenderer.GraphicsView property to the same view, thus a circular reference in all cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #19347 (comment) |
||||
private CGColorSpace _colorSpace; | ||||
private WeakReference<IDrawable> _drawable; | ||||
private IDrawable _drawable; | ||||
Comment on lines
-11
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this test case still passes, this is probably good 👍 :
There may also be other cases that depend on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yea I ran the whole set of "leak" tests locally and they all passed |
||||
private CGRect _lastBounds; | ||||
|
||||
public PlatformGraphicsView(CGRect frame, IDrawable drawable = null, IGraphicsRenderer renderer = null) : base(frame) | ||||
|
@@ -34,34 +34,32 @@ public PlatformGraphicsView(IntPtr aPtr) : base(aPtr) | |||
|
||||
public IGraphicsRenderer Renderer | ||||
{ | ||||
get => _renderer is not null && _renderer.TryGetTarget(out var r) ? r : null; | ||||
get => _renderer; | ||||
|
||||
set | ||||
{ | ||||
var renderer = Renderer; | ||||
if (renderer != null) | ||||
if (_renderer != null) | ||||
{ | ||||
renderer.Drawable = null; | ||||
renderer.GraphicsView = null; | ||||
renderer.Dispose(); | ||||
_renderer.Drawable = null; | ||||
_renderer.GraphicsView = null; | ||||
_renderer.Dispose(); | ||||
PureWeen marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
} | ||||
|
||||
renderer = value ?? new DirectRenderer(); | ||||
_renderer = new(renderer); | ||||
_renderer = value ?? new DirectRenderer(); | ||||
|
||||
renderer.GraphicsView = this; | ||||
renderer.Drawable = Drawable; | ||||
_renderer.GraphicsView = this; | ||||
_renderer.Drawable = Drawable; | ||||
var bounds = Bounds; | ||||
renderer.SizeChanged((float)bounds.Width, (float)bounds.Height); | ||||
_renderer.SizeChanged((float)bounds.Width, (float)bounds.Height); | ||||
} | ||||
} | ||||
|
||||
public IDrawable Drawable | ||||
{ | ||||
get => _drawable is not null && _drawable.TryGetTarget(out var d) ? d : null; | ||||
get => _drawable; | ||||
set | ||||
{ | ||||
_drawable = new(value); | ||||
_drawable = value; | ||||
if (Renderer is IGraphicsRenderer renderer) | ||||
{ | ||||
renderer.Drawable = value; | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have more exhaustive tests that validate this inside controls. This test is currently redundant, and the failure currently only happens occasionally in CI. The failure is most likely due to how the test is written and not an actual bug in the bug.