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

Avoid dereferencing IO manager weak pointers on the UI thread #13232

Merged
merged 1 commit into from
Oct 18, 2019
Merged

Avoid dereferencing IO manager weak pointers on the UI thread #13232

Show file tree
Hide file tree
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
25 changes: 10 additions & 15 deletions lib/ui/painting/image_encoding.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,21 +211,16 @@ Dart_Handle EncodeImage(CanvasImage* canvas_image,
tonic::DartState::Current(), callback_handle);

const auto& task_runners = UIDartState::Current()->GetTaskRunners();
auto context = UIDartState::Current()->GetResourceContext();

task_runners.GetIOTaskRunner()->PostTask(
fml::MakeCopyable([callback = std::move(callback), //
image = canvas_image->image(), //
context = std::move(context), //
ui_task_runner = task_runners.GetUITaskRunner(), //
image_format //
]() mutable {
EncodeImageAndInvokeDataCallback(std::move(callback), //
std::move(image), //
context.get(), //
std::move(ui_task_runner), //
image_format //
);

task_runners.GetIOTaskRunner()->PostTask(fml::MakeCopyable(
Copy link
Member

@chinmaygarde chinmaygarde Oct 18, 2019

Choose a reason for hiding this comment

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

Noooo. The slashes.

[callback = std::move(callback), image = canvas_image->image(),
io_manager = UIDartState::Current()->GetIOManager(),
ui_task_runner = task_runners.GetUITaskRunner(),
image_format]() mutable {
EncodeImageAndInvokeDataCallback(std::move(callback), std::move(image),
io_manager->GetResourceContext().get(),
std::move(ui_task_runner),
image_format);
}));

return Dart_Null();
Expand Down
10 changes: 5 additions & 5 deletions lib/ui/painting/multi_frame_codec.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,11 +168,11 @@ Dart_Handle MultiFrameCodec::getNextFrame(Dart_Handle callback_handle) {
[callback = std::make_unique<DartPersistentValue>(
tonic::DartState::Current(), callback_handle),
this, trace_id, ui_task_runner = task_runners.GetUITaskRunner(),
queue = UIDartState::Current()->GetSkiaUnrefQueue(),
context = dart_state->GetResourceContext()]() mutable {
GetNextFrameAndInvokeCallback(std::move(callback),
std::move(ui_task_runner), context,
std::move(queue), trace_id);
io_manager = dart_state->GetIOManager()]() mutable {
GetNextFrameAndInvokeCallback(
std::move(callback), std::move(ui_task_runner),
io_manager->GetResourceContext(), io_manager->GetSkiaUnrefQueue(),
trace_id);
}));

return Dart_Null();
Expand Down
6 changes: 3 additions & 3 deletions lib/ui/painting/picture.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ Dart_Handle Picture::RasterizeToImage(sk_sp<SkPicture> picture,
auto unref_queue = dart_state->GetSkiaUnrefQueue();
auto ui_task_runner = dart_state->GetTaskRunners().GetUITaskRunner();
auto io_task_runner = dart_state->GetTaskRunners().GetIOTaskRunner();
fml::WeakPtr<GrContext> resource_context = dart_state->GetResourceContext();
fml::WeakPtr<IOManager> io_manager = dart_state->GetIOManager();

// We can't create an image on this task runner because we don't have a
// graphics context. Even if we did, it would be slow anyway. Also, this
Expand Down Expand Up @@ -173,9 +173,9 @@ Dart_Handle Picture::RasterizeToImage(sk_sp<SkPicture> picture,

fml::TaskRunner::RunNowOrPostTask(io_task_runner, [ui_task_runner, picture,
picture_bounds, ui_task,
resource_context] {
io_manager] {
sk_sp<SkSurface> surface =
MakeSnapshotSurface(picture_bounds, resource_context);
MakeSnapshotSurface(picture_bounds, io_manager->GetResourceContext());
sk_sp<SkImage> raster_image = MakeRasterSnapshot(picture, surface);

fml::TaskRunner::RunNowOrPostTask(
Expand Down
12 changes: 4 additions & 8 deletions lib/ui/ui_dart_state.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ const TaskRunners& UIDartState::GetTaskRunners() const {
return task_runners_;
}

fml::WeakPtr<IOManager> UIDartState::GetIOManager() const {
return io_manager_;
Copy link
Contributor

@gw280 gw280 Oct 18, 2019

Choose a reason for hiding this comment

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

Should we be vending a new WeakPtr here by calling io_manager_->GetWeakIOManager()?

Copy link
Member Author

@jason-simmons jason-simmons Oct 18, 2019

Choose a reason for hiding this comment

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

The UI thread can safely copy io_manager_, but it can't safely dereference and call io_manager_->GetWeakIOManager

}

fml::RefPtr<flutter::SkiaUnrefQueue> UIDartState::GetSkiaUnrefQueue() const {
// TODO(gw280): The WeakPtr here asserts that we are derefing it on the
// same thread as it was created on. As we can't guarantee that currently
Expand Down Expand Up @@ -119,14 +123,6 @@ void UIDartState::AddOrRemoveTaskObserver(bool add) {
}
}

fml::WeakPtr<GrContext> UIDartState::GetResourceContext() const {
FML_DCHECK(task_runners_.GetIOTaskRunner()->RunsTasksOnCurrentThread());
if (!io_manager_) {
return {};
}
return io_manager_->GetResourceContext();
}

fml::WeakPtr<ImageDecoder> UIDartState::GetImageDecoder() const {
return image_decoder_;
}
Expand Down
4 changes: 2 additions & 2 deletions lib/ui/ui_dart_state.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,9 @@ class UIDartState : public tonic::DartState {

void FlushMicrotasksNow();

fml::RefPtr<flutter::SkiaUnrefQueue> GetSkiaUnrefQueue() const;
fml::WeakPtr<IOManager> GetIOManager() const;

fml::WeakPtr<GrContext> GetResourceContext() const;
fml::RefPtr<flutter::SkiaUnrefQueue> GetSkiaUnrefQueue() const;
Copy link
Contributor

@gw280 gw280 Oct 18, 2019

Choose a reason for hiding this comment

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

Should we also delete this method and expect callees to go through the IOManager as well? It seems like that should be the direction we go down - but I'm happy to defer this to another issue (and file one to track that) instead of overloading this PR.

Copy link
Member Author

@jason-simmons jason-simmons Oct 18, 2019

Choose a reason for hiding this comment

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

Ideally we should never dereference io_manager_ on the UI thread.

Fixing this for SkiaUnrefQueue will require a different approach because it is used widely in UI thread APIs that do not run tasks on the IO thread. One option would be keeping a strong RefPtr to the SkiaUnrefQueue in UIDartState.


fml::WeakPtr<ImageDecoder> GetImageDecoder() const;

Expand Down