Clarify the relationship between PictureRecorder and Canvas #19393
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
There was a problem hiding this comment.
I may be mistaken but the way this reads to me is "the canvas references a recorder and a recorder references a canvas with the association broken when the caller ends recording". So, if a canvas and recorder are created by the caller but end recording is not called before both are unreachable, won't they both never be collected because their native peers still hold references to them?
lib/ui/painting/picture_recorder.cc
Outdated
| @@ -41,7 +39,7 @@ PictureRecorder::PictureRecorder() {} | |||
| PictureRecorder::~PictureRecorder() {} | |||
|
|
|||
| bool PictureRecorder::isRecording() { | |||
| return canvas_ && canvas_->IsRecording(); | |||
| return bool(canvas_); | |||
There was a problem hiding this comment.
static_cast instead of C style casts please.
There was a problem hiding this comment.
FWIW, this is a C++ bool constructor invocation. Removed this.
lib/ui/painting/picture_recorder.cc
Outdated
| @@ -41,7 +39,7 @@ PictureRecorder::PictureRecorder() {} | |||
| PictureRecorder::~PictureRecorder() {} | |||
|
|
|||
| bool PictureRecorder::isRecording() { | |||
There was a problem hiding this comment.
This is now unused right (Dart won't call this anymore)? Can we just remove this method?
There was a problem hiding this comment.
This was being used by an assert in the Canvas constructor, but that check is redundant with checks done in Dart. Removed this.
| @@ -432,12 +432,11 @@ void Canvas::drawShadow(const CanvasPath* path, | |||
| elevation, transparentOccluder, dpr); | |||
| } | |||
|
|
|||
| void Canvas::Clear() { | |||
| void Canvas::Invalidate() { | |||
| if (dart_wrapper()) { | |||
There was a problem hiding this comment.
Who else invalidates the canvas? The only owner would be a picture recorder which gets its own bespoke canvas. In case the caller tries to end recording multiple time, the isRecording check on the wrapper (PictureRecorder) would trip. Not sure when the Dart wrapper would ever be unset here. Seems like a redundant check.
There was a problem hiding this comment.
This is defensive coding intended to avoid the scenario described in flutter/flutter#60319 (comment) where a Dart API invocation causes the Canvas to be GCed.
If the PictureRecorder Dart object is still holding its reference to the Canvas when the endRecording native method is called, then it would not be possible for the Canvas to be GCed inside the native method. But I wanted to add the check so that Invalidate will still be safe if the order of operations in the Dart code changes.
|
The native peers hold weak references to the Dart objects, which will not prevent the Even if |
jason-simmons
force-pushed
the
bug_60319
branch
4 times, most recently
from
f893567 to
60f59dc
Compare
A Canvas is only valid as long as the PictureRecorder used to construct the Canvas is alive. Previously Dart was unaware of this relationship, and it was possible for a PictureRecorder to be GCed before the Canvas. * Canvas will hold a reference to its PictureRecorder in order to prevent the PictureRecorder from being GCed until endRecording is called or the Canvas itself is GCed. * PictureRecorder.endRecording will disconnect the Canvas native object from its Dart peer. Attempts to use the Canvas after endRecoding will result in an "Object has been disposed" exception thrown by Tonic. See flutter/flutter#60319 See flutter/flutter#51066
jason-simmons
added
the
waiting for tree to go green
A Canvas is only valid as long as the PictureRecorder used to construct
the Canvas is alive. Previously Dart was unaware of this relationship,
and it was possible for a PictureRecorder to be GCed before the Canvas.
prevent the PictureRecorder from being GCed until endRecording
is called or the Canvas itself is GCed.
from its Dart peer. Attempts to use the Canvas after endRecoding
will result in an "Object has been disposed" exception thrown by
Tonic.
See flutter/flutter#60319
See flutter/flutter#51066