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

Fix invalid usage of typed data detected via dart_debug=true #31745

Merged
fluttergithubbot merged 3 commits into from
Mar 2, 2022
Merged

Fix invalid usage of typed data detected via dart_debug=true #31745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
273036f
Fix invalid usage of typed data detected via dart_debug=true
dnfield Mar 1, 2022
6061594
Fix order
dnfield Mar 1, 2022
a902910
Revert changes to gn
dnfield Mar 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/ui/painting/fragment_program.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ void FragmentProgram::init(std::string sksl, bool debugPrintSksl) {

fml::RefPtr<FragmentShader> FragmentProgram::shader(
Dart_Handle shader,
const tonic::Float32List& uniforms,
tonic::Float32List& uniforms,
Dart_Handle samplers) {
auto sampler_shaders =
tonic::DartConverter<std::vector<ImageShader*>>::FromDart(samplers);
Expand All @@ -69,6 +69,7 @@ fml::RefPtr<FragmentShader> FragmentProgram::shader(
for (size_t i = 0; i < uniform_count; i++) {
uniform_floats[i] = uniforms[i];
}
uniforms.Release();
Copy link
Member

@zanderso zanderso Mar 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we always be doing an explicit Release() on typed data types when they're passed like this? If so, maybe instead of a reference type, they could be passed in such a way that their destructor complains if there wasn't an explicit Release() somewhere?

As it stands with this PR, we're relying on debug unopt unit tests having good coverage.

Copy link
Contributor Author

@dnfield dnfield Mar 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to do an explicit release before we call any other Dart API. But if we don't call any Dart API, it's fine to not explicitly release the data.

IMHO, the tonic typed data class(es) are poorly designed, mainly because the API they are wrapping is difficult to use correctly. One thing we could try to do is avoid having automatic conversion of these in tonic, since misuse tends to happen where it's unclear that you have to free the passed in argument before you do any other dart_api calls that may allocate anything.

Copy link
Contributor Author

@dnfield dnfield Mar 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, put differently: we could refactor the tonic API to not automatically acquire the typed data, and instead just have some way of copying the data to the desired structure since that's what we typically do with this (e.g. SkMatrix, SkData, etc.).

Copy link
Contributor Author

@dnfield dnfield Mar 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done a couple experiments and filed flutter/flutter#99431. I think it's worth refactoring this further but it should probably be done incrementally and is a bigger task than I can take on at the moment.

std::vector<sk_sp<SkShader>> sk_samplers(sampler_shaders.size());
for (size_t i = 0; i < sampler_shaders.size(); i++) {
ImageShader* image_shader = sampler_shaders[i];
Expand Down
2 changes: 1 addition & 1 deletion lib/ui/painting/fragment_program.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class FragmentProgram : public RefCountedDartWrappable<FragmentProgram> {
void init(std::string sksl, bool debugPrintSksl);

fml::RefPtr<FragmentShader> shader(Dart_Handle shader,
const tonic::Float32List& uniforms,
tonic::Float32List& uniforms,
Dart_Handle samplers);

static void RegisterNatives(tonic::DartLibraryNatives* natives);
Expand Down
11 changes: 7 additions & 4 deletions lib/ui/painting/path.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,16 +253,17 @@ void CanvasPath::addPathWithMatrix(CanvasPath* path,
double dy,
tonic::Float64List& matrix4) {
if (!path) {
matrix4.Release();
Dart_ThrowException(
ToDart("Path.addPathWithMatrix called with non-genuine Path."));
return;
}

SkMatrix matrix = ToSkMatrix(matrix4);
matrix4.Release();
matrix.setTranslateX(matrix.getTranslateX() + dx);
matrix.setTranslateY(matrix.getTranslateY() + dy);
mutable_path().addPath(path->path(), matrix, SkPath::kAppend_AddPathMode);
matrix4.Release();
resetVolatility();
}

Expand All @@ -281,16 +282,17 @@ void CanvasPath::extendWithPathAndMatrix(CanvasPath* path,
double dy,
tonic::Float64List& matrix4) {
if (!path) {
matrix4.Release();
Dart_ThrowException(
ToDart("Path.addPathWithMatrix called with non-genuine Path."));
return;
}

SkMatrix matrix = ToSkMatrix(matrix4);
matrix4.Release();
matrix.setTranslateX(matrix.getTranslateX() + dx);
matrix.setTranslateY(matrix.getTranslateY() + dy);
mutable_path().addPath(path->path(), matrix, SkPath::kExtend_AddPathMode);
matrix4.Release();
resetVolatility();
}

Expand All @@ -317,10 +319,11 @@ void CanvasPath::shift(Dart_Handle path_handle, double dx, double dy) {

void CanvasPath::transform(Dart_Handle path_handle,
tonic::Float64List& matrix4) {
auto sk_matrix = ToSkMatrix(matrix4);
matrix4.Release();
fml::RefPtr<CanvasPath> path = CanvasPath::Create(path_handle);
auto& other_mutable_path = path->mutable_path();
mutable_path().transform(ToSkMatrix(matrix4), &other_mutable_path);
matrix4.Release();
mutable_path().transform(sk_matrix, &other_mutable_path);
}

tonic::Float32List CanvasPath::getBounds() {
Expand Down
14 changes: 10 additions & 4 deletions lib/ui/painting/vertices.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,10 @@ void Vertices::RegisterNatives(tonic::DartLibraryNatives* natives) {

bool Vertices::init(Dart_Handle vertices_handle,
SkVertices::VertexMode vertex_mode,
const tonic::Float32List& positions,
const tonic::Float32List& texture_coordinates,
const tonic::Int32List& colors,
const tonic::Uint16List& indices) {
tonic::Float32List& positions,
tonic::Float32List& texture_coordinates,
tonic::Int32List& colors,
tonic::Uint16List& indices) {
UIDartState::ThrowIfUIOperationsProhibited();
uint32_t builderFlags = 0;
if (texture_coordinates.data()) {
Expand Down Expand Up @@ -76,6 +76,7 @@ bool Vertices::init(Dart_Handle vertices_handle,
FML_DCHECK(positions.num_elements() == texture_coordinates.num_elements());
DecodePoints(texture_coordinates, builder.texCoords());
}

if (colors.data()) {
// SkVertices::Builder assumes equal numbers of elements
FML_DCHECK(positions.num_elements() / 2 == colors.num_elements());
Expand All @@ -87,6 +88,11 @@ bool Vertices::init(Dart_Handle vertices_handle,
builder.indices());
}

positions.Release();
texture_coordinates.Release();
colors.Release();
indices.Release();

auto vertices = fml::MakeRefCounted<Vertices>();
vertices->vertices_ = builder.detach();
vertices->AssociateWithDartWrapper(vertices_handle);
Expand Down
8 changes: 4 additions & 4 deletions lib/ui/painting/vertices.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@ class Vertices : public RefCountedDartWrappable<Vertices> {

static bool init(Dart_Handle vertices_handle,
SkVertices::VertexMode vertex_mode,
const tonic::Float32List& positions,
const tonic::Float32List& texture_coordinates,
const tonic::Int32List& colors,
const tonic::Uint16List& indices);
tonic::Float32List& positions,
tonic::Float32List& texture_coordinates,
tonic::Int32List& colors,
tonic::Uint16List& indices);

const sk_sp<SkVertices>& vertices() const { return vertices_; }

Expand Down