From 4adb0ba6467912b97683ff88510af4c8e4ea3c50 Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Mon, 3 Apr 2023 13:02:01 -0700 Subject: [PATCH 1/2] [web] fix SkFinalizationRegistry for dart2js --- ci/licenses_golden/licenses_flutter | 2 + lib/web_ui/lib/src/engine.dart | 1 + .../lib/src/engine/canvaskit/canvas.dart | 2 - .../src/engine/canvaskit/canvaskit_api.dart | 9 +- .../lib/src/engine/canvaskit/image.dart | 56 +--- .../src/engine/canvaskit/native_memory.dart | 216 ++++++++++++++ .../lib/src/engine/canvaskit/picture.dart | 3 +- .../engine/canvaskit/skia_object_cache.dart | 276 ------------------ lib/web_ui/lib/src/engine/canvaskit/text.dart | 223 ++++---------- .../test/canvaskit/canvaskit_api_test.dart | 11 +- .../test/canvaskit/image_golden_test.dart | 32 +- .../test/canvaskit/native_memory_test.dart | 258 ++++++++++++++++ .../canvaskit/skia_objects_cache_test.dart | 238 --------------- 13 files changed, 557 insertions(+), 770 deletions(-) create mode 100644 lib/web_ui/lib/src/engine/canvaskit/native_memory.dart create mode 100644 lib/web_ui/test/canvaskit/native_memory_test.dart diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index f636a268e2994..46c68743f2e8b 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -1878,6 +1878,7 @@ ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/layer_scene_builder ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/layer_tree.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/mask_filter.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/n_way_canvas.dart + ../../../flutter/LICENSE +ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/native_memory.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/noto_font.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/painting.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/path.dart + ../../../flutter/LICENSE @@ -4464,6 +4465,7 @@ FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/layer_scene_builder.d FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/layer_tree.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/mask_filter.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/n_way_canvas.dart +FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/native_memory.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/noto_font.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/painting.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/path.dart diff --git a/lib/web_ui/lib/src/engine.dart b/lib/web_ui/lib/src/engine.dart index a82b9d9b82a0e..07e54ca03c127 100644 --- a/lib/web_ui/lib/src/engine.dart +++ b/lib/web_ui/lib/src/engine.dart @@ -39,6 +39,7 @@ export 'engine/canvaskit/layer_scene_builder.dart'; export 'engine/canvaskit/layer_tree.dart'; export 'engine/canvaskit/mask_filter.dart'; export 'engine/canvaskit/n_way_canvas.dart'; +export 'engine/canvaskit/native_memory.dart'; export 'engine/canvaskit/noto_font.dart'; export 'engine/canvaskit/painting.dart'; export 'engine/canvaskit/path.dart'; diff --git a/lib/web_ui/lib/src/engine/canvaskit/canvas.dart b/lib/web_ui/lib/src/engine/canvaskit/canvas.dart index 3d35df90b4c41..9a5dd90426cc7 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/canvas.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/canvas.dart @@ -215,7 +215,6 @@ class CkCanvas { offset.dx, offset.dy, ); - paragraph.markUsed(); } void drawPath(CkPath path, CkPaint paint) { @@ -1112,7 +1111,6 @@ class CkDrawParagraphCommand extends CkPaintCommand { offset.dx, offset.dy, ); - paragraph.markUsed(); } } diff --git a/lib/web_ui/lib/src/engine/canvaskit/canvaskit_api.dart b/lib/web_ui/lib/src/engine/canvaskit/canvaskit_api.dart index 4aee9ba55ee9c..2d6b8fde36689 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/canvaskit_api.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/canvaskit_api.dart @@ -3569,9 +3569,12 @@ extension JsConstructorExtension on JsConstructor { @JS('window.FinalizationRegistry') @staticInterop class SkObjectFinalizationRegistry { - // TODO(hterkelsen): Add a type for the `cleanup` function when - // native constructors support type parameters. - external factory SkObjectFinalizationRegistry(JSFunction cleanup); + factory SkObjectFinalizationRegistry(JSFunction cleanup) { + return js_util.callConstructor( + _finalizationRegistryConstructor!.toObjectShallow, + [cleanup], + ); + } } extension SkObjectFinalizationRegistryExtension on SkObjectFinalizationRegistry { diff --git a/lib/web_ui/lib/src/engine/canvaskit/image.dart b/lib/web_ui/lib/src/engine/canvaskit/image.dart index e43157da53138..3e167422d992a 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/image.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/image.dart @@ -15,10 +15,10 @@ import 'canvas.dart'; import 'canvaskit_api.dart'; import 'image_wasm_codecs.dart'; import 'image_web_codecs.dart'; +import 'native_memory.dart'; import 'painting.dart'; import 'picture.dart'; import 'picture_recorder.dart'; -import 'skia_object_cache.dart'; /// Instantiates a [ui.Codec] backed by an `SkAnimatedImage` from Skia. FutureOr skiaInstantiateImageCodec(Uint8List list, @@ -227,52 +227,8 @@ Future readChunked(HttpFetchPayload payload, int contentLength, WebOn /// A [ui.Image] backed by an `SkImage` from Skia. class CkImage implements ui.Image, StackTraceDebugger { CkImage(SkImage skImage, { this.videoFrame }) { + box = CountedRef(skImage, this, 'SkImage'); _init(); - if (browserSupportsFinalizationRegistry) { - box = SkiaObjectBox(this, skImage); - } else { - // If finalizers are not supported we need to be able to resurrect the - // image if it was temporarily deleted. To do that, we keep the original - // pixels and ask the SkiaObjectBox to make an image from them when - // resurrecting. - // - // IMPORTANT: the alphaType, colorType, and colorSpace passed to - // _encodeImage and to canvasKit.MakeImage must be the same. Otherwise - // Skia will misinterpret the pixels and corrupt the image. - final ByteData? originalBytes = _encodeImage( - skImage: skImage, - format: ui.ImageByteFormat.rawRgba, - alphaType: canvasKit.AlphaType.Premul, - colorType: canvasKit.ColorType.RGBA_8888, - colorSpace: SkColorSpaceSRGB, - ); - if (originalBytes == null) { - printWarning('Unable to encode image to bytes. We will not ' - 'be able to resurrect it once it has been garbage collected.'); - return; - } - final int originalWidth = skImage.width().toInt(); - final int originalHeight = skImage.height().toInt(); - box = SkiaObjectBox.resurrectable(this, skImage, () { - final SkImage? skImage = canvasKit.MakeImage( - SkImageInfo( - alphaType: canvasKit.AlphaType.Premul, - colorType: canvasKit.ColorType.RGBA_8888, - colorSpace: SkColorSpaceSRGB, - width: originalWidth.toDouble(), - height: originalHeight.toDouble(), - ), - originalBytes.buffer.asUint8List(), - (4 * originalWidth).toDouble(), - ); - if (skImage == null) { - throw ImageCodecException( - 'Failed to resurrect image from pixels.' - ); - } - return skImage; - }); - } } CkImage.cloneOf(this.box, {this.videoFrame}) { @@ -291,9 +247,9 @@ class CkImage implements ui.Image, StackTraceDebugger { StackTrace get debugStackTrace => _debugStackTrace; late StackTrace _debugStackTrace; - // Use a box because `SkImage` may be deleted either due to this object + // Use ref counting because `SkImage` may be deleted either due to this object // being garbage-collected, or by an explicit call to [delete]. - late final SkiaObjectBox box; + late final CountedRef box; /// For browsers that support `ImageDecoder` this field holds the video frame /// from which this image was created. @@ -305,9 +261,9 @@ class CkImage implements ui.Image, StackTraceDebugger { /// The underlying Skia image object. /// - /// Do not store the returned value. It is memory-managed by [SkiaObjectBox]. + /// Do not store the returned value. It is memory-managed by [CountedRef]. /// Storing it may result in use-after-free bugs. - SkImage get skImage => box.skiaObject; + SkImage get skImage => box.nativeObject; bool _disposed = false; diff --git a/lib/web_ui/lib/src/engine/canvaskit/native_memory.dart b/lib/web_ui/lib/src/engine/canvaskit/native_memory.dart new file mode 100644 index 0000000000000..45b051c5058e1 --- /dev/null +++ b/lib/web_ui/lib/src/engine/canvaskit/native_memory.dart @@ -0,0 +1,216 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:js_interop'; +import 'package:meta/meta.dart'; + +import '../../engine.dart' show Instrumentation; +import '../util.dart'; +import 'canvaskit_api.dart'; + +/// Collects native objects that weren't explicitly disposed of using +/// [UniqueRef.dispose] or [CountedRef.unref]. +SkObjectFinalizationRegistry _finalizationRegistry = SkObjectFinalizationRegistry( + (UniqueRef uniq) { + uniq.collect(); + }.toJS +); + +NativeMemoryFinalizationRegistry nativeMemoryFinalizationRegistry = NativeMemoryFinalizationRegistry(); + +/// An indirection to [SkObjectFinalizationRegistry] to enable tests provide a +/// mock implementation of a finalization registry. +class NativeMemoryFinalizationRegistry { + void register(Object owner, UniqueRef ref) { + _finalizationRegistry.register(owner, ref); + } +} + +/// Manages the lifecycle of a C++ object referenced by a single Dart object. +/// +/// It is expected that when the C++ object is no longer needed [dispose] is +/// called. +/// +/// To prevent memory leaks, the underlying C++ object is deleted by the GC if +/// it wasn't previously disposed of explicitly. +class UniqueRef { + UniqueRef(Object owner, T nativeObject, this._debugOwnerLabel) { + _nativeObject = nativeObject; + if (Instrumentation.enabled) { + Instrumentation.instance.incrementCounter('$_debugOwnerLabel Created'); + } + nativeMemoryFinalizationRegistry.register(owner, this); + } + + T? _nativeObject; + final String _debugOwnerLabel; + + /// Returns the underlying native object reference, if it has not been + /// disposed of yet. + /// + /// The returned reference must not be stored. I should only be borrowed + /// temporarily. Storing this reference may result in dangling pointer errors. + T get nativeObject { + assert(!isDisposed, 'Native object was disposed.'); + return _nativeObject!; + } + + /// Returns whether the underlying native object has been disposed and + /// therefore can no longer be used. + bool get isDisposed => _nativeObject == null; + + /// Disposes the underlying native object. + /// + /// The underlying object may be deleted or its ref count may be bumped down. + /// The exact action taken depends on the sharing model of that particular + /// object. For example, an [SkImage] may not be immediately deleted if a + /// [SkPicture] exists that still references it. On the other hand, [SkPaint] + /// is deleted eagerly. + void dispose() { + assert(!isDisposed, 'A native object reference cannot be disposed more than once.'); + if (Instrumentation.enabled) { + Instrumentation.instance.incrementCounter('$_debugOwnerLabel Deleted'); + } + final SkDeletable object = nativeObject as SkDeletable; + if (!object.isDeleted()) { + object.delete(); + } + _nativeObject = null; + } + + /// Called by the garbage [Collector] when the owner of this handle is + /// collected. + /// + /// Garbage collection is used as a back-up for the cases when the handle + /// isn't disposed of explicitly by calling [dispose]. It most likely + /// indicates a memory leak or inefficiency in the framework or application + /// code. + @visibleForTesting + void collect() { + if (!isDisposed) { + if (Instrumentation.enabled) { + Instrumentation.instance.incrementCounter('$_debugOwnerLabel Leaked'); + } + dispose(); + } + } +} + +/// Interface that classes wrapping [UniqueRef] must implement. +/// +/// Used to collect stack traces in debug mode. +abstract class StackTraceDebugger { + /// The stack trace pointing to code location that created or upreffed a + /// [CountedRef]. + StackTrace get debugStackTrace; +} + +/// Manages the lifecycle of a C++ object referenced by multiple Dart objects. +/// +/// Uses reference counting to manage the lifecycle of the C++ object. +/// +/// If the C++ object has a unique owner, use [UniqueRef] instead. +/// +/// The [ref] method can be used to increment the refcount to tell this box to +/// keep the underlying C++ object alive. +/// +/// The [unref] method can be used to decrement the refcount indicating that a +/// referring object no longer needs it. When the refcount drops to zero the +/// underlying C++ object is deleted. +/// +/// In addition to ref counting, this object is also managed by GC. When this +/// reference is garbage collected, the underlying C++ object is automatically +/// deleted. This is mostly done to prevent memory leaks in production. Well +/// behaving framework and app code are expected to rely on [ref] and [unref] +/// for timely collection of resources. +class CountedRef { + /// Creates a counted reference. + CountedRef(T nativeObject, R debugReferrer, String debugLabel) { + _ref = UniqueRef(this, nativeObject, debugLabel); + if (assertionsEnabled) { + debugReferrers.add(debugReferrer); + } + assert(refCount == debugReferrers.length); + } + + /// The native object reference whose lifecycle is being managed by this ref + /// count. + /// + /// Do not store this value outside this class. + late final UniqueRef _ref; + + /// Returns the underlying native object reference, if it has not been + /// disposed of yet. + /// + /// The returned reference must not be stored. I should only be borrowed + /// temporarily. Storing this reference may result in dangling pointer errors. + T get nativeObject => _ref.nativeObject; + + /// The number of objects sharing references to this box. + /// + /// When this count reaches zero, the underlying [nativeObject] is scheduled + /// for deletion. + int get refCount => _refCount; + int _refCount = 1; + + /// Whether the underlying [nativeObject] has been disposed and is no longer + /// accessible. + bool get isDisposed => _ref.isDisposed; + + /// When assertions are enabled, stores all objects that share this box. + /// + /// The length of this list is always identical to [refCount]. + /// + /// This list can be used for debugging ref counting issues. + final Set debugReferrers = {}; + + /// If asserts are enabled, the [StackTrace]s representing when a reference + /// was created. + List debugGetStackTraces() { + if (assertionsEnabled) { + return debugReferrers + .map((R referrer) => referrer.debugStackTrace) + .toList(); + } + throw UnsupportedError(''); + } + + /// Increases the reference count of this box because a new object began + /// sharing ownership of the underlying [nativeObject]. + void ref(R debugReferrer) { + assert( + !_ref.isDisposed, + 'Cannot increment ref count on a deleted handle.', + ); + assert(_refCount > 0); + assert( + debugReferrers.add(debugReferrer), + 'Attempted to increment ref count by the same referrer more than once.', + ); + _refCount += 1; + assert(refCount == debugReferrers.length); + } + + /// Decrements the reference count for the [nativeObject]. + /// + /// Does nothing if the object has already been deleted. + /// + /// If this causes the reference count to drop to zero, deletes the + /// [nativeObject]. + void unref(R debugReferrer) { + assert( + !_ref.isDisposed, + 'Attempted to unref an already deleted native object.', + ); + assert( + debugReferrers.remove(debugReferrer), + 'Attempted to decrement ref count by the same referrer more than once.', + ); + _refCount -= 1; + assert(refCount == debugReferrers.length); + if (_refCount == 0) { + _ref.dispose(); + } + } +} diff --git a/lib/web_ui/lib/src/engine/canvaskit/picture.dart b/lib/web_ui/lib/src/engine/canvaskit/picture.dart index 28511bbba7bc3..936d5897ba9ec 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/picture.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/picture.dart @@ -46,8 +46,7 @@ class CkPicture extends ManagedSkiaObject implements ui.Picture { /// false. /// /// This extra flag is necessary on top of [rawSkiaObject] because - /// [rawSkiaObject] being null does not indicate permanent deletion. See - /// similar flag [SkiaObjectBox.isDeletedPermanently]. + /// [rawSkiaObject] being null does not indicate permanent deletion. bool _isDisposed = false; /// The stack trace taken when [dispose] was called. diff --git a/lib/web_ui/lib/src/engine/canvaskit/skia_object_cache.dart b/lib/web_ui/lib/src/engine/canvaskit/skia_object_cache.dart index 21debea93cfb9..677397b953082 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/skia_object_cache.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/skia_object_cache.dart @@ -7,7 +7,6 @@ import 'dart:collection'; import 'package:meta/meta.dart'; import '../../engine.dart' show Instrumentation; -import '../util.dart'; import 'canvaskit_api.dart'; import 'renderer.dart'; @@ -88,85 +87,6 @@ class SkiaObjectCache { } } -/// Like [SkiaObjectCache] but enforces the [maximumSize] of the cache -/// synchronously instead of waiting until a post-frame callback. -class SynchronousSkiaObjectCache { - SynchronousSkiaObjectCache(this.maximumSize) - : _itemQueue = DoubleLinkedQueue>(), - _itemMap = , DoubleLinkedQueueEntry>>{}; - - /// This cache will never exceed this limit, even temporarily. - final int maximumSize; - - /// A doubly linked list of the objects in the cache. - /// - /// This makes it fast to move a recently used object to the front. - final DoubleLinkedQueue> _itemQueue; - - /// A map of objects to their associated node in the [_itemQueue]. - /// - /// This makes it fast to find the node in the queue when we need to - /// move the object to the front of the queue. - final Map, DoubleLinkedQueueEntry>> _itemMap; - - /// The number of objects in the cache. - int get length => _itemQueue.length; - - /// Whether or not [object] is in the cache. - /// - /// This is only for testing. - @visibleForTesting - bool debugContains(SkiaObject object) { - return _itemMap.containsKey(object); - } - - /// Adds [object] to the cache. - /// - /// If adding [object] causes the total size of the cache to exceed - /// [maximumSize], then the least recently used objects are evicted and - /// deleted. - void add(SkiaObject object) { - assert( - !_itemMap.containsKey(object), - 'Cannot add object. Object is already in the cache: $object', - ); - _itemQueue.addFirst(object); - _itemMap[object] = _itemQueue.firstEntry()!; - _enforceCacheLimit(); - } - - /// Marks the [object] as most recently used. - /// - /// If [object] is in the cache returns true. If the object is not in - /// the cache, for example, because it was never added or because it was - /// evicted as a result of the app reaching the cache limit, returns false. - bool markUsed(SkiaObject object) { - final DoubleLinkedQueueEntry>? item = _itemMap[object]; - - if (item == null) { - return false; - } - - item.remove(); - _itemQueue.addFirst(object); - _itemMap[object] = _itemQueue.firstEntry()!; - return true; - } - - /// Ensures the cache does not exceed [maximumSize], evicting objects if - /// necessary. - /// - /// Calls `delete` and `didDelete` on objects evicted from the cache. - void _enforceCacheLimit() { - while (_itemQueue.length > maximumSize) { - final SkiaObject oldObject = _itemQueue.removeLast(); - _itemMap.remove(oldObject); - oldObject.delete(); - oldObject.didDelete(); - } - } -} - /// An object backed by a JavaScript object mapped onto a Skia C++ object in the /// WebAssembly heap. /// @@ -311,205 +231,9 @@ abstract class ManagedSkiaObject extends SkiaObject { bool get isResurrectionExpensive => false; } -/// Interface that classes wrapping [SkiaObjectBox] must implement. -/// -/// Used to collect stack traces in debug mode. -abstract class StackTraceDebugger { - /// The stack trace pointing to code location that created or upreffed a - /// [SkiaObjectBox]. - StackTrace get debugStackTrace; -} - /// A function that restores a Skia object that was temporarily deleted. typedef Resurrector = T Function(); -/// Uses reference counting to manage the lifecycle of a Skia object owned by a -/// wrapper object. -/// -/// The [ref] method can be used to increment the refcount to tell this box to -/// keep the underlying Skia object alive. -/// -/// The [unref] method can be used to decrement the refcount to tell this box -/// that a wrapper object no longer needs it. When the refcount drops to zero -/// the underlying Skia object is deleted permanently (see [isDeletedPermanently]). -/// -/// In addition to ref counting, this object is also managed by GC. In browsers -/// that support [SkFinalizationRegistry] the underlying Skia object is deleted -/// permanently when no JavaScript objects have references to this box. In -/// browsers that do not support [SkFinalizationRegistry] the underlying Skia -/// object may undergo several cycles of temporary deletions and resurrections -/// prior to being deleted permanently. A temporary deletion may effectively -/// be permanent if this object is garbage collected. This is safe because a -/// temporarily deleted object has no C++ resources to collect. -class SkiaObjectBox - extends SkiaObject { - /// Creates an object box that's memory-managed using [SkFinalizationRegistry]. - /// - /// This constructor must only be used if [browserSupportsFinalizationRegistry] is true. - SkiaObjectBox(R debugReferrer, T initialValue) : - assert(browserSupportsFinalizationRegistry), _resurrector = null { - _initialize(debugReferrer, initialValue); - Collector.instance.register(this, _skDeletable!); - } - - /// Creates an object box that's memory-managed using a [Resurrector]. - /// - /// This constructor must only be used if [browserSupportsFinalizationRegistry] is false. - SkiaObjectBox.resurrectable( - R debugReferrer, T initialValue, this._resurrector) : - assert(!browserSupportsFinalizationRegistry) { - _initialize(debugReferrer, initialValue); - if (Instrumentation.enabled) { - Instrumentation.instance.incrementCounter( - '${_skDeletable?.constructor.name} created', - ); - } - SkiaObjects.manageExpensive(this); - } - - void _initialize(R debugReferrer, T initialValue) { - _update(initialValue); - if (assertionsEnabled) { - debugReferrers.add(debugReferrer); - } - assert(refCount == debugReferrers.length); - } - - /// The number of objects sharing references to this box. - /// - /// When this count reaches zero, the underlying [skiaObject] is scheduled - /// for deletion. - int get refCount => _refCount; - int _refCount = 1; - - /// When assertions are enabled, stores all objects that share this box. - /// - /// The length of this list is always identical to [refCount]. - /// - /// This list can be used for debugging ref counting issues. - final Set debugReferrers = {}; - - /// If asserts are enabled, the [StackTrace]s representing when a reference - /// was created. - List debugGetStackTraces() { - if (assertionsEnabled) { - return debugReferrers - .map((R referrer) => referrer.debugStackTrace) - .toList(); - } - throw UnsupportedError(''); - } - - /// The Skia object whose lifecycle is being managed. - /// - /// Do not store this value outside this box. It is memory-managed by - /// [SkiaObjectBox]. Storing it may result in use-after-free bugs. - T? rawSkiaObject; - SkDeletable? _skDeletable; - Resurrector? _resurrector; - - void _update(T? newSkiaObject) { - rawSkiaObject = newSkiaObject; - _skDeletable = newSkiaObject as SkDeletable?; - } - - @override - T get skiaObject => rawSkiaObject ?? _doResurrect(); - - T _doResurrect() { - assert(!browserSupportsFinalizationRegistry); - assert(_resurrector != null); - assert(!_isDeletedPermanently, 'Cannot use deleted object.'); - _update(_resurrector!()); - if (Instrumentation.enabled) { - Instrumentation.instance.incrementCounter( - '${_skDeletable?.constructor.name} resurrected', - ); - } - SkiaObjects.manageExpensive(this); - return skiaObject; - } - - @override - void delete() { - _skDeletable?.delete(); - } - - @override - void didDelete() { - if (Instrumentation.enabled) { - Instrumentation.instance.incrementCounter( - '${_skDeletable?.constructor.name} deleted', - ); - } - assert(!browserSupportsFinalizationRegistry); - _update(null); - } - - /// Whether this object has been deleted permanently. - /// - /// If this is true it will remain true forever, and the Skia object is no - /// longer resurrectable. - /// - /// See also [isDeletedTemporarily]. - bool get isDeletedPermanently => _isDeletedPermanently; - bool _isDeletedPermanently = false; - - /// Whether the underlying [rawSkiaObject] has been deleted, but it may still - /// be resurrected (see [SkiaObjectBox.resurrectable]). - bool get isDeletedTemporarily => - rawSkiaObject == null && !_isDeletedPermanently; - - /// Increases the reference count of this box because a new object began - /// sharing ownership of the underlying [skiaObject]. - /// - /// Clones must be [dispose]d when finished. - void ref(R debugReferrer) { - assert(!_isDeletedPermanently, - 'Cannot increment ref count on a deleted handle.'); - assert(_refCount > 0); - assert( - debugReferrers.add(debugReferrer), - 'Attempted to increment ref count by the same referrer more than once.', - ); - _refCount += 1; - assert(refCount == debugReferrers.length); - } - - /// Decrements the reference count for the [skObject]. - /// - /// Does nothing if the object has already been deleted. - /// - /// If this causes the reference count to drop to zero, deletes the - /// [skObject]. - void unref(R debugReferrer) { - assert(!_isDeletedPermanently, - 'Attempted to unref an already deleted Skia object.'); - assert( - debugReferrers.remove(debugReferrer), - 'Attempted to decrement ref count by the same referrer more than once.', - ); - _refCount -= 1; - assert(refCount == debugReferrers.length); - if (_refCount == 0) { - // The object may be null because it was deleted temporarily, i.e. it was - // expecting the possibility of resurrection. - if (_skDeletable != null) { - if (browserSupportsFinalizationRegistry) { - Collector.instance.collect(_skDeletable!); - } else { - delete(); - didDelete(); - } - } - rawSkiaObject = null; - _skDeletable = null; - _resurrector = null; - _isDeletedPermanently = true; - } - } -} - // ignore: avoid_classes_with_only_static_members /// Singleton that manages the lifecycles of [SkiaObject] instances. class SkiaObjects { diff --git a/lib/web_ui/lib/src/engine/canvaskit/text.dart b/lib/web_ui/lib/src/engine/canvaskit/text.dart index 2b4ab31d53728..ed14a91689099 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/text.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/text.dart @@ -10,6 +10,7 @@ import 'package:ui/ui.dart' as ui; import '../util.dart'; import 'canvaskit_api.dart'; import 'font_fallbacks.dart'; +import 'native_memory.dart'; import 'painting.dart'; import 'renderer.dart'; import 'skia_object_cache.dart'; @@ -561,128 +562,27 @@ SkFontStyle toSkFontStyle(ui.FontWeight? fontWeight, ui.FontStyle? fontStyle) { /// while painting a small subset of it. To achieve this a /// [SynchronousSkiaObjectCache] is used that limits the number of live laid out /// paragraphs at any point in time within or outside the frame. -class CkParagraph extends SkiaObject implements ui.Paragraph { - CkParagraph(this._skParagraph, this._paragraphStyle, this._paragraphCommands); +class CkParagraph implements ui.Paragraph { + CkParagraph(SkParagraph skParagraph, this._paragraphStyle) { + _ref = UniqueRef(this, skParagraph, 'Paragraph'); + } - /// The result of calling `build()` on the JS CkParagraphBuilder. - /// - /// This may be invalidated later. - SkParagraph? _skParagraph; + late final UniqueRef _ref; - /// The paragraph style used to build this paragraph. - /// - /// This is used to resurrect the paragraph if the initial paragraph - /// is deleted. - final CkParagraphStyle _paragraphStyle; + SkParagraph get skiaObject => _ref.nativeObject; - /// The paragraph builder commands used to build this paragraph. + /// The constraints from the last time we laid the paragraph out. /// /// This is used to resurrect the paragraph if the initial paragraph /// is deleted. - final List<_ParagraphCommand> _paragraphCommands; + double _lastLayoutConstraints = double.negativeInfinity; - /// The constraints from the last time we laid the paragraph out. + /// The paragraph style used to build this paragraph. /// /// This is used to resurrect the paragraph if the initial paragraph /// is deleted. - ui.ParagraphConstraints? _lastLayoutConstraints; - - @override - SkParagraph get skiaObject => _ensureInitialized(_lastLayoutConstraints!); - - SkParagraph _ensureInitialized(ui.ParagraphConstraints constraints) { - SkParagraph? paragraph = _skParagraph; - - // Paragraph objects are immutable. It's OK to skip initialization and reuse - // existing object. - bool didRebuildSkiaObject = false; - if (paragraph == null) { - final CkParagraphBuilder builder = CkParagraphBuilder(_paragraphStyle); - for (final _ParagraphCommand command in _paragraphCommands) { - switch (command.type) { - case _ParagraphCommandType.addText: - builder.addText(command.text!); - case _ParagraphCommandType.pop: - builder.pop(); - case _ParagraphCommandType.pushStyle: - builder.pushStyle(command.style!); - case _ParagraphCommandType.addPlaceholder: - builder._addPlaceholder(command.placeholderStyle!); - } - } - paragraph = builder._buildSkParagraph(); - _skParagraph = paragraph; - didRebuildSkiaObject = true; - } - - final bool constraintsChanged = _lastLayoutConstraints != constraints; - if (didRebuildSkiaObject || constraintsChanged) { - _lastLayoutConstraints = constraints; - // TODO(het): CanvasKit throws an exception when laid out with - // a font that wasn't registered. - try { - paragraph.layout(constraints.width); - _alphabeticBaseline = paragraph.getAlphabeticBaseline(); - _didExceedMaxLines = paragraph.didExceedMaxLines(); - _height = paragraph.getHeight(); - _ideographicBaseline = paragraph.getIdeographicBaseline(); - _longestLine = paragraph.getLongestLine(); - _maxIntrinsicWidth = paragraph.getMaxIntrinsicWidth(); - _minIntrinsicWidth = paragraph.getMinIntrinsicWidth(); - _width = paragraph.getMaxWidth(); - _boxesForPlaceholders = - skRectsToTextBoxes(paragraph.getRectsForPlaceholders()); - } catch (e) { - printWarning('CanvasKit threw an exception while laying ' - 'out the paragraph. The font was "${_paragraphStyle._fontFamily}". ' - 'Exception:\n$e'); - rethrow; - } - } - - return paragraph; - } - - // Caches laid out paragraphs and synchronously reclaims memory if there's - // memory pressure. - // - // On May 26, 2021, 500 seemed like a reasonable number to pick for the cache - // size. At the time a single laid out SkParagraph used 100KB of memory. So, - // 500 items in the cache is roughly 50MB of memory, which is not too high, - // while at the same time enough for most use-cases. - // - // TODO(yjbanov): this strategy is not sufficient for the use-case where a - // lot of paragraphs are laid out _and_ rendered. To support - // this use-case without blowing up memory usage we need this: - // https://github.com/flutter/flutter/issues/81224 - static final SynchronousSkiaObjectCache _paragraphCache = - SynchronousSkiaObjectCache(500); - - /// Marks this paragraph as having been used this frame. - /// - /// Puts this paragraph in a [SynchronousSkiaObjectCache], which will delete it - /// if there's memory pressure to do so. This protects our memory usage from - /// blowing up if within a single frame the framework needs to layout a lot of - /// paragraphs. One common use-case is `ListView.builder`, which needs to layout - /// more of its content than it actually renders to compute the scroll position. - void markUsed() { - // If the paragraph is already in the cache, just mark it as most recently - // used. Otherwise, add to cache. - if (!_paragraphCache.markUsed(this)) { - _paragraphCache.add(this); - } - } - - @override - void delete() { - _skParagraph?.delete(); - _skParagraph = null; - } + final CkParagraphStyle _paragraphStyle; - @override - void didDelete() { - _skParagraph = null; - } @override double get alphabeticBaseline => _alphabeticBaseline; @@ -727,12 +627,12 @@ class CkParagraph extends SkiaObject implements ui.Paragraph { ui.BoxHeightStyle boxHeightStyle = ui.BoxHeightStyle.tight, ui.BoxWidthStyle boxWidthStyle = ui.BoxWidthStyle.tight, }) { + assert(!_disposed, 'Paragraph has been disposed.'); if (start < 0 || end < 0) { return const []; } - final SkParagraph paragraph = _ensureInitialized(_lastLayoutConstraints!); - final List skRects = paragraph.getRectsForRange( + final List skRects = skiaObject.getRectsForRange( start.toDouble(), end.toDouble(), toSkRectHeightStyle(boxHeightStyle), @@ -743,6 +643,7 @@ class CkParagraph extends SkiaObject implements ui.Paragraph { } List skRectsToTextBoxes(List skRects) { + assert(!_disposed, 'Paragraph has been disposed.'); final List result = []; for (int i = 0; i < skRects.length; i++) { @@ -763,9 +664,8 @@ class CkParagraph extends SkiaObject implements ui.Paragraph { @override ui.TextPosition getPositionForOffset(ui.Offset offset) { - final SkParagraph paragraph = _ensureInitialized(_lastLayoutConstraints!); - final SkTextPosition positionWithAffinity = - paragraph.getGlyphPositionAtCoordinate( + assert(!_disposed, 'Paragraph has been disposed.'); + final SkTextPosition positionWithAffinity = skiaObject.getGlyphPositionAtCoordinate( offset.dx, offset.dy, ); @@ -774,7 +674,7 @@ class CkParagraph extends SkiaObject implements ui.Paragraph { @override ui.TextRange getWordBoundary(ui.TextPosition position) { - final SkParagraph paragraph = _ensureInitialized(_lastLayoutConstraints!); + assert(!_disposed, 'Paragraph has been disposed.'); final int characterPosition; switch (position.affinity) { case ui.TextAffinity.upstream: @@ -782,26 +682,46 @@ class CkParagraph extends SkiaObject implements ui.Paragraph { case ui.TextAffinity.downstream: characterPosition = position.offset; } - final SkTextRange skRange = paragraph.getWordBoundary(characterPosition.toDouble()); + final SkTextRange skRange = skiaObject.getWordBoundary(characterPosition.toDouble()); return ui.TextRange(start: skRange.start.toInt(), end: skRange.end.toInt()); } @override void layout(ui.ParagraphConstraints constraints) { - if (_lastLayoutConstraints == constraints) { + assert(!_disposed, 'Paragraph has been disposed.'); + if (_lastLayoutConstraints == constraints.width) { return; } - _ensureInitialized(constraints); - // See class-level and _paragraphCache doc comments for why we're releasing - // the paragraph immediately after layout. - markUsed(); + _lastLayoutConstraints = constraints.width; + + // TODO(het): CanvasKit throws an exception when laid out with + // a font that wasn't registered. + try { + final SkParagraph paragraph = skiaObject; + paragraph.layout(constraints.width); + _alphabeticBaseline = paragraph.getAlphabeticBaseline(); + _didExceedMaxLines = paragraph.didExceedMaxLines(); + _height = paragraph.getHeight(); + _ideographicBaseline = paragraph.getIdeographicBaseline(); + _longestLine = paragraph.getLongestLine(); + _maxIntrinsicWidth = paragraph.getMaxIntrinsicWidth(); + _minIntrinsicWidth = paragraph.getMinIntrinsicWidth(); + _width = paragraph.getMaxWidth(); + _boxesForPlaceholders = + skRectsToTextBoxes(paragraph.getRectsForPlaceholders()); + } catch (e) { + printWarning('CanvasKit threw an exception while laying ' + 'out the paragraph. The font was "${_paragraphStyle._fontFamily}". ' + 'Exception:\n$e'); + rethrow; + } } @override ui.TextRange getLineBoundary(ui.TextPosition position) { - final SkParagraph paragraph = _ensureInitialized(_lastLayoutConstraints!); - final List metrics = paragraph.getLineMetrics(); + assert(!_disposed, 'Paragraph has been disposed.'); + final List metrics = skiaObject.getLineMetrics(); final int offset = position.offset; for (final SkLineMetrics metric in metrics) { if (offset >= metric.startIndex && offset <= metric.endIndex) { @@ -813,8 +733,8 @@ class CkParagraph extends SkiaObject implements ui.Paragraph { @override List computeLineMetrics() { - final SkParagraph paragraph = _ensureInitialized(_lastLayoutConstraints!); - final List skLineMetrics = paragraph.getLineMetrics(); + assert(!_disposed, 'Paragraph has been disposed.'); + final List skLineMetrics = skiaObject.getLineMetrics(); final List result = []; for (final SkLineMetrics metric in skLineMetrics) { result.add(CkLineMetrics._(metric)); @@ -826,8 +746,8 @@ class CkParagraph extends SkiaObject implements ui.Paragraph { @override void dispose() { - delete(); - didDelete(); + assert(!_disposed, 'Paragraph has been disposed.'); + _ref.dispose(); _disposed = true; } @@ -877,8 +797,7 @@ class CkLineMetrics implements ui.LineMetrics { class CkParagraphBuilder implements ui.ParagraphBuilder { CkParagraphBuilder(ui.ParagraphStyle style) - : _commands = <_ParagraphCommand>[], - _style = style as CkParagraphStyle, + : _style = style as CkParagraphStyle, _placeholderCount = 0, _placeholderScales = [], _styleStack = [], @@ -891,7 +810,6 @@ class CkParagraphBuilder implements ui.ParagraphBuilder { final SkParagraphBuilder _paragraphBuilder; final CkParagraphStyle _style; - final List<_ParagraphCommand> _commands; int _placeholderCount; final List _placeholderScales; final List _styleStack; @@ -923,7 +841,6 @@ class CkParagraphBuilder implements ui.ParagraphBuilder { } void _addPlaceholder(_CkParagraphPlaceholder placeholderStyle) { - _commands.add(_ParagraphCommand.addPlaceholder(placeholderStyle)); _paragraphBuilder.addPlaceholder( placeholderStyle.width, placeholderStyle.height, @@ -961,14 +878,13 @@ class CkParagraphBuilder implements ui.ParagraphBuilder { fontFamilies.addAll(style.fontFamilyFallback!); } FontFallbackData.instance.ensureFontsSupportText(text, fontFamilies); - _commands.add(_ParagraphCommand.addText(text)); _paragraphBuilder.addText(text); } @override CkParagraph build() { final SkParagraph builtParagraph = _buildSkParagraph(); - return CkParagraph(builtParagraph, _style, _commands); + return CkParagraph(builtParagraph, _style); } /// Builds the CkParagraph with the builder and deletes the builder. @@ -999,7 +915,6 @@ class CkParagraphBuilder implements ui.ParagraphBuilder { } return; } - _commands.add(const _ParagraphCommand.pop()); _styleStack.removeLast(); _paragraphBuilder.pop(); } @@ -1026,7 +941,6 @@ class CkParagraphBuilder implements ui.ParagraphBuilder { final CkTextStyle ckStyle = style as CkTextStyle; final CkTextStyle skStyle = baseStyle.mergeWith(ckStyle); _styleStack.add(skStyle); - _commands.add(_ParagraphCommand.pushStyle(ckStyle)); if (skStyle.foreground != null || skStyle.background != null) { SkPaint? foreground = skStyle.foreground?.skiaObject; if (foreground == null) { @@ -1062,41 +976,6 @@ class _CkParagraphPlaceholder { final double offset; } -class _ParagraphCommand { - const _ParagraphCommand._( - this.type, - this.text, - this.style, - this.placeholderStyle, - ); - - const _ParagraphCommand.addText(String text) - : this._(_ParagraphCommandType.addText, text, null, null); - - const _ParagraphCommand.pop() - : this._(_ParagraphCommandType.pop, null, null, null); - - const _ParagraphCommand.pushStyle(CkTextStyle style) - : this._(_ParagraphCommandType.pushStyle, null, style, null); - - const _ParagraphCommand.addPlaceholder( - _CkParagraphPlaceholder placeholderStyle) - : this._( - _ParagraphCommandType.addPlaceholder, null, null, placeholderStyle); - - final _ParagraphCommandType type; - final String? text; - final CkTextStyle? style; - final _CkParagraphPlaceholder? placeholderStyle; -} - -enum _ParagraphCommandType { - addText, - pop, - pushStyle, - addPlaceholder, -} - List _getEffectiveFontFamilies(String? fontFamily, [List? fontFamilyFallback]) { final List fontFamilies = []; diff --git a/lib/web_ui/test/canvaskit/canvaskit_api_test.dart b/lib/web_ui/test/canvaskit/canvaskit_api_test.dart index 6bdd2b301d54d..16260d41317bf 100644 --- a/lib/web_ui/test/canvaskit/canvaskit_api_test.dart +++ b/lib/web_ui/test/canvaskit/canvaskit_api_test.dart @@ -2,6 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'dart:js_interop'; import 'dart:math'; import 'dart:typed_data'; @@ -1432,7 +1433,6 @@ void _canvasTests() { builder.addText('Hello'); final CkParagraph paragraph = builder.build(); - paragraph.delete(); paragraph.dispose(); expect(paragraph.debugDisposed, true); }); @@ -1894,6 +1894,15 @@ void _paragraphTests() { 'http://localhost:1234/foo/canvaskit.wasm', ); }); + + test('SkObjectFinalizationRegistry', () { + // There's no reliable way to test the actual functionality of + // FinalizationRegistry because it depends on GC, which cannot be controlled, + // So the test simply tests that a FinalizationRegistry can be constructed + // and its `register` method can be called. + final SkObjectFinalizationRegistry registry = SkObjectFinalizationRegistry((String arg) {}.toJS); + registry.register(Object(), Object()); + }); } diff --git a/lib/web_ui/test/canvaskit/image_golden_test.dart b/lib/web_ui/test/canvaskit/image_golden_test.dart index 3fae53fc9d978..ffc9507620f2e 100644 --- a/lib/web_ui/test/canvaskit/image_golden_test.dart +++ b/lib/web_ui/test/canvaskit/image_golden_test.dart @@ -137,10 +137,10 @@ void _testForImageCodecs({required bool useBrowserImageDecoder}) { .makeImageAtCurrentFrame(); final CkImage image = CkImage(skImage); expect(image.debugDisposed, isFalse); - expect(image.box.isDeletedPermanently, isFalse); + expect(image.box.isDisposed, isFalse); image.dispose(); expect(image.debugDisposed, isTrue); - expect(image.box.isDeletedPermanently, isTrue); + expect(image.box.isDisposed, isTrue); // Disallow double-dispose. expect(() => image.dispose(), throwsAssertionError); @@ -152,7 +152,7 @@ void _testForImageCodecs({required bool useBrowserImageDecoder}) { canvasKit.MakeAnimatedImageFromEncoded(kTransparentImage)! .makeImageAtCurrentFrame(); final CkImage image = CkImage(skImage); - final SkiaObjectBox box = image.box; + final CountedRef box = image.box; expect(box.refCount, 1); expect(box.debugGetStackTraces().length, 1); @@ -161,19 +161,19 @@ void _testForImageCodecs({required bool useBrowserImageDecoder}) { expect(box.debugGetStackTraces().length, 2); expect(image.isCloneOf(clone), isTrue); - expect(box.isDeletedPermanently, isFalse); + expect(box.isDisposed, isFalse); testCollector.collectNow(); expect(skImage.isDeleted(), isFalse); image.dispose(); expect(box.refCount, 1); - expect(box.isDeletedPermanently, isFalse); + expect(box.isDisposed, isFalse); testCollector.collectNow(); expect(skImage.isDeleted(), isFalse); clone.dispose(); expect(box.refCount, 0); - expect(box.isDeletedPermanently, isTrue); + expect(box.isDisposed, isTrue); testCollector.collectNow(); expect(skImage.isDeleted(), isTrue); @@ -266,26 +266,6 @@ void _testForImageCodecs({required bool useBrowserImageDecoder}) { // TODO(hterkelsen): Firefox and Safari do not currently support ImageDecoder. }, skip: isFirefox || isSafari); - // Regression test for https://github.com/flutter/flutter/issues/72469 - test('CkImage can be resurrected', () { - browserSupportsFinalizationRegistry = false; - final SkImage skImage = - canvasKit.MakeAnimatedImageFromEncoded(kTransparentImage)! - .makeImageAtCurrentFrame(); - final CkImage image = CkImage(skImage); - expect(image.box.rawSkiaObject, isNotNull); - - // Pretend that the image is temporarily deleted. - image.box.delete(); - image.box.didDelete(); - expect(image.box.rawSkiaObject, isNull); - - // Attempting to access the skia object here would previously throw - // "Stack Overflow" in Safari. - expect(image.box.skiaObject, isNotNull); - testCollector.collectNow(); - }); - test('skiaInstantiateWebImageCodec loads an image from the network', () async { mockHttpFetchResponseFactory = (String url) async { diff --git a/lib/web_ui/test/canvaskit/native_memory_test.dart b/lib/web_ui/test/canvaskit/native_memory_test.dart new file mode 100644 index 0000000000000..42a99d284b65a --- /dev/null +++ b/lib/web_ui/test/canvaskit/native_memory_test.dart @@ -0,0 +1,258 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:js_interop'; +import 'package:js/js.dart'; + +import 'package:test/bootstrap/browser.dart'; +import 'package:test/test.dart'; + +import 'package:ui/src/engine.dart'; + +import 'common.dart'; + +void main() { + internalBootstrapBrowserTest(() => testMain); +} + +void testMain() { + setUpCanvasKitTest(); + + late _MockNativeMemoryFinalizationRegistry mockFinalizationRegistry; + + setUp(() { + TestSkDeletableMock.deleteCount = 0; + nativeMemoryFinalizationRegistry = mockFinalizationRegistry = _MockNativeMemoryFinalizationRegistry(); + }); + + tearDown(() { + nativeMemoryFinalizationRegistry = NativeMemoryFinalizationRegistry(); + }); + + group(UniqueRef, () { + test('create-dispose-collect cycle', () { + expect(mockFinalizationRegistry.registeredPairs, hasLength(0)); + final Object owner = Object(); + final TestSkDeletable nativeObject = TestSkDeletable(); + final UniqueRef ref = UniqueRef(owner, nativeObject, 'TestSkDeletable'); + expect(ref.isDisposed, isFalse); + expect(ref.nativeObject, same(nativeObject)); + expect(TestSkDeletableMock.deleteCount, 0); + expect(mockFinalizationRegistry.registeredPairs, hasLength(1)); + expect(mockFinalizationRegistry.registeredPairs.single.owner, same(owner)); + expect(mockFinalizationRegistry.registeredPairs.single.ref, same(ref)); + + ref.dispose(); + expect(TestSkDeletableMock.deleteCount, 1); + expect(ref.isDisposed, isTrue); + expect( + reason: 'Cannot access object that was disposed', + () => ref.nativeObject, throwsA(isA()), + ); + expect( + reason: 'Cannot dispose object more than once', + () => ref.dispose(), throwsA(isA()), + ); + expect(TestSkDeletableMock.deleteCount, 1); + + // Simulate a GC + mockFinalizationRegistry.registeredPairs.single.ref.collect(); + expect( + reason: 'Manually disposed object should not be deleted again by GC.', + TestSkDeletableMock.deleteCount, 1, + ); + }); + + test('create-collect cycle', () { + expect(mockFinalizationRegistry.registeredPairs, hasLength(0)); + final Object owner = Object(); + final TestSkDeletable nativeObject = TestSkDeletable(); + final UniqueRef ref = UniqueRef(owner, nativeObject, 'TestSkDeletable'); + expect(ref.isDisposed, isFalse); + expect(ref.nativeObject, same(nativeObject)); + expect(TestSkDeletableMock.deleteCount, 0); + expect(mockFinalizationRegistry.registeredPairs, hasLength(1)); + + ref.collect(); + expect(TestSkDeletableMock.deleteCount, 1); + // There's nothing else to test for any practical gain. UniqueRef.collect + // is called when GC decided that the owner is no longer reachable. So + // there must not be anything else calling into this object for anything + // useful. + }); + }); + + group(CountedRef, () { + test('single owner', () { + expect(mockFinalizationRegistry.registeredPairs, hasLength(0)); + final TestSkDeletable nativeObject = TestSkDeletable(); + final TestCountedRefOwner owner = TestCountedRefOwner(nativeObject); + expect(owner.ref.debugReferrers, hasLength(1)); + expect(owner.ref.debugReferrers.single, owner); + expect(owner.ref.refCount, 1); + expect(owner.ref.nativeObject, nativeObject); + expect(TestSkDeletableMock.deleteCount, 0); + expect(mockFinalizationRegistry.registeredPairs, hasLength(1)); + + owner.dispose(); + expect(owner.ref.debugReferrers, isEmpty); + expect(owner.ref.refCount, 0); + expect( + reason: 'Cannot access object that was disposed', + () => owner.ref.nativeObject, throwsA(isA()), + ); + expect(TestSkDeletableMock.deleteCount, 1); + + expect( + reason: 'Cannot dispose object more than once', + () => owner.dispose(), throwsA(isA()), + ); + }); + + test('multiple owners', () { + expect(mockFinalizationRegistry.registeredPairs, hasLength(0)); + final TestSkDeletable nativeObject = TestSkDeletable(); + final TestCountedRefOwner owner1 = TestCountedRefOwner(nativeObject); + expect(owner1.ref.debugReferrers, hasLength(1)); + expect(owner1.ref.debugReferrers.single, owner1); + expect(owner1.ref.refCount, 1); + expect(owner1.ref.nativeObject, nativeObject); + expect(TestSkDeletableMock.deleteCount, 0); + expect(mockFinalizationRegistry.registeredPairs, hasLength(1)); + + final TestCountedRefOwner owner2 = owner1.clone(); + expect(owner2.ref, same(owner1.ref)); + expect(owner2.ref.debugReferrers, hasLength(2)); + expect(owner2.ref.debugReferrers.first, owner1); + expect(owner2.ref.debugReferrers.last, owner2); + expect(owner2.ref.refCount, 2); + expect(owner2.ref.nativeObject, nativeObject); + expect(TestSkDeletableMock.deleteCount, 0); + expect( + reason: 'Second owner does not add more native object owners. ' + 'The underlying shared UniqueRef is the only one.', + mockFinalizationRegistry.registeredPairs, hasLength(1), + ); + + owner1.dispose(); + expect(owner2.ref.debugReferrers, hasLength(1)); + expect(owner2.ref.debugReferrers.single, owner2); + expect(owner2.ref.refCount, 1); + expect(owner2.ref.nativeObject, nativeObject); + expect(TestSkDeletableMock.deleteCount, 0); + expect( + reason: 'The same owner cannot dispose its CountedRef more than once, even when CountedRef is still alive.', + () => owner1.dispose(), throwsA(isA()), + ); + + owner2.dispose(); + expect(owner2.ref.debugReferrers, isEmpty); + expect(owner2.ref.refCount, 0); + expect( + reason: 'Cannot access object that was disposed', + () => owner2.ref.nativeObject, throwsA(isA()), + ); + expect(TestSkDeletableMock.deleteCount, 1); + + expect( + reason: 'The same owner cannot dispose its CountedRef more than once.', + () => owner2.dispose(), throwsA(isA()), + ); + + // Simulate a GC + mockFinalizationRegistry.registeredPairs.single.ref.collect(); + expect( + reason: 'Manually disposed object should not be deleted again by GC.', + TestSkDeletableMock.deleteCount, 1, + ); + }); + }); +} + +class TestSkDeletableMock { + static int deleteCount = 0; + + bool isDeleted() => _isDeleted; + bool _isDeleted = false; + + void delete() { + expect(_isDeleted, isFalse, + reason: + 'CanvasKit does not allow deleting the same object more than once.'); + _isDeleted = true; + deleteCount++; + } + + JsConstructor get constructor => TestJsConstructor(name: 'TestSkDeletable'); +} + +@JS() +@anonymous +@staticInterop +class TestSkDeletable implements SkDeletable { + factory TestSkDeletable() { + final TestSkDeletableMock mock = TestSkDeletableMock(); + return TestSkDeletable._( + isDeleted: () { return mock.isDeleted(); }.toJS, + delete: () { return mock.delete(); }.toJS, + constructor: mock.constructor); + } + + external factory TestSkDeletable._({ + JSFunction isDeleted, + JSFunction delete, + JsConstructor constructor}); +} + +@JS() +@anonymous +@staticInterop +class TestJsConstructor implements JsConstructor { + external factory TestJsConstructor({String name}); +} + +class TestCountedRefOwner implements StackTraceDebugger { + TestCountedRefOwner(TestSkDeletable nativeObject) { + if (assertionsEnabled) { + _debugStackTrace = StackTrace.current; + } + ref = CountedRef( + nativeObject, this, 'TestCountedRefOwner'); + } + + TestCountedRefOwner.cloneOf(this.ref) { + if (assertionsEnabled) { + _debugStackTrace = StackTrace.current; + } + ref.ref(this); + } + + @override + StackTrace get debugStackTrace => _debugStackTrace; + late StackTrace _debugStackTrace; + + late final CountedRef ref; + + void dispose() { + ref.unref(this); + } + + TestCountedRefOwner clone() => TestCountedRefOwner.cloneOf(ref); +} + +class _MockNativeMemoryFinalizationRegistry implements NativeMemoryFinalizationRegistry { + final List<_MockPair> registeredPairs = <_MockPair>[]; + + @override + void register(Object owner, UniqueRef ref) { + registeredPairs.add(_MockPair(owner, ref)); + } +} + +class _MockPair { + _MockPair(this.owner, this.ref); + + Object owner; + UniqueRef ref; +} diff --git a/lib/web_ui/test/canvaskit/skia_objects_cache_test.dart b/lib/web_ui/test/canvaskit/skia_objects_cache_test.dart index 9e7def11ca9ba..14df75d7e709c 100644 --- a/lib/web_ui/test/canvaskit/skia_objects_cache_test.dart +++ b/lib/web_ui/test/canvaskit/skia_objects_cache_test.dart @@ -12,8 +12,6 @@ import 'package:test/test.dart'; import 'package:ui/src/engine.dart'; import 'package:ui/ui.dart'; -import '../common/matchers.dart'; -import '../common/spy.dart'; import 'common.dart'; void main() { @@ -113,242 +111,6 @@ void _tests() { expect(SkiaObjects.expensiveCache.debugContains(object2), isFalse); }); }); - - group(SkiaObjectBox, () { - test('Records stack traces and respects refcounts', () async { - final ZoneSpy spy = ZoneSpy(); - spy.run(() { - Instrumentation.enabled = true; - TestSkDeletableMock.deleteCount = 0; - TestBoxWrapper.resurrectCount = 0; - final TestBoxWrapper original = TestBoxWrapper(); - - expect(original.box.debugGetStackTraces().length, 1); - expect(original.box.refCount, 1); - expect(original.box.isDeletedPermanently, isFalse); - - final TestBoxWrapper clone = original.clone(); - expect(clone.box, same(original.box)); - expect(clone.box.debugGetStackTraces().length, 2); - expect(clone.box.refCount, 2); - expect(original.box.debugGetStackTraces().length, 2); - expect(original.box.refCount, 2); - expect(original.box.isDeletedPermanently, isFalse); - - original.dispose(); - - testCollector.collectNow(); - expect(TestSkDeletableMock.deleteCount, 0); - - spy.fakeAsync.elapse(const Duration(seconds: 2)); - expect( - spy.printLog, - [ - 'Engine counters:\n TestSkDeletable created: 1\n' - ], - ); - - expect(clone.box.debugGetStackTraces().length, 1); - expect(clone.box.refCount, 1); - expect(original.box.debugGetStackTraces().length, 1); - expect(original.box.refCount, 1); - - clone.dispose(); - expect(clone.box.debugGetStackTraces().length, 0); - expect(clone.box.refCount, 0); - expect(original.box.debugGetStackTraces().length, 0); - expect(original.box.refCount, 0); - expect(original.box.isDeletedPermanently, isTrue); - - testCollector.collectNow(); - expect(TestSkDeletableMock.deleteCount, 1); - expect(TestBoxWrapper.resurrectCount, 0); - - expect(() => clone.box.unref(clone), throwsAssertionError); - spy.printLog.clear(); - spy.fakeAsync.elapse(const Duration(seconds: 2)); - expect( - spy.printLog, - [ - 'Engine counters:\n TestSkDeletable created: 1\n TestSkDeletable deleted: 1\n' - ], - ); - Instrumentation.enabled = false; - }); - }); - - test('Can resurrect Skia objects', () async { - TestSkDeletableMock.deleteCount = 0; - TestBoxWrapper.resurrectCount = 0; - final TestBoxWrapper object = TestBoxWrapper(); - expect(TestSkDeletableMock.deleteCount, 0); - expect(TestBoxWrapper.resurrectCount, 0); - - // Test 3 cycles of delete/resurrect. - for (int i = 0; i < 3; i++) { - object.box.delete(); - object.box.didDelete(); - expect(TestSkDeletableMock.deleteCount, i + 1); - expect(TestBoxWrapper.resurrectCount, i); - expect(object.box.isDeletedTemporarily, isTrue); - expect(object.box.isDeletedPermanently, isFalse); - - expect(object.box.skiaObject, isNotNull); - expect(TestSkDeletableMock.deleteCount, i + 1); - expect(TestBoxWrapper.resurrectCount, i + 1); - expect(object.box.isDeletedTemporarily, isFalse); - expect(object.box.isDeletedPermanently, isFalse); - } - - object.dispose(); - expect(object.box.isDeletedPermanently, isTrue); - }); - - test('Can dispose temporarily deleted object', () async { - TestSkDeletableMock.deleteCount = 0; - TestBoxWrapper.resurrectCount = 0; - final TestBoxWrapper object = TestBoxWrapper(); - expect(TestSkDeletableMock.deleteCount, 0); - expect(TestBoxWrapper.resurrectCount, 0); - - object.box.delete(); - object.box.didDelete(); - expect(TestSkDeletableMock.deleteCount, 1); - expect(TestBoxWrapper.resurrectCount, 0); - expect(object.box.isDeletedTemporarily, isTrue); - expect(object.box.isDeletedPermanently, isFalse); - - object.dispose(); - expect(object.box.isDeletedPermanently, isTrue); - }); - }); - - group('$SynchronousSkiaObjectCache', () { - test('is initialized empty', () { - expect(SynchronousSkiaObjectCache(10), hasLength(0)); - }); - - test('adds objects', () { - final SynchronousSkiaObjectCache cache = SynchronousSkiaObjectCache(2); - cache.add(TestSelfManagedObject()); - expect(cache, hasLength(1)); - cache.add(TestSelfManagedObject()); - expect(cache, hasLength(2)); - }); - - test('forbids adding the same object twice', () { - final SynchronousSkiaObjectCache cache = SynchronousSkiaObjectCache(2); - final TestSelfManagedObject object = TestSelfManagedObject(); - cache.add(object); - expect(cache, hasLength(1)); - expect(() => cache.add(object), throwsAssertionError); - }); - - void expectObjectInCache( - SynchronousSkiaObjectCache cache, - TestSelfManagedObject object, - ) { - expect(cache.debugContains(object), isTrue); - expect(object._skiaObject, isNotNull); - } - - void expectObjectNotInCache( - SynchronousSkiaObjectCache cache, - TestSelfManagedObject object, - ) { - expect(cache.debugContains(object), isFalse); - expect(object._skiaObject, isNull); - } - - test('respects maximumSize', () { - final SynchronousSkiaObjectCache cache = SynchronousSkiaObjectCache(2); - final TestSelfManagedObject object1 = TestSelfManagedObject(); - final TestSelfManagedObject object2 = TestSelfManagedObject(); - final TestSelfManagedObject object3 = TestSelfManagedObject(); - final TestSelfManagedObject object4 = TestSelfManagedObject(); - - cache.add(object1); - expect(cache, hasLength(1)); - expectObjectInCache(cache, object1); - - cache.add(object2); - expect(cache, hasLength(2)); - expectObjectInCache(cache, object1); - expectObjectInCache(cache, object2); - - cache.add(object3); - expect(cache, hasLength(2)); - expectObjectNotInCache(cache, object1); - expectObjectInCache(cache, object2); - expectObjectInCache(cache, object3); - - cache.add(object4); - expect(cache, hasLength(2)); - expectObjectNotInCache(cache, object1); - expectObjectNotInCache(cache, object2); - expectObjectInCache(cache, object3); - expectObjectInCache(cache, object4); - }); - - test('uses RLU strategy', () { - final SynchronousSkiaObjectCache cache = SynchronousSkiaObjectCache(2); - final TestSelfManagedObject object1 = TestSelfManagedObject(); - final TestSelfManagedObject object2 = TestSelfManagedObject(); - final TestSelfManagedObject object3 = TestSelfManagedObject(); - final TestSelfManagedObject object4 = TestSelfManagedObject(); - - cache.add(object1); - expectObjectInCache(cache, object1); - cache.add(object2); - expectObjectInCache(cache, object2); - cache.add(object3); - expectObjectInCache(cache, object3); - expectObjectNotInCache(cache, object1); - - cache.markUsed(object2); - cache.add(object4); - expectObjectInCache(cache, object2); - expectObjectNotInCache(cache, object3); - expectObjectInCache(cache, object4); - }); - }); -} - -/// A simple class that wraps a [SkiaObjectBox]. -/// -/// Can be [clone]d such that the clones share the same ref counted box. -class TestBoxWrapper implements StackTraceDebugger { - TestBoxWrapper() { - if (assertionsEnabled) { - _debugStackTrace = StackTrace.current; - } - box = SkiaObjectBox.resurrectable( - this, TestSkDeletable(), () { - resurrectCount += 1; - return TestSkDeletable(); - }); - } - - TestBoxWrapper.cloneOf(this.box) { - if (assertionsEnabled) { - _debugStackTrace = StackTrace.current; - } - box.ref(this); - } - - static int resurrectCount = 0; - - @override - StackTrace get debugStackTrace => _debugStackTrace; - late StackTrace _debugStackTrace; - - late SkiaObjectBox box; - - void dispose() { - box.unref(this); - } - - TestBoxWrapper clone() => TestBoxWrapper.cloneOf(box); } class TestSkDeletableMock { From da3318d04c8473fdd4b93faae7c52d9ef74fa3b4 Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Mon, 3 Apr 2023 13:10:53 -0700 Subject: [PATCH 2/2] replace factory with top-level function --- .../lib/src/engine/canvaskit/canvaskit_api.dart | 16 ++++++++-------- .../lib/src/engine/canvaskit/native_memory.dart | 2 +- .../test/canvaskit/canvaskit_api_test.dart | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/web_ui/lib/src/engine/canvaskit/canvaskit_api.dart b/lib/web_ui/lib/src/engine/canvaskit/canvaskit_api.dart index 2d6b8fde36689..39491cc275c4a 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/canvaskit_api.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/canvaskit_api.dart @@ -3392,7 +3392,7 @@ abstract class Collector { class ProductionCollector implements Collector { ProductionCollector() { _skObjectFinalizationRegistry = - SkObjectFinalizationRegistry((SkDeletable deletable) { + createSkObjectFinalizationRegistry((SkDeletable deletable) { // This is called when GC decides to collect the wrapper object and // notify us, which may happen after the object is already deleted // explicitly, e.g. when its ref count drops to zero. When that happens @@ -3568,13 +3568,13 @@ extension JsConstructorExtension on JsConstructor { /// 6. We call `delete` on SkPaint. @JS('window.FinalizationRegistry') @staticInterop -class SkObjectFinalizationRegistry { - factory SkObjectFinalizationRegistry(JSFunction cleanup) { - return js_util.callConstructor( - _finalizationRegistryConstructor!.toObjectShallow, - [cleanup], - ); - } +class SkObjectFinalizationRegistry {} + +SkObjectFinalizationRegistry createSkObjectFinalizationRegistry(JSFunction cleanup) { + return js_util.callConstructor( + _finalizationRegistryConstructor!.toObjectShallow, + [cleanup], + ); } extension SkObjectFinalizationRegistryExtension on SkObjectFinalizationRegistry { diff --git a/lib/web_ui/lib/src/engine/canvaskit/native_memory.dart b/lib/web_ui/lib/src/engine/canvaskit/native_memory.dart index 45b051c5058e1..9d9ed733698ca 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/native_memory.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/native_memory.dart @@ -11,7 +11,7 @@ import 'canvaskit_api.dart'; /// Collects native objects that weren't explicitly disposed of using /// [UniqueRef.dispose] or [CountedRef.unref]. -SkObjectFinalizationRegistry _finalizationRegistry = SkObjectFinalizationRegistry( +SkObjectFinalizationRegistry _finalizationRegistry = createSkObjectFinalizationRegistry( (UniqueRef uniq) { uniq.collect(); }.toJS diff --git a/lib/web_ui/test/canvaskit/canvaskit_api_test.dart b/lib/web_ui/test/canvaskit/canvaskit_api_test.dart index 16260d41317bf..d2140d9f94618 100644 --- a/lib/web_ui/test/canvaskit/canvaskit_api_test.dart +++ b/lib/web_ui/test/canvaskit/canvaskit_api_test.dart @@ -1900,7 +1900,7 @@ void _paragraphTests() { // FinalizationRegistry because it depends on GC, which cannot be controlled, // So the test simply tests that a FinalizationRegistry can be constructed // and its `register` method can be called. - final SkObjectFinalizationRegistry registry = SkObjectFinalizationRegistry((String arg) {}.toJS); + final SkObjectFinalizationRegistry registry = createSkObjectFinalizationRegistry((String arg) {}.toJS); registry.register(Object(), Object()); }); }