Reland path vol tracker (#23840)

This time making sure to deref the native object on GC.

Author: dnfield

ci/licenses_golden/licenses_flutter

@@ -359,6 +359,7 @@ FILE: ../../../flutter/lib/ui/painting/path.cc
 FILE: ../../../flutter/lib/ui/painting/path.h
 FILE: ../../../flutter/lib/ui/painting/path_measure.cc
 FILE: ../../../flutter/lib/ui/painting/path_measure.h
+FILE: ../../../flutter/lib/ui/painting/path_unittests.cc
 FILE: ../../../flutter/lib/ui/painting/picture.cc
 FILE: ../../../flutter/lib/ui/painting/picture.h
 FILE: ../../../flutter/lib/ui/painting/picture_recorder.cc
@@ -402,6 +403,8 @@ FILE: ../../../flutter/lib/ui/ui.dart
 FILE: ../../../flutter/lib/ui/ui_benchmarks.cc
 FILE: ../../../flutter/lib/ui/ui_dart_state.cc
 FILE: ../../../flutter/lib/ui/ui_dart_state.h
+FILE: ../../../flutter/lib/ui/volatile_path_tracker.cc
+FILE: ../../../flutter/lib/ui/volatile_path_tracker.h
 FILE: ../../../flutter/lib/ui/window.dart
 FILE: ../../../flutter/lib/ui/window/platform_configuration.cc
 FILE: ../../../flutter/lib/ui/window/platform_configuration.h

fml/trace_event.h

@@ -68,6 +68,19 @@
   ::fml::tracing::TraceCounter((category_group), (name), (counter_id), (arg1), \
                                __VA_ARGS__);
 
+// Avoid using the same `name` and `argX_name` for nested traces, which can
+// lead to double free errors. E.g. the following code should be avoided:
+//
+// ```cpp
+// {
+//    TRACE_EVENT1("flutter", "Foo::Bar", "count", "initial_count_value");
+//    ...
+//    TRACE_EVENT_INSTANT1("flutter", "Foo::Bar",
+//                         "count", "updated_count_value");
+// }
+// ```
+//
+// Instead, either use different `name` or `arg1` parameter names.
 #define FML_TRACE_EVENT(category_group, name, ...)                   \
   ::fml::tracing::TraceEvent((category_group), (name), __VA_ARGS__); \
   __FML__AUTO_TRACE_END(name)

lib/ui/BUILD.gn

@@ -91,6 +91,8 @@ source_set("ui") {
     "text/text_box.h",
     "ui_dart_state.cc",
     "ui_dart_state.h",
+    "volatile_path_tracker.cc",
+    "volatile_path_tracker.h",
     "window/platform_configuration.cc",
     "window/platform_configuration.h",
     "window/platform_message.cc",
@@ -191,6 +193,7 @@ if (enable_unittests) {
     sources = [
       "painting/image_dispose_unittests.cc",
       "painting/image_encoding_unittests.cc",
+      "painting/path_unittests.cc",
       "painting/vertices_unittests.cc",
       "window/platform_configuration_unittests.cc",
       "window/pointer_data_packet_converter_unittests.cc",

lib/ui/fixtures/ui_test.dart

@@ -37,6 +37,18 @@ void createVertices() {
 }
 void _validateVertices(Vertices vertices) native 'ValidateVertices';
 
+@pragma('vm:entry-point')
+void createPath() {
+  final Path path = Path()..lineTo(10, 10);
+  _validatePath(path);
+  // Arbitrarily hold a reference to the path to make sure it does not get
+  // garbage collected.
+  Future<void>.delayed(const Duration(days: 100)).then((_) {
+    path.lineTo(100, 100);
+  });
+}
+void _validatePath(Path path) native 'ValidatePath';
+
 @pragma('vm:entry-point')
 void frameCallback(FrameInfo info) {
   print('called back');

lib/ui/painting/path.cc

@@ -67,43 +67,70 @@ void CanvasPath::RegisterNatives(tonic::DartLibraryNatives* natives) {
                      FOR_EACH_BINDING(DART_REGISTER_NATIVE)});
 }
 
-CanvasPath::CanvasPath() {}
+CanvasPath::CanvasPath()
+    : path_tracker_(UIDartState::Current()->GetVolatilePathTracker()),
+      tracked_path_(std::make_shared<VolatilePathTracker::TrackedPath>()) {
+  FML_DCHECK(path_tracker_);
+  resetVolatility();
+}
+
+CanvasPath::~CanvasPath() = default;
+
+void CanvasPath::resetVolatility() {
+  if (!tracked_path_->tracking_volatility) {
+    mutable_path().setIsVolatile(true);
+    tracked_path_->frame_count = 0;
+    tracked_path_->tracking_volatility = true;
+    path_tracker_->Insert(tracked_path_);
+  }
+}
 
-CanvasPath::~CanvasPath() {}
+void CanvasPath::ReleaseDartWrappableReference() const {
+  FML_DCHECK(path_tracker_);
+  path_tracker_->Erase(tracked_path_);
+  RefCountedDartWrappable::ReleaseDartWrappableReference();
+}
 
 int CanvasPath::getFillType() {
-  return static_cast<int>(path_.getFillType());
+  return static_cast<int>(path().getFillType());
 }
 
 void CanvasPath::setFillType(int fill_type) {
-  path_.setFillType(static_cast<SkPathFillType>(fill_type));
+  mutable_path().setFillType(static_cast<SkPathFillType>(fill_type));
+  resetVolatility();
 }
 
 void CanvasPath::moveTo(float x, float y) {
-  path_.moveTo(x, y);
+  mutable_path().moveTo(x, y);
+  resetVolatility();
 }
 
 void CanvasPath::relativeMoveTo(float x, float y) {
-  path_.rMoveTo(x, y);
+  mutable_path().rMoveTo(x, y);
+  resetVolatility();
 }
 
 void CanvasPath::lineTo(float x, float y) {
-  path_.lineTo(x, y);
+  mutable_path().lineTo(x, y);
+  resetVolatility();
 }
 
 void CanvasPath::relativeLineTo(float x, float y) {
-  path_.rLineTo(x, y);
+  mutable_path().rLineTo(x, y);
+  resetVolatility();
 }
 
 void CanvasPath::quadraticBezierTo(float x1, float y1, float x2, float y2) {
-  path_.quadTo(x1, y1, x2, y2);
+  mutable_path().quadTo(x1, y1, x2, y2);
+  resetVolatility();
 }
 
 void CanvasPath::relativeQuadraticBezierTo(float x1,
                                            float y1,
                                            float x2,
                                            float y2) {
-  path_.rQuadTo(x1, y1, x2, y2);
+  mutable_path().rQuadTo(x1, y1, x2, y2);
+  resetVolatility();
 }
 
 void CanvasPath::cubicTo(float x1,
@@ -112,7 +139,8 @@ void CanvasPath::cubicTo(float x1,
                          float y2,
                          float x3,
                          float y3) {
-  path_.cubicTo(x1, y1, x2, y2, x3, y3);
+  mutable_path().cubicTo(x1, y1, x2, y2, x3, y3);
+  resetVolatility();
 }
 
 void CanvasPath::relativeCubicTo(float x1,
@@ -121,19 +149,22 @@ void CanvasPath::relativeCubicTo(float x1,
                                  float y2,
                                  float x3,
                                  float y3) {
-  path_.rCubicTo(x1, y1, x2, y2, x3, y3);
+  mutable_path().rCubicTo(x1, y1, x2, y2, x3, y3);
+  resetVolatility();
 }
 
 void CanvasPath::conicTo(float x1, float y1, float x2, float y2, float w) {
-  path_.conicTo(x1, y1, x2, y2, w);
+  mutable_path().conicTo(x1, y1, x2, y2, w);
+  resetVolatility();
 }
 
 void CanvasPath::relativeConicTo(float x1,
                                  float y1,
                                  float x2,
                                  float y2,
                                  float w) {
-  path_.rConicTo(x1, y1, x2, y2, w);
+  mutable_path().rConicTo(x1, y1, x2, y2, w);
+  resetVolatility();
 }
 
 void CanvasPath::arcTo(float left,
@@ -143,9 +174,10 @@ void CanvasPath::arcTo(float left,
                        float startAngle,
                        float sweepAngle,
                        bool forceMoveTo) {
-  path_.arcTo(SkRect::MakeLTRB(left, top, right, bottom),
-              startAngle * 180.0 / M_PI, sweepAngle * 180.0 / M_PI,
-              forceMoveTo);
+  mutable_path().arcTo(SkRect::MakeLTRB(left, top, right, bottom),
+                       startAngle * 180.0 / M_PI, sweepAngle * 180.0 / M_PI,
+                       forceMoveTo);
+  resetVolatility();
 }
 
 void CanvasPath::arcToPoint(float arcEndX,
@@ -160,8 +192,9 @@ void CanvasPath::arcToPoint(float arcEndX,
   const auto direction =
       isClockwiseDirection ? SkPathDirection::kCW : SkPathDirection::kCCW;
 
-  path_.arcTo(radiusX, radiusY, xAxisRotation, arcSize, direction, arcEndX,
-              arcEndY);
+  mutable_path().arcTo(radiusX, radiusY, xAxisRotation, arcSize, direction,
+                       arcEndX, arcEndY);
+  resetVolatility();
 }
 
 void CanvasPath::relativeArcToPoint(float arcEndDeltaX,
@@ -175,16 +208,19 @@ void CanvasPath::relativeArcToPoint(float arcEndDeltaX,
                                   : SkPath::ArcSize::kSmall_ArcSize;
   const auto direction =
       isClockwiseDirection ? SkPathDirection::kCW : SkPathDirection::kCCW;
-  path_.rArcTo(radiusX, radiusY, xAxisRotation, arcSize, direction,
-               arcEndDeltaX, arcEndDeltaY);
+  mutable_path().rArcTo(radiusX, radiusY, xAxisRotation, arcSize, direction,
+                        arcEndDeltaX, arcEndDeltaY);
+  resetVolatility();
 }
 
 void CanvasPath::addRect(float left, float top, float right, float bottom) {
-  path_.addRect(SkRect::MakeLTRB(left, top, right, bottom));
+  mutable_path().addRect(SkRect::MakeLTRB(left, top, right, bottom));
+  resetVolatility();
 }
 
 void CanvasPath::addOval(float left, float top, float right, float bottom) {
-  path_.addOval(SkRect::MakeLTRB(left, top, right, bottom));
+  mutable_path().addOval(SkRect::MakeLTRB(left, top, right, bottom));
+  resetVolatility();
 }
 
 void CanvasPath::addArc(float left,
@@ -193,25 +229,29 @@ void CanvasPath::addArc(float left,
                         float bottom,
                         float startAngle,
                         float sweepAngle) {
-  path_.addArc(SkRect::MakeLTRB(left, top, right, bottom),
-               startAngle * 180.0 / M_PI, sweepAngle * 180.0 / M_PI);
+  mutable_path().addArc(SkRect::MakeLTRB(left, top, right, bottom),
+                        startAngle * 180.0 / M_PI, sweepAngle * 180.0 / M_PI);
+  resetVolatility();
 }
 
 void CanvasPath::addPolygon(const tonic::Float32List& points, bool close) {
-  path_.addPoly(reinterpret_cast<const SkPoint*>(points.data()),
-                points.num_elements() / 2, close);
+  mutable_path().addPoly(reinterpret_cast<const SkPoint*>(points.data()),
+                         points.num_elements() / 2, close);
+  resetVolatility();
 }
 
 void CanvasPath::addRRect(const RRect& rrect) {
-  path_.addRRect(rrect.sk_rrect);
+  mutable_path().addRRect(rrect.sk_rrect);
+  resetVolatility();
 }
 
 void CanvasPath::addPath(CanvasPath* path, double dx, double dy) {
   if (!path) {
     Dart_ThrowException(ToDart("Path.addPath called with non-genuine Path."));
     return;
   }
-  path_.addPath(path->path(), dx, dy, SkPath::kAppend_AddPathMode);
+  mutable_path().addPath(path->path(), dx, dy, SkPath::kAppend_AddPathMode);
+  resetVolatility();
 }
 
 void CanvasPath::addPathWithMatrix(CanvasPath* path,
@@ -227,8 +267,9 @@ void CanvasPath::addPathWithMatrix(CanvasPath* path,
   SkMatrix matrix = ToSkMatrix(matrix4);
   matrix.setTranslateX(matrix.getTranslateX() + dx);
   matrix.setTranslateY(matrix.getTranslateY() + dy);
-  path_.addPath(path->path(), matrix, SkPath::kAppend_AddPathMode);
+  mutable_path().addPath(path->path(), matrix, SkPath::kAppend_AddPathMode);
   matrix4.Release();
+  resetVolatility();
 }
 
 void CanvasPath::extendWithPath(CanvasPath* path, double dx, double dy) {
@@ -237,7 +278,8 @@ void CanvasPath::extendWithPath(CanvasPath* path, double dx, double dy) {
         ToDart("Path.extendWithPath called with non-genuine Path."));
     return;
   }
-  path_.addPath(path->path(), dx, dy, SkPath::kExtend_AddPathMode);
+  mutable_path().addPath(path->path(), dx, dy, SkPath::kExtend_AddPathMode);
+  resetVolatility();
 }
 
 void CanvasPath::extendWithPathAndMatrix(CanvasPath* path,
@@ -253,37 +295,43 @@ void CanvasPath::extendWithPathAndMatrix(CanvasPath* path,
   SkMatrix matrix = ToSkMatrix(matrix4);
   matrix.setTranslateX(matrix.getTranslateX() + dx);
   matrix.setTranslateY(matrix.getTranslateY() + dy);
-  path_.addPath(path->path(), matrix, SkPath::kExtend_AddPathMode);
+  mutable_path().addPath(path->path(), matrix, SkPath::kExtend_AddPathMode);
   matrix4.Release();
+  resetVolatility();
 }
 
 void CanvasPath::close() {
-  path_.close();
+  mutable_path().close();
+  resetVolatility();
 }
 
 void CanvasPath::reset() {
-  path_.reset();
+  mutable_path().reset();
+  resetVolatility();
 }
 
 bool CanvasPath::contains(double x, double y) {
-  return path_.contains(x, y);
+  return path().contains(x, y);
 }
 
 void CanvasPath::shift(Dart_Handle path_handle, double dx, double dy) {
   fml::RefPtr<CanvasPath> path = CanvasPath::Create(path_handle);
-  path_.offset(dx, dy, &path->path_);
+  auto& other_mutable_path = path->mutable_path();
+  mutable_path().offset(dx, dy, &other_mutable_path);
+  resetVolatility();
 }
 
 void CanvasPath::transform(Dart_Handle path_handle,
                            tonic::Float64List& matrix4) {
   fml::RefPtr<CanvasPath> path = CanvasPath::Create(path_handle);
-  path_.transform(ToSkMatrix(matrix4), &path->path_);
+  auto& other_mutable_path = path->mutable_path();
+  mutable_path().transform(ToSkMatrix(matrix4), &other_mutable_path);
   matrix4.Release();
 }
 
 tonic::Float32List CanvasPath::getBounds() {
   tonic::Float32List rect(Dart_NewTypedData(Dart_TypedData_kFloat32, 4));
-  const SkRect& bounds = path_.getBounds();
+  const SkRect& bounds = path().getBounds();
   rect[0] = bounds.left();
   rect[1] = bounds.top();
   rect[2] = bounds.right();
@@ -293,21 +341,22 @@ tonic::Float32List CanvasPath::getBounds() {
 
 bool CanvasPath::op(CanvasPath* path1, CanvasPath* path2, int operation) {
   return Op(path1->path(), path2->path(), static_cast<SkPathOp>(operation),
-            &path_);
+            &tracked_path_->path);
+  resetVolatility();
 }
 
 void CanvasPath::clone(Dart_Handle path_handle) {
   fml::RefPtr<CanvasPath> path = CanvasPath::Create(path_handle);
   // per Skia docs, this will create a fast copy
   // data is shared until the source path or dest path are mutated
-  path->path_ = path_;
+  path->mutable_path() = this->path();
 }
 
 // This is doomed to be called too early, since Paths are mutable.
 // However, it can help for some of the clone/shift/transform type methods
 // where the resultant path will initially have a meaningful size.
 size_t CanvasPath::GetAllocationSize() const {
-  return sizeof(CanvasPath) + path_.approximateBytesUsed();
+  return sizeof(CanvasPath) + path().approximateBytesUsed();
 }
 
 }  // namespace flutter