Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Commit cc241fa

Browse files
authored
[web] fix SkFinalizationRegistry for dart2js (attempt 4) (#40938)
(this is attempt 4; details below) Remove obsolete object caches and introduce a simpler way to manage native objects: * Remove the unused `SynchronousSkiaObjectCache`. * Introduce new library `native_memory.dart` that's smaller and simpler than `skia_object_cache.dart`. * Introduce two types of native object references: * `UniqueRef` a reference with a unique Dart object owner. * `CountedRef` a ref-counted reference with multiple Dart object owners. * All native references use GC (via `FinalizationRegistry`) as a back-up. * The new library removes everything related to object resurrection that was needed only in browsers that didn't support `FinalizationRegistry`. All browsers support it now. * Remove the ad hoc `SkParagraph` cache that predates the introduction of `Paragraph.dispose`. * Rewrite `CkParagraph` in terms of `UniqueRef`. * Rewrite `CkImage` in terms of `CountedRef`; delete `SkiaObjectBox`. This PR does not migrate all objects from the old `skia_object_cache.dart` to `native_memory.dart`. That would be too big of a change. The migration can be done in multiple smaller PRs. This also removes a few unnecessary relayouts observed in flutter/flutter#120921, but not all of them (more details in flutter/flutter#120921 (comment)) ## About attempt 4 More info about the revert of attempt 3 in #40937. In this attempt I check that the browser supports `FinalizationRegistry` before registering the object. This will allow the code to run in older browsers, but there will be no protection from memory leaks when the app fails to dispose of the respective objects. ## Benchmarks Now that this landed in flutter/flutter I have some benchmark numbers from the devicelab. The `text_out_of_picture_bounds` benchmark dropped by 3-4x (lower is better): <img width="358" alt="Screenshot 2023-04-04 at 6 13 06 PM" src="https://user-images.githubusercontent.com/211513/229956170-a5399ed3-c779-4af0-babb-ea40440f96ff.png"> The repro provided in flutter/flutter#123204 dropped from 110ms/frame to 10ms/frame.
1 parent a0f955f commit cc241fa

13 files changed

Lines changed: 561 additions & 772 deletions

ci/licenses_golden/licenses_flutter

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1876,6 +1876,7 @@ ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/layer_scene_builder
18761876
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/layer_tree.dart + ../../../flutter/LICENSE
18771877
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/mask_filter.dart + ../../../flutter/LICENSE
18781878
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/n_way_canvas.dart + ../../../flutter/LICENSE
1879+
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/native_memory.dart + ../../../flutter/LICENSE
18791880
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/noto_font.dart + ../../../flutter/LICENSE
18801881
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/painting.dart + ../../../flutter/LICENSE
18811882
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/path.dart + ../../../flutter/LICENSE
@@ -4457,6 +4458,7 @@ FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/layer_scene_builder.d
44574458
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/layer_tree.dart
44584459
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/mask_filter.dart
44594460
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/n_way_canvas.dart
4461+
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/native_memory.dart
44604462
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/noto_font.dart
44614463
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/painting.dart
44624464
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/path.dart

lib/web_ui/lib/src/engine.dart

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ export 'engine/canvaskit/layer_scene_builder.dart';
3939
export 'engine/canvaskit/layer_tree.dart';
4040
export 'engine/canvaskit/mask_filter.dart';
4141
export 'engine/canvaskit/n_way_canvas.dart';
42+
export 'engine/canvaskit/native_memory.dart';
4243
export 'engine/canvaskit/noto_font.dart';
4344
export 'engine/canvaskit/painting.dart';
4445
export 'engine/canvaskit/path.dart';

lib/web_ui/lib/src/engine/canvaskit/canvas.dart

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,6 @@ class CkCanvas {
215215
offset.dx,
216216
offset.dy,
217217
);
218-
paragraph.markUsed();
219218
}
220219

221220
void drawPath(CkPath path, CkPaint paint) {
@@ -1112,7 +1111,6 @@ class CkDrawParagraphCommand extends CkPaintCommand {
11121111
offset.dx,
11131112
offset.dy,
11141113
);
1115-
paragraph.markUsed();
11161114
}
11171115
}
11181116

lib/web_ui/lib/src/engine/canvaskit/canvaskit_api.dart

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3392,7 +3392,7 @@ abstract class Collector {
33923392
class ProductionCollector implements Collector {
33933393
ProductionCollector() {
33943394
_skObjectFinalizationRegistry =
3395-
SkObjectFinalizationRegistry((SkDeletable deletable) {
3395+
createSkObjectFinalizationRegistry((SkDeletable deletable) {
33963396
// This is called when GC decides to collect the wrapper object and
33973397
// notify us, which may happen after the object is already deleted
33983398
// explicitly, e.g. when its ref count drops to zero. When that happens
@@ -3568,10 +3568,13 @@ extension JsConstructorExtension on JsConstructor {
35683568
/// 6. We call `delete` on SkPaint.
35693569
@JS('window.FinalizationRegistry')
35703570
@staticInterop
3571-
class SkObjectFinalizationRegistry {
3572-
// TODO(hterkelsen): Add a type for the `cleanup` function when
3573-
// native constructors support type parameters.
3574-
external factory SkObjectFinalizationRegistry(JSFunction cleanup);
3571+
class SkObjectFinalizationRegistry {}
3572+
3573+
SkObjectFinalizationRegistry createSkObjectFinalizationRegistry(JSFunction cleanup) {
3574+
return js_util.callConstructor(
3575+
_finalizationRegistryConstructor!.toObjectShallow,
3576+
<Object>[cleanup],
3577+
);
35753578
}
35763579

35773580
extension SkObjectFinalizationRegistryExtension on SkObjectFinalizationRegistry {

lib/web_ui/lib/src/engine/canvaskit/image.dart

Lines changed: 6 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,10 @@ import 'canvas.dart';
1515
import 'canvaskit_api.dart';
1616
import 'image_wasm_codecs.dart';
1717
import 'image_web_codecs.dart';
18+
import 'native_memory.dart';
1819
import 'painting.dart';
1920
import 'picture.dart';
2021
import 'picture_recorder.dart';
21-
import 'skia_object_cache.dart';
2222

2323
/// Instantiates a [ui.Codec] backed by an `SkAnimatedImage` from Skia.
2424
FutureOr<ui.Codec> skiaInstantiateImageCodec(Uint8List list,
@@ -227,52 +227,8 @@ Future<Uint8List> readChunked(HttpFetchPayload payload, int contentLength, WebOn
227227
/// A [ui.Image] backed by an `SkImage` from Skia.
228228
class CkImage implements ui.Image, StackTraceDebugger {
229229
CkImage(SkImage skImage, { this.videoFrame }) {
230+
box = CountedRef<CkImage, SkImage>(skImage, this, 'SkImage');
230231
_init();
231-
if (browserSupportsFinalizationRegistry) {
232-
box = SkiaObjectBox<CkImage, SkImage>(this, skImage);
233-
} else {
234-
// If finalizers are not supported we need to be able to resurrect the
235-
// image if it was temporarily deleted. To do that, we keep the original
236-
// pixels and ask the SkiaObjectBox to make an image from them when
237-
// resurrecting.
238-
//
239-
// IMPORTANT: the alphaType, colorType, and colorSpace passed to
240-
// _encodeImage and to canvasKit.MakeImage must be the same. Otherwise
241-
// Skia will misinterpret the pixels and corrupt the image.
242-
final ByteData? originalBytes = _encodeImage(
243-
skImage: skImage,
244-
format: ui.ImageByteFormat.rawRgba,
245-
alphaType: canvasKit.AlphaType.Premul,
246-
colorType: canvasKit.ColorType.RGBA_8888,
247-
colorSpace: SkColorSpaceSRGB,
248-
);
249-
if (originalBytes == null) {
250-
printWarning('Unable to encode image to bytes. We will not '
251-
'be able to resurrect it once it has been garbage collected.');
252-
return;
253-
}
254-
final int originalWidth = skImage.width().toInt();
255-
final int originalHeight = skImage.height().toInt();
256-
box = SkiaObjectBox<CkImage, SkImage>.resurrectable(this, skImage, () {
257-
final SkImage? skImage = canvasKit.MakeImage(
258-
SkImageInfo(
259-
alphaType: canvasKit.AlphaType.Premul,
260-
colorType: canvasKit.ColorType.RGBA_8888,
261-
colorSpace: SkColorSpaceSRGB,
262-
width: originalWidth.toDouble(),
263-
height: originalHeight.toDouble(),
264-
),
265-
originalBytes.buffer.asUint8List(),
266-
(4 * originalWidth).toDouble(),
267-
);
268-
if (skImage == null) {
269-
throw ImageCodecException(
270-
'Failed to resurrect image from pixels.'
271-
);
272-
}
273-
return skImage;
274-
});
275-
}
276232
}
277233

278234
CkImage.cloneOf(this.box, {this.videoFrame}) {
@@ -291,9 +247,9 @@ class CkImage implements ui.Image, StackTraceDebugger {
291247
StackTrace get debugStackTrace => _debugStackTrace;
292248
late StackTrace _debugStackTrace;
293249

294-
// Use a box because `SkImage` may be deleted either due to this object
250+
// Use ref counting because `SkImage` may be deleted either due to this object
295251
// being garbage-collected, or by an explicit call to [delete].
296-
late final SkiaObjectBox<CkImage, SkImage> box;
252+
late final CountedRef<CkImage, SkImage> box;
297253

298254
/// For browsers that support `ImageDecoder` this field holds the video frame
299255
/// from which this image was created.
@@ -305,9 +261,9 @@ class CkImage implements ui.Image, StackTraceDebugger {
305261

306262
/// The underlying Skia image object.
307263
///
308-
/// Do not store the returned value. It is memory-managed by [SkiaObjectBox].
264+
/// Do not store the returned value. It is memory-managed by [CountedRef].
309265
/// Storing it may result in use-after-free bugs.
310-
SkImage get skImage => box.skiaObject;
266+
SkImage get skImage => box.nativeObject;
311267

312268
bool _disposed = false;
313269

Lines changed: 218 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,218 @@
1+
// Copyright 2013 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
import 'dart:js_interop';
6+
import 'package:meta/meta.dart';
7+
8+
import '../../engine.dart' show Instrumentation;
9+
import '../util.dart';
10+
import 'canvaskit_api.dart';
11+
12+
/// Collects native objects that weren't explicitly disposed of using
13+
/// [UniqueRef.dispose] or [CountedRef.unref].
14+
SkObjectFinalizationRegistry _finalizationRegistry = createSkObjectFinalizationRegistry(
15+
(UniqueRef<Object> uniq) {
16+
uniq.collect();
17+
}.toJS
18+
);
19+
20+
NativeMemoryFinalizationRegistry nativeMemoryFinalizationRegistry = NativeMemoryFinalizationRegistry();
21+
22+
/// An indirection to [SkObjectFinalizationRegistry] to enable tests provide a
23+
/// mock implementation of a finalization registry.
24+
class NativeMemoryFinalizationRegistry {
25+
void register(Object owner, UniqueRef<Object> ref) {
26+
if (browserSupportsFinalizationRegistry) {
27+
_finalizationRegistry.register(owner, ref);
28+
}
29+
}
30+
}
31+
32+
/// Manages the lifecycle of a C++ object referenced by a single Dart object.
33+
///
34+
/// It is expected that when the C++ object is no longer needed [dispose] is
35+
/// called.
36+
///
37+
/// To prevent memory leaks, the underlying C++ object is deleted by the GC if
38+
/// it wasn't previously disposed of explicitly.
39+
class UniqueRef<T extends Object> {
40+
UniqueRef(Object owner, T nativeObject, this._debugOwnerLabel) {
41+
_nativeObject = nativeObject;
42+
if (Instrumentation.enabled) {
43+
Instrumentation.instance.incrementCounter('$_debugOwnerLabel Created');
44+
}
45+
nativeMemoryFinalizationRegistry.register(owner, this);
46+
}
47+
48+
T? _nativeObject;
49+
final String _debugOwnerLabel;
50+
51+
/// Returns the underlying native object reference, if it has not been
52+
/// disposed of yet.
53+
///
54+
/// The returned reference must not be stored. I should only be borrowed
55+
/// temporarily. Storing this reference may result in dangling pointer errors.
56+
T get nativeObject {
57+
assert(!isDisposed, 'Native object was disposed.');
58+
return _nativeObject!;
59+
}
60+
61+
/// Returns whether the underlying native object has been disposed and
62+
/// therefore can no longer be used.
63+
bool get isDisposed => _nativeObject == null;
64+
65+
/// Disposes the underlying native object.
66+
///
67+
/// The underlying object may be deleted or its ref count may be bumped down.
68+
/// The exact action taken depends on the sharing model of that particular
69+
/// object. For example, an [SkImage] may not be immediately deleted if a
70+
/// [SkPicture] exists that still references it. On the other hand, [SkPaint]
71+
/// is deleted eagerly.
72+
void dispose() {
73+
assert(!isDisposed, 'A native object reference cannot be disposed more than once.');
74+
if (Instrumentation.enabled) {
75+
Instrumentation.instance.incrementCounter('$_debugOwnerLabel Deleted');
76+
}
77+
final SkDeletable object = nativeObject as SkDeletable;
78+
if (!object.isDeleted()) {
79+
object.delete();
80+
}
81+
_nativeObject = null;
82+
}
83+
84+
/// Called by the garbage [Collector] when the owner of this handle is
85+
/// collected.
86+
///
87+
/// Garbage collection is used as a back-up for the cases when the handle
88+
/// isn't disposed of explicitly by calling [dispose]. It most likely
89+
/// indicates a memory leak or inefficiency in the framework or application
90+
/// code.
91+
@visibleForTesting
92+
void collect() {
93+
if (!isDisposed) {
94+
if (Instrumentation.enabled) {
95+
Instrumentation.instance.incrementCounter('$_debugOwnerLabel Leaked');
96+
}
97+
dispose();
98+
}
99+
}
100+
}
101+
102+
/// Interface that classes wrapping [UniqueRef] must implement.
103+
///
104+
/// Used to collect stack traces in debug mode.
105+
abstract class StackTraceDebugger {
106+
/// The stack trace pointing to code location that created or upreffed a
107+
/// [CountedRef].
108+
StackTrace get debugStackTrace;
109+
}
110+
111+
/// Manages the lifecycle of a C++ object referenced by multiple Dart objects.
112+
///
113+
/// Uses reference counting to manage the lifecycle of the C++ object.
114+
///
115+
/// If the C++ object has a unique owner, use [UniqueRef] instead.
116+
///
117+
/// The [ref] method can be used to increment the refcount to tell this box to
118+
/// keep the underlying C++ object alive.
119+
///
120+
/// The [unref] method can be used to decrement the refcount indicating that a
121+
/// referring object no longer needs it. When the refcount drops to zero the
122+
/// underlying C++ object is deleted.
123+
///
124+
/// In addition to ref counting, this object is also managed by GC. When this
125+
/// reference is garbage collected, the underlying C++ object is automatically
126+
/// deleted. This is mostly done to prevent memory leaks in production. Well
127+
/// behaving framework and app code are expected to rely on [ref] and [unref]
128+
/// for timely collection of resources.
129+
class CountedRef<R extends StackTraceDebugger, T extends Object> {
130+
/// Creates a counted reference.
131+
CountedRef(T nativeObject, R debugReferrer, String debugLabel) {
132+
_ref = UniqueRef<T>(this, nativeObject, debugLabel);
133+
if (assertionsEnabled) {
134+
debugReferrers.add(debugReferrer);
135+
}
136+
assert(refCount == debugReferrers.length);
137+
}
138+
139+
/// The native object reference whose lifecycle is being managed by this ref
140+
/// count.
141+
///
142+
/// Do not store this value outside this class.
143+
late final UniqueRef<T> _ref;
144+
145+
/// Returns the underlying native object reference, if it has not been
146+
/// disposed of yet.
147+
///
148+
/// The returned reference must not be stored. I should only be borrowed
149+
/// temporarily. Storing this reference may result in dangling pointer errors.
150+
T get nativeObject => _ref.nativeObject;
151+
152+
/// The number of objects sharing references to this box.
153+
///
154+
/// When this count reaches zero, the underlying [nativeObject] is scheduled
155+
/// for deletion.
156+
int get refCount => _refCount;
157+
int _refCount = 1;
158+
159+
/// Whether the underlying [nativeObject] has been disposed and is no longer
160+
/// accessible.
161+
bool get isDisposed => _ref.isDisposed;
162+
163+
/// When assertions are enabled, stores all objects that share this box.
164+
///
165+
/// The length of this list is always identical to [refCount].
166+
///
167+
/// This list can be used for debugging ref counting issues.
168+
final Set<R> debugReferrers = <R>{};
169+
170+
/// If asserts are enabled, the [StackTrace]s representing when a reference
171+
/// was created.
172+
List<StackTrace> debugGetStackTraces() {
173+
if (assertionsEnabled) {
174+
return debugReferrers
175+
.map<StackTrace>((R referrer) => referrer.debugStackTrace)
176+
.toList();
177+
}
178+
throw UnsupportedError('');
179+
}
180+
181+
/// Increases the reference count of this box because a new object began
182+
/// sharing ownership of the underlying [nativeObject].
183+
void ref(R debugReferrer) {
184+
assert(
185+
!_ref.isDisposed,
186+
'Cannot increment ref count on a deleted handle.',
187+
);
188+
assert(_refCount > 0);
189+
assert(
190+
debugReferrers.add(debugReferrer),
191+
'Attempted to increment ref count by the same referrer more than once.',
192+
);
193+
_refCount += 1;
194+
assert(refCount == debugReferrers.length);
195+
}
196+
197+
/// Decrements the reference count for the [nativeObject].
198+
///
199+
/// Does nothing if the object has already been deleted.
200+
///
201+
/// If this causes the reference count to drop to zero, deletes the
202+
/// [nativeObject].
203+
void unref(R debugReferrer) {
204+
assert(
205+
!_ref.isDisposed,
206+
'Attempted to unref an already deleted native object.',
207+
);
208+
assert(
209+
debugReferrers.remove(debugReferrer),
210+
'Attempted to decrement ref count by the same referrer more than once.',
211+
);
212+
_refCount -= 1;
213+
assert(refCount == debugReferrers.length);
214+
if (_refCount == 0) {
215+
_ref.dispose();
216+
}
217+
}
218+
}

lib/web_ui/lib/src/engine/canvaskit/picture.dart

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,7 @@ class CkPicture extends ManagedSkiaObject<SkPicture> implements ui.Picture {
4646
/// false.
4747
///
4848
/// This extra flag is necessary on top of [rawSkiaObject] because
49-
/// [rawSkiaObject] being null does not indicate permanent deletion. See
50-
/// similar flag [SkiaObjectBox.isDeletedPermanently].
49+
/// [rawSkiaObject] being null does not indicate permanent deletion.
5150
bool _isDisposed = false;
5251

5352
/// The stack trace taken when [dispose] was called.

0 commit comments

Comments
 (0)