Avoid dereferencing IO manager weak pointers on the UI thread #13232
Conversation
gw280
left a comment
gw280
left a comment
There was a problem hiding this comment.
LGTM apart from some questions. Needs a unit test, though.
| } | ||
|
|
||
| fml::WeakPtr<IOManager> UIDartState::GetIOManager() const { | ||
| return io_manager_; |
There was a problem hiding this comment.
Should we be vending a new WeakPtr here by calling io_manager_->GetWeakIOManager()?
There was a problem hiding this comment.
The UI thread can safely copy io_manager_, but it can't safely dereference and call io_manager_->GetWeakIOManager
lib/ui/painting/image_encoding.cc
Outdated
| image = canvas_image->image(), // | ||
| context = std::move(context), // | ||
| fml::MakeCopyable([callback = std::move(callback), // | ||
| image = canvas_image->image(), // |
There was a problem hiding this comment.
Removed the empty comments
lib/ui/painting/image_encoding.cc
Outdated
| std::move(image), // | ||
| context.get(), // | ||
| EncodeImageAndInvokeDataCallback(std::move(callback), // | ||
| std::move(image), // |
There was a problem hiding this comment.
Removed the empty comments
| fml::WeakPtr<IOManager> GetIOManager() const; | ||
|
|
||
| fml::WeakPtr<GrContext> GetResourceContext() const; | ||
| fml::RefPtr<flutter::SkiaUnrefQueue> GetSkiaUnrefQueue() const; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
jason-simmons
force-pushed
the
resource_ctx_weak_ptr
branch
from
9f06820 to
0e7857c
Compare
UI thread APIs that need to access the IO thread's resource context should obtain a weak pointer to the IO manager and pass that to the IO thread.
jason-simmons
force-pushed
the
resource_ctx_weak_ptr
branch
from
0e7857c to
13aa052
Compare
|
Not sure how to test this effectively with our current infrastructure. This issue involves Dart APIs invoked on the UI thread using an object that is owned by the IO thread. Ideally a Dart test like https://github.com/flutter/engine/blob/master/testing/dart/canvas_test.dart should catch this. But the flutter_tester environment that runs the Dart tests uses the same thread for platform/UI/IO/GPU. So the assertions can't differentiate beween the UI thread and the IO thread. |
gw280
left a comment
gw280
left a comment
There was a problem hiding this comment.
LGTM with the caveat that we file an issue for this unit test and GetSkiaUnrefQueue() change. I can start looking at the unit test now.
| image_format // | ||
| ); | ||
|
|
||
| task_runners.GetIOTaskRunner()->PostTask(fml::MakeCopyable( |
…flutter/engine#13232) (#43045) git@github.com:flutter/engine.git/compare/8aefcd857508...8882bf3 git log 8aefcd8..8882bf3 --no-merges --oneline 2019-10-18 jason-simmons@users.noreply.github.com Avoid dereferencing IO manager weak pointers on the UI thread (flutter/engine#13232) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC franciscojma@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
…flutter/engine#13232) (flutter#43045) git@github.com:flutter/engine.git/compare/8aefcd857508...8882bf3 git log 8aefcd8..8882bf3 --no-merges --oneline 2019-10-18 jason-simmons@users.noreply.github.com Avoid dereferencing IO manager weak pointers on the UI thread (flutter/engine#13232) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC franciscojma@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
5 participants
UI thread APIs that need to access the IO thread's resource context should
obtain a weak pointer to the IO manager and pass that to the IO thread.