From 1ba81d9ec703289faa3cd8f983bc5c69d6142107 Mon Sep 17 00:00:00 2001 From: Kazuki Sakamoto Date: Wed, 26 Oct 2016 06:51:39 -0700 Subject: [PATCH] Prevent GC and avoid new Summary: - Prevent the GC from collecting MeasureInternal in order to call managed MeasureFunction properly from unmanaged - TestMeasureFuncWithDestructor will fail without the fix - Avoid new as C implementation Reviewed By: emilsjolander Differential Revision: D4080765 fbshipit-source-id: d58fa18f6f74012aeda5dd15dfa7ceb0b64584d0 --- csharp/Facebook.CSSLayout/CSSNode.cs | 52 +++++++++---- .../tests/Facebook.CSSLayout/CSSNodeTest.cs | 75 +++++++++++++++++++ 2 files changed, 113 insertions(+), 14 deletions(-) diff --git a/csharp/Facebook.CSSLayout/CSSNode.cs b/csharp/Facebook.CSSLayout/CSSNode.cs index ea3d4ba0..d7581a7c 100644 --- a/csharp/Facebook.CSSLayout/CSSNode.cs +++ b/csharp/Facebook.CSSLayout/CSSNode.cs @@ -18,17 +18,17 @@ namespace Facebook.CSSLayout public class CSSNode : IEnumerable { private IntPtr _cssNode; - private WeakReference _parent; private List _children; private MeasureFunction _measureFunction; + private CSSMeasureFunc _cssMeasureFunc; + private MeasureOutput _measureOutput; private object _data; public CSSNode() { CSSAssert.Initialize(); CSSLogger.Initialize(); - _children = new List(4); _cssNode = Native.CSSNodeNew(); if (_cssNode == IntPtr.Zero) @@ -108,6 +108,7 @@ namespace Facebook.CSSLayout { return Native.CSSNodeStyleGetDirection(_cssNode); } + set { Native.CSSNodeStyleSetDirection(_cssNode, value); @@ -448,7 +449,7 @@ namespace Facebook.CSSLayout { get { - return _children.Count; + return _children != null ? _children.Count : 0; } } @@ -469,6 +470,10 @@ namespace Facebook.CSSLayout public void Insert(int index, CSSNode node) { + if (_children == null) + { + _children = new List(4); + } _children.Insert(index, node); node._parent = new WeakReference(this); Native.CSSNodeInsertChild(_cssNode, node._cssNode, (uint)index); @@ -484,26 +489,43 @@ namespace Facebook.CSSLayout public void Clear() { - while (_children.Count > 0) + if (_children != null) { - RemoveAt(_children.Count-1); + while (_children.Count > 0) + { + RemoveAt(_children.Count-1); + } } } public int IndexOf(CSSNode node) { - return _children.IndexOf(node); + return _children != null ? _children.IndexOf(node) : -1; } public void SetMeasureFunction(MeasureFunction measureFunction) { _measureFunction = measureFunction; - Native.CSSNodeSetMeasureFunc(_cssNode, measureFunction != null ? MeasureInternal : (CSSMeasureFunc)null); + if (measureFunction != null) + { + _cssMeasureFunc = MeasureInternal; + _measureOutput = new MeasureOutput(); + } + else + { + _cssMeasureFunc = null; + _measureOutput = null; + } + Native.CSSNodeSetMeasureFunc(_cssNode, _cssMeasureFunc); } public void CalculateLayout() { - Native.CSSNodeCalculateLayout(_cssNode, CSSConstants.Undefined, CSSConstants.Undefined, Native.CSSNodeStyleGetDirection(_cssNode)); + Native.CSSNodeCalculateLayout( + _cssNode, + CSSConstants.Undefined, + CSSConstants.Undefined, + Native.CSSNodeStyleGetDirection(_cssNode)); } private CSSSize MeasureInternal( @@ -518,13 +540,13 @@ namespace Facebook.CSSLayout throw new InvalidOperationException("Measure function is not defined."); } - var measureResult = new MeasureOutput(); - _measureFunction(this, width, widthMode, height, heightMode, measureResult); + _measureFunction(this, width, widthMode, height, heightMode, _measureOutput); - return new CSSSize { width = measureResult.Width, height = measureResult.Height }; + return new CSSSize { width = _measureOutput.Width, height = _measureOutput.Height }; } - public string Print(CSSPrintOptions options = CSSPrintOptions.Layout|CSSPrintOptions.Style|CSSPrintOptions.Children) + public string Print(CSSPrintOptions options = + CSSPrintOptions.Layout|CSSPrintOptions.Style|CSSPrintOptions.Children) { StringBuilder sb = new StringBuilder(); CSSLogger.Logger = (message) => {sb.Append(message);}; @@ -535,12 +557,14 @@ namespace Facebook.CSSLayout public IEnumerator GetEnumerator() { - return ((IEnumerable)_children).GetEnumerator(); + return _children != null ? ((IEnumerable)_children).GetEnumerator() : + System.Linq.Enumerable.Empty().GetEnumerator(); } IEnumerator IEnumerable.GetEnumerator() { - return ((IEnumerable)_children).GetEnumerator(); + return _children != null ? ((IEnumerable)_children).GetEnumerator() : + System.Linq.Enumerable.Empty().GetEnumerator(); } public static int GetInstanceCount() diff --git a/csharp/tests/Facebook.CSSLayout/CSSNodeTest.cs b/csharp/tests/Facebook.CSSLayout/CSSNodeTest.cs index 3be61960..640aaeed 100644 --- a/csharp/tests/Facebook.CSSLayout/CSSNodeTest.cs +++ b/csharp/tests/Facebook.CSSLayout/CSSNodeTest.cs @@ -39,6 +39,56 @@ namespace Facebook.CSSLayout Assert.AreEqual(0, parent.Count); } + [Test] + public void TestChildren() + { + CSSNode parent = new CSSNode(); + foreach (CSSNode node in parent) { + } + + CSSNode child0 = new CSSNode(); + Assert.AreEqual(-1, parent.IndexOf(child0)); + parent.Insert(0, child0); + foreach (CSSNode node in parent) { + Assert.AreEqual(0, parent.IndexOf(node)); + } + + CSSNode child1 = new CSSNode(); + parent.Insert(1, child1); + int index = 0; + foreach (CSSNode node in parent) { + Assert.AreEqual(index++, parent.IndexOf(node)); + } + + parent.RemoveAt(0); + Assert.AreEqual(-1, parent.IndexOf(child0)); + Assert.AreEqual(0, parent.IndexOf(child1)); + + parent.Clear(); + Assert.AreEqual(0, parent.Count); + + parent.Clear(); + Assert.AreEqual(0, parent.Count); + } + + [Test] + [ExpectedException("System.NullReferenceException")] + public void TestRemoveAtFromEmpty() + { + CSSNode parent = new CSSNode(); + parent.RemoveAt(0); + } + + [Test] + [ExpectedException("System.ArgumentOutOfRangeException")] + public void TestRemoveAtOutOfRange() + { + CSSNode parent = new CSSNode(); + CSSNode child = new CSSNode(); + parent.Insert(0, child); + parent.RemoveAt(1); + } + [Test] [ExpectedException("System.InvalidOperationException")] public void TestCannotAddChildToMultipleParents() @@ -233,6 +283,31 @@ namespace Facebook.CSSLayout Assert.AreEqual(instanceCount + 1, CSSNode.GetInstanceCount()); parent.Insert(0, child); } + + [Test] + public void TestMeasureFuncWithDestructor() + { + ForceGC(); + int instanceCount = CSSNode.GetInstanceCount(); + CSSNode parent = new CSSNode(); + Assert.AreEqual(instanceCount + 1, CSSNode.GetInstanceCount()); + TestMeasureFuncWithDestructorForGC(parent); + ForceGC(); + Assert.AreEqual(instanceCount + 2, CSSNode.GetInstanceCount()); + parent.CalculateLayout(); + Assert.AreEqual(120, (int)parent.LayoutWidth); + Assert.AreEqual(130, (int)parent.LayoutHeight); + } + + private void TestMeasureFuncWithDestructorForGC(CSSNode parent) + { + CSSNode child = new CSSNode(); + parent.Insert(0, child); + child.SetMeasureFunction((_, width, widthMode, height, heightMode, measureResult) => { + measureResult.Width = 120; + measureResult.Height = 130; + }); + } #endif } }