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

Added some thread asserts to the code and made ios_surface_ safe since #12775

Merged
gaaclarke merged 2 commits into from
Oct 3, 2019
Merged

Added some thread asserts to the code and made ios_surface_ safe since #12775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb51f57
Added some thread asserts to the code and made ios_surface_ safe since
gaaclarke Oct 3, 2019
a3d4103
responded to chinmays feedback, added comment
gaaclarke Oct 3, 2019
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
5 changes: 4 additions & 1 deletion shell/platform/darwin/ios/platform_view_ios.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,10 @@ class PlatformViewIOS final : public PlatformView {

private:
fml::WeakPtr<FlutterViewController> owner_controller_;
std::unique_ptr<IOSSurface> ios_surface_;
// Since the `ios_surface_` is created on the platform thread but
// used on the GPU thread we need to protect it with a mutex.
std::mutex ios_surface_mutex_;
Copy link
Contributor

@chinmaygarde chinmaygarde Oct 3, 2019

Choose a reason for hiding this comment

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

You can annotate ios_surface_ with FML_GUARDED_BY so that static thread safety check can be enabled. Also, can you add a comment there saying which threads access that resource and when. Since there wont be any contention, its hard to reason about why that lock is necessary.

Copy link
Member Author

@gaaclarke gaaclarke Oct 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Done.

I'm not so sure usage of ios_surface_ is coordinated externally, calling SetOwnerViewController rapidly could cause CreateRenderingSurface to be talking to a undefined ios_surface_ without this mutex, for example.

Copy link
Contributor

@chinmaygarde chinmaygarde Oct 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Thats good to document too. But your call.

std::unique_ptr<IOSSurface> ios_surface_ FML_GUARDED_BY(ios_surface_mutex_);
std::shared_ptr<IOSGLContext> gl_context_;
PlatformMessageRouter platform_message_router_;
std::unique_ptr<AccessibilityBridge> accessibility_bridge_;
Expand Down
5 changes: 5 additions & 0 deletions shell/platform/darwin/ios/platform_view_ios.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@
}

void PlatformViewIOS::SetOwnerViewController(fml::WeakPtr<FlutterViewController> owner_controller) {
FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread());
std::lock_guard<std::mutex> guard(ios_surface_mutex_);
if (ios_surface_ || !owner_controller) {
NotifyDestroyed();
ios_surface_.reset();
Expand Down Expand Up @@ -92,6 +94,8 @@ new AccessibilityBridge(static_cast<FlutterView*>(owner_controller_.get().view),

// |PlatformView|
std::unique_ptr<Surface> PlatformViewIOS::CreateRenderingSurface() {
FML_DCHECK(task_runners_.GetGPUTaskRunner()->RunsTasksOnCurrentThread());
std::lock_guard<std::mutex> guard(ios_surface_mutex_);
if (!ios_surface_) {
FML_DLOG(INFO) << "Could not CreateRenderingSurface, this PlatformViewIOS "
"has no ViewController.";
Expand All @@ -102,6 +106,7 @@ new AccessibilityBridge(static_cast<FlutterView*>(owner_controller_.get().view),

// |PlatformView|
sk_sp<GrContext> PlatformViewIOS::CreateResourceContext() const {
FML_DCHECK(task_runners_.GetIOTaskRunner()->RunsTasksOnCurrentThread());
if (!gl_context_ || !gl_context_->ResourceMakeCurrent()) {
FML_DLOG(INFO) << "Could not make resource context current on IO thread. "
"Async texture uploads will be disabled. On Simulators, "
Expand Down