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

@@ -18,17 +18,17 @@ namespace Facebook.CSSLayout
     public class CSSNode : IEnumerable<CSSNode>
     {
         private IntPtr _cssNode;
-
         private WeakReference _parent;
         private List<CSSNode> _children;
         private MeasureFunction _measureFunction;
+        private CSSMeasureFunc _cssMeasureFunc;
+        private MeasureOutput _measureOutput;
         private object _data;
 
         public CSSNode()
         {
             CSSAssert.Initialize();
             CSSLogger.Initialize();
-            _children = new List<CSSNode>(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<CSSNode>(4);
+            }
             _children.Insert(index, node);
             node._parent = new WeakReference(this);
             Native.CSSNodeInsertChild(_cssNode, node._cssNode, (uint)index);
@@ -483,27 +488,44 @@ namespace Facebook.CSSLayout
         }
 
         public void Clear()
+        {
+            if (_children != null)
             {
                 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, _measureOutput);
-            _measureFunction(this, width, widthMode, height, heightMode, measureResult);
 
-            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<CSSNode> GetEnumerator()
         {
-            return ((IEnumerable<CSSNode>)_children).GetEnumerator();
+            return _children != null ? ((IEnumerable<CSSNode>)_children).GetEnumerator() :
+                System.Linq.Enumerable.Empty<CSSNode>().GetEnumerator();
         }
 
         IEnumerator IEnumerable.GetEnumerator()
         {
-            return ((IEnumerable<CSSNode>)_children).GetEnumerator();
+            return _children != null ? ((IEnumerable<CSSNode>)_children).GetEnumerator() :
+                System.Linq.Enumerable.Empty<CSSNode>().GetEnumerator();
         }
 
         public static int GetInstanceCount()

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
     }
 }