-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Description
The code in //third_party/tonic/typed_data/typed_list.h is dangerous as written.
The underlying C API it wraps says that you cannot do anything that would cause an allocation to occur before releasing the data, but for users of e.g. a Float32List object, that's not at all clear. In fact, it's not always clear what other methods in the engine may go down some branch that will end up causing a Dart allocation to happen. It is also difficult because tonic wrapped C++ methods make the object look like something scoped to your method, and give no indication that you would ever need to release it early. I and other engine developers have made this mistake (see flutter/engine#31745 and related issues).
In general, the usage of typed data needs to follow an Acquire - Copy - Release pattern. Ideally, Tonic would just give the method the copied result (but we have to evaluate whether this can work for Skia/DL APIs that we pass the data into and end up doing copying behind the scenes).
For example, instead of a method taking a const tonic::Float64List& matrix4 parameter, it would just take a SkM44 parameter. Instead of taking a const tonic::Uint8List& data, it would take whatever structure that data gets copied into.
The main goal here is to make it harder for contributors to make mistakes on this, particularly contributors who are not familiar with the underlying Dart API that is getting wrapped and assume this is a RAII object like any other.
/cc @flar for thoughts on whether/how this works with display list code.