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

Ensure that PlatformViewIOS does not call into Shell semantics APIs during destruction #30835

Merged
fluttergithubbot merged 1 commit into from
Jan 14, 2022
+73 −32
Merged

Ensure that PlatformViewIOS does not call into Shell semantics APIs during destruction #30835

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
47 changes: 47 additions & 0 deletions shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1664,4 +1664,51 @@ - (void)testFlutterSemanticsScrollViewManagedObjectLifecycleCorrectly {
// flutterSemanticsScrollView) will cause an EXC_BAD_ACCESS.
XCTAssertFalse([flutterSemanticsScrollView isAccessibilityElement]);
}

- (void)testPlatformViewDestructorDoesNotCallSemanticsAPIs {
class TestDelegate : public flutter::MockDelegate {
public:
void OnPlatformViewSetSemanticsEnabled(bool enabled) override { set_semantics_enabled_calls++; }
int set_semantics_enabled_calls = 0;
Copy link
Member

@gaaclarke gaaclarke Jan 13, 2022

Choose a reason for hiding this comment

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

nit: google c++ style says this should have a postfix _ (only structs have fields without them).

I was thinking that you'd show it called through once so the final assert would be 1, not 2, not a huge deal.

};

TestDelegate test_delegate;
auto thread = std::make_unique<fml::Thread>("AccessibilityBridgeTest");
auto thread_task_runner = thread->GetTaskRunner();
flutter::TaskRunners runners(/*label=*/self.name.UTF8String,
/*platform=*/thread_task_runner,
/*raster=*/thread_task_runner,
/*ui=*/thread_task_runner,
/*io=*/thread_task_runner);

fml::AutoResetWaitableEvent latch;
thread_task_runner->PostTask([&] {
auto platform_view = std::make_unique<flutter::PlatformViewIOS>(
/*delegate=*/test_delegate,
/*rendering_api=*/flutter::IOSRenderingAPI::kSoftware,
/*platform_views_controller=*/nil,
/*task_runners=*/runners);

id mockFlutterViewController = OCMClassMock([FlutterViewController class]);
auto flutterPlatformViewsController =
std::make_shared<flutter::FlutterPlatformViewsController>();
OCMStub([mockFlutterViewController platformViewsController])
.andReturn(flutterPlatformViewsController.get());
auto weakFactory =
std::make_unique<fml::WeakPtrFactory<FlutterViewController>>(mockFlutterViewController);
platform_view->SetOwnerViewController(weakFactory->GetWeakPtr());

platform_view->SetSemanticsEnabled(true);
XCTAssertNotEqual(test_delegate.set_semantics_enabled_calls, 0);

// Deleting PlatformViewIOS should not call OnPlatformViewSetSemanticsEnabled
test_delegate.set_semantics_enabled_calls = 0;
platform_view.reset();
XCTAssertEqual(test_delegate.set_semantics_enabled_calls, 0);

latch.Signal();
});
latch.Wait();
}

@end
20 changes: 10 additions & 10 deletions 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 @@ -113,20 +113,20 @@ class PlatformViewIOS final : public PlatformView {
id<NSObject> observer_;
};

/// Smart pointer that guarantees we communicate clearing Accessibility
/// Wrapper that guarantees we communicate clearing Accessibility
/// information to Dart.
class AccessibilityBridgePtr {
class AccessibilityBridgeManager {
public:
explicit AccessibilityBridgePtr(const std::function<void(bool)>& set_semantics_enabled);
AccessibilityBridgePtr(const std::function<void(bool)>& set_semantics_enabled,
AccessibilityBridge* bridge);
~AccessibilityBridgePtr();
explicit AccessibilityBridgeManager(const std::function<void(bool)>& set_semantics_enabled);
AccessibilityBridgeManager(const std::function<void(bool)>& set_semantics_enabled,
AccessibilityBridge* bridge);
explicit operator bool() const noexcept { return static_cast<bool>(accessibility_bridge_); }
AccessibilityBridge* operator->() const noexcept { return accessibility_bridge_.get(); }
void reset(AccessibilityBridge* bridge = nullptr);
AccessibilityBridge* get() const noexcept { return accessibility_bridge_.get(); }
void Set(std::unique_ptr<AccessibilityBridge> bridge);
void Clear();

private:
FML_DISALLOW_COPY_AND_ASSIGN(AccessibilityBridgePtr);
FML_DISALLOW_COPY_AND_ASSIGN(AccessibilityBridgeManager);
std::unique_ptr<AccessibilityBridge> accessibility_bridge_;
std::function<void(bool)> set_semantics_enabled_;
};
Expand All @@ -138,7 +138,7 @@ class PlatformViewIOS final : public PlatformView {
std::unique_ptr<IOSSurface> ios_surface_;
std::shared_ptr<IOSContext> ios_context_;
const std::shared_ptr<FlutterPlatformViewsController>& platform_views_controller_;
AccessibilityBridgePtr accessibility_bridge_;
AccessibilityBridgeManager accessibility_bridge_;
fml::scoped_nsprotocol<FlutterTextInputPlugin*> text_input_plugin_;
fml::closure firstFrameCallback_;
ScopedObserver dealloc_view_controller_observer_;
Expand Down
38 changes: 16 additions & 22 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 @@ -16,11 +16,11 @@

namespace flutter {

PlatformViewIOS::AccessibilityBridgePtr::AccessibilityBridgePtr(
PlatformViewIOS::AccessibilityBridgeManager::AccessibilityBridgeManager(
const std::function<void(bool)>& set_semantics_enabled)
: AccessibilityBridgePtr(set_semantics_enabled, nullptr) {}
: AccessibilityBridgeManager(set_semantics_enabled, nullptr) {}

PlatformViewIOS::AccessibilityBridgePtr::AccessibilityBridgePtr(
PlatformViewIOS::AccessibilityBridgeManager::AccessibilityBridgeManager(
const std::function<void(bool)>& set_semantics_enabled,
AccessibilityBridge* bridge)
: accessibility_bridge_(bridge), set_semantics_enabled_(set_semantics_enabled) {
Expand All @@ -29,20 +29,14 @@
}
}

PlatformViewIOS::AccessibilityBridgePtr::~AccessibilityBridgePtr() {
if (accessibility_bridge_) {
set_semantics_enabled_(false);
}
void PlatformViewIOS::AccessibilityBridgeManager::Set(std::unique_ptr<AccessibilityBridge> bridge) {
accessibility_bridge_ = std::move(bridge);
set_semantics_enabled_(true);
}

void PlatformViewIOS::AccessibilityBridgePtr::reset(AccessibilityBridge* bridge) {
if (accessibility_bridge_) {
set_semantics_enabled_(false);
}
accessibility_bridge_.reset(bridge);
if (accessibility_bridge_) {
set_semantics_enabled_(true);
}
void PlatformViewIOS::AccessibilityBridgeManager::Clear() {
set_semantics_enabled_(false);
accessibility_bridge_.reset();
}

PlatformViewIOS::PlatformViewIOS(
Expand Down Expand Up @@ -83,7 +77,7 @@
if (ios_surface_ || !owner_controller) {
NotifyDestroyed();
ios_surface_.reset();
accessibility_bridge_.reset();
accessibility_bridge_.Clear();
}
owner_controller_ = owner_controller;

Expand All @@ -95,7 +89,7 @@
queue:[NSOperationQueue mainQueue]
usingBlock:^(NSNotification* note) {
// Implicit copy of 'this' is fine.
accessibility_bridge_.reset();
accessibility_bridge_.Clear();
owner_controller_.reset();
}] retain]);

Expand All @@ -119,7 +113,7 @@
FML_DCHECK(ios_surface_ != nullptr);

if (accessibility_bridge_) {
accessibility_bridge_.reset(new AccessibilityBridge(
accessibility_bridge_.Set(std::make_unique<AccessibilityBridge>(
owner_controller_.get(), this, [owner_controller_.get() platformViewsController]));
}
}
Expand Down Expand Up @@ -166,10 +160,10 @@
return;
}
if (enabled && !accessibility_bridge_) {
accessibility_bridge_.reset(new AccessibilityBridge(
accessibility_bridge_.Set(std::make_unique<AccessibilityBridge>(
owner_controller_.get(), this, [owner_controller_.get() platformViewsController]));
} else if (!enabled && accessibility_bridge_) {
accessibility_bridge_.reset();
accessibility_bridge_.Clear();
} else {
PlatformView::SetSemanticsEnabled(enabled);
}
Expand All @@ -185,7 +179,7 @@
flutter::CustomAccessibilityActionUpdates actions) {
FML_DCHECK(owner_controller_);
if (accessibility_bridge_) {
accessibility_bridge_->UpdateSemantics(std::move(update), std::move(actions));
accessibility_bridge_.get()->UpdateSemantics(std::move(update), std::move(actions));
[[NSNotificationCenter defaultCenter] postNotificationName:FlutterSemanticsUpdateNotification
object:owner_controller_.get()];
}
Expand All @@ -198,7 +192,7 @@

void PlatformViewIOS::OnPreEngineRestart() const {
if (accessibility_bridge_) {
accessibility_bridge_->clearState();
accessibility_bridge_.get()->clearState();
}
if (!owner_controller_) {
return;
Expand Down