Remove circular reference from ShapeDrawable #19347
Conversation
PureWeen
requested review from
mattleibow,
jsuarezruiz,
pictos,
jonathanpeppers and
Redth
and removed request for
pictos
| private WeakReference<IGraphicsRenderer> _renderer; | ||
| private IGraphicsRenderer _renderer; |
There was a problem hiding this comment.
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.
#19347 (comment)
I ran the leak tests locally and they all passed.
| private WeakReference<IGraphicsRenderer> _renderer; | ||
| private IGraphicsRenderer _renderer; | ||
| private CGColorSpace _colorSpace; | ||
| private WeakReference<IDrawable> _drawable; | ||
| private IDrawable _drawable; |
There was a problem hiding this comment.
If this test case still passes, this is probably good 👍 :
There may also be other cases that depend on PlatformGraphicsView: Polygon? Polyline?
There was a problem hiding this comment.
yea I ran the whole set of "leak" tests locally and they all passed
PureWeen
force-pushed
the
fix_boxview
branch
from
3583cc4
to
36702b1
Compare
|
/azp run |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
Is something AzDO down? I was just trying to review what tests passed and see:
Maybe we can try rerunning in the morning. |
|
/azp run |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
/azp run |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
- remove date picker allocation test
PureWeen
force-pushed
the
fix_boxview
branch
from
071f8a0
to
e172f6f
Compare
| @@ -9,7 +9,9 @@ public class MemoryTestTypes : IEnumerable<object[]> | |||
| { | |||
| public IEnumerator<object[]> GetEnumerator() | |||
| { | |||
| #if !ANDROID | |||
| yield return new object[] { (typeof(DatePickerStub), typeof(DatePickerHandler)) }; | |||
There was a problem hiding this comment.
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.
…19257) * Use Start-Process to run build.ps1 * Show Start-Process command * Fix: Comma separator prior to -Script argument * Dedicated .cake parameter setting for -Script * Fix: Start-Process: Start 3rd parameter with a quote and not an escaped quote * Execute build.ps1 under pwsh in bash * Remove references to Start-Process since that solution did not work * Execute build.ps1 from bash on Mac only
Description of Change
It looks like #18434 introduced too much weakness and the PlatformGraphicsView is keeping a reference to the created
DirectRendererbecause the only root for this is insidePlatformGraphicsView. This PR proposes an alternate fix that makes theIShapesViewinsideShapesDrawableweak. It feels like theShapeDrawableis causing a bunch of cycles because it's retaining a reference to the view that uses it.Issues Fixed
Fixes #19309