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

Ensure that Picture::RasterizeToImage destroys Dart persistent values on the UI thread #8182

Merged
jason-simmons merged 1 commit into from
Mar 28, 2019
Merged

Ensure that Picture::RasterizeToImage destroys Dart persistent values on the UI thread #8182

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
81 changes: 36 additions & 45 deletions lib/ui/painting/picture.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ Dart_Handle Picture::RasterizeToImage(sk_sp<SkPicture> picture,
}

auto* dart_state = UIDartState::Current();
auto image_callback = std::make_unique<tonic::DartPersistentValue>(
dart_state, raw_image_callback);
tonic::DartPersistentValue* image_callback =
new tonic::DartPersistentValue(dart_state, raw_image_callback);
auto unref_queue = dart_state->GetSkiaUnrefQueue();
auto ui_task_runner = dart_state->GetTaskRunners().GetUITaskRunner();
auto gpu_task_runner = dart_state->GetTaskRunners().GetGPUTaskRunner();
Expand All @@ -84,52 +84,43 @@ Dart_Handle Picture::RasterizeToImage(sk_sp<SkPicture> picture,

auto picture_bounds = SkISize::Make(width, height);

auto ui_task = fml::MakeCopyable(
[ui_task_runner, image_callback = std::move(image_callback),
unref_queue](sk_sp<SkImage> raster_image) mutable {
// Send the raster image back to the UI thread for submission to the
// framework.
fml::TaskRunner::RunNowOrPostTask(
ui_task_runner,
fml::MakeCopyable([raster_image,
image_callback = std::move(image_callback),
unref_queue]() mutable {
auto dart_state = image_callback->dart_state().lock();
if (!dart_state) {
// The root isolate could have died in the meantime.
return;
}
tonic::DartState::Scope scope(dart_state);

if (!raster_image) {
tonic::DartInvoke(image_callback->Get(), {Dart_Null()});
return;
}

auto dart_image = CanvasImage::Create();
dart_image->set_image(
{std::move(raster_image), std::move(unref_queue)});
auto* raw_dart_image = tonic::ToDart(std::move(dart_image));

// All done!
tonic::DartInvoke(image_callback->Get(), {raw_dart_image});
Copy link
Contributor

@liyuqian liyuqian Mar 15, 2019

Choose a reason for hiding this comment

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

I'd expect that the std::unique_ptr to be destructed here after that image_callback is no longer used, which means destruction in the ui thread. Is it not the case? @chinmaygarde

Copy link
Member Author

@jason-simmons jason-simmons Mar 15, 2019

Choose a reason for hiding this comment

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

In the old implementation, the std::unique_ptr was being captured into a ref counted lambda by MakeCopyable.

The MakeCopyable lambda was then being passed through the GPU thread to the UI thread. Either the GPU thread or the UI thread could drop the last reference to the lambda. If the GPU thread destroyed the lambda, then the captured image_callback would be destructed on the GPU thread, causing a failure.

}));
});

auto gpu_task = fml::MakeCopyable([gpu_task_runner, picture, picture_bounds,
snapshot_delegate, ui_task]() {
fml::TaskRunner::RunNowOrPostTask(gpu_task_runner, [snapshot_delegate,
picture, picture_bounds,
ui_task]() {
// Snapshot the picture on the GPU thread. This thread has access to the
// GPU contexts that may contain the sole references to texture backed
// images in the picture.
ui_task(snapshot_delegate->MakeRasterSnapshot(picture, picture_bounds));
});
auto ui_task = fml::MakeCopyable([image_callback, unref_queue](
sk_sp<SkImage> raster_image) mutable {
auto dart_state = image_callback->dart_state().lock();
if (!dart_state) {
// The root isolate could have died in the meantime.
return;
}
tonic::DartState::Scope scope(dart_state);

if (!raster_image) {
tonic::DartInvoke(image_callback->Get(), {Dart_Null()});
return;
}

auto dart_image = CanvasImage::Create();
dart_image->set_image({std::move(raster_image), std::move(unref_queue)});
auto* raw_dart_image = tonic::ToDart(std::move(dart_image));

// All done!
tonic::DartInvoke(image_callback->Get(), {raw_dart_image});

// image_callback is associated with the Dart isolate and must be deleted
// on the UI thread
delete image_callback;
});

// Kick things off on the GPU.
gpu_task();
fml::TaskRunner::RunNowOrPostTask(
gpu_task_runner,
[ui_task_runner, snapshot_delegate, picture, picture_bounds, ui_task] {
sk_sp<SkImage> raster_image =
snapshot_delegate->MakeRasterSnapshot(picture, picture_bounds);

fml::TaskRunner::RunNowOrPostTask(
ui_task_runner,
[ui_task, raster_image]() { ui_task(raster_image); });
});

return Dart_Null();
}
Expand Down