From 3726d1ec323538b63f25131248eeb20d033511ec Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Thu, 13 Jan 2022 18:40:02 -0800 Subject: [PATCH] Ensure that PlatformViewIOS does not call into Shell semantics APIs during destruction (#30835) --- .../Source/accessibility_bridge_test.mm | 47 +++++++++++++++++++ shell/platform/darwin/ios/platform_view_ios.h | 20 ++++---- .../platform/darwin/ios/platform_view_ios.mm | 38 +++++++-------- 3 files changed, 73 insertions(+), 32 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm index 1e281e250e251..b8e6ca5fdb654 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm @@ -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; + }; + + TestDelegate test_delegate; + auto thread = std::make_unique("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( + /*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(); + OCMStub([mockFlutterViewController platformViewsController]) + .andReturn(flutterPlatformViewsController.get()); + auto weakFactory = + std::make_unique>(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 diff --git a/shell/platform/darwin/ios/platform_view_ios.h b/shell/platform/darwin/ios/platform_view_ios.h index 3af69b7b2bf8a..aef27035a5940 100644 --- a/shell/platform/darwin/ios/platform_view_ios.h +++ b/shell/platform/darwin/ios/platform_view_ios.h @@ -111,20 +111,20 @@ class PlatformViewIOS final : public PlatformView { id 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& set_semantics_enabled); - AccessibilityBridgePtr(const std::function& set_semantics_enabled, - AccessibilityBridge* bridge); - ~AccessibilityBridgePtr(); + explicit AccessibilityBridgeManager(const std::function& set_semantics_enabled); + AccessibilityBridgeManager(const std::function& set_semantics_enabled, + AccessibilityBridge* bridge); explicit operator bool() const noexcept { return static_cast(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 bridge); + void Clear(); private: - FML_DISALLOW_COPY_AND_ASSIGN(AccessibilityBridgePtr); + FML_DISALLOW_COPY_AND_ASSIGN(AccessibilityBridgeManager); std::unique_ptr accessibility_bridge_; std::function set_semantics_enabled_; }; @@ -137,7 +137,7 @@ class PlatformViewIOS final : public PlatformView { std::shared_ptr ios_context_; const std::shared_ptr& platform_views_controller_; PlatformMessageRouter platform_message_router_; - AccessibilityBridgePtr accessibility_bridge_; + AccessibilityBridgeManager accessibility_bridge_; fml::scoped_nsprotocol text_input_plugin_; fml::closure firstFrameCallback_; ScopedObserver dealloc_view_controller_observer_; diff --git a/shell/platform/darwin/ios/platform_view_ios.mm b/shell/platform/darwin/ios/platform_view_ios.mm index 081bbe0db130a..ad47dac00279d 100644 --- a/shell/platform/darwin/ios/platform_view_ios.mm +++ b/shell/platform/darwin/ios/platform_view_ios.mm @@ -16,11 +16,11 @@ namespace flutter { -PlatformViewIOS::AccessibilityBridgePtr::AccessibilityBridgePtr( +PlatformViewIOS::AccessibilityBridgeManager::AccessibilityBridgeManager( const std::function& set_semantics_enabled) - : AccessibilityBridgePtr(set_semantics_enabled, nullptr) {} + : AccessibilityBridgeManager(set_semantics_enabled, nullptr) {} -PlatformViewIOS::AccessibilityBridgePtr::AccessibilityBridgePtr( +PlatformViewIOS::AccessibilityBridgeManager::AccessibilityBridgeManager( const std::function& set_semantics_enabled, AccessibilityBridge* bridge) : accessibility_bridge_(bridge), set_semantics_enabled_(set_semantics_enabled) { @@ -29,20 +29,14 @@ } } -PlatformViewIOS::AccessibilityBridgePtr::~AccessibilityBridgePtr() { - if (accessibility_bridge_) { - set_semantics_enabled_(false); - } +void PlatformViewIOS::AccessibilityBridgeManager::Set(std::unique_ptr 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( @@ -86,7 +80,7 @@ if (ios_surface_ || !owner_controller) { NotifyDestroyed(); ios_surface_.reset(); - accessibility_bridge_.reset(); + accessibility_bridge_.Clear(); } owner_controller_ = owner_controller; @@ -98,7 +92,7 @@ queue:[NSOperationQueue mainQueue] usingBlock:^(NSNotification* note) { // Implicit copy of 'this' is fine. - accessibility_bridge_.reset(); + accessibility_bridge_.Clear(); owner_controller_.reset(); }] retain]); @@ -122,7 +116,7 @@ FML_DCHECK(ios_surface_ != nullptr); if (accessibility_bridge_) { - accessibility_bridge_.reset(new AccessibilityBridge( + accessibility_bridge_.Set(std::make_unique( owner_controller_.get(), this, [owner_controller_.get() platformViewsController])); } } @@ -169,10 +163,10 @@ return; } if (enabled && !accessibility_bridge_) { - accessibility_bridge_.reset(new AccessibilityBridge( + accessibility_bridge_.Set(std::make_unique( owner_controller_.get(), this, [owner_controller_.get() platformViewsController])); } else if (!enabled && accessibility_bridge_) { - accessibility_bridge_.reset(); + accessibility_bridge_.Clear(); } else { PlatformView::SetSemanticsEnabled(enabled); } @@ -188,7 +182,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()]; } @@ -201,7 +195,7 @@ void PlatformViewIOS::OnPreEngineRestart() const { if (accessibility_bridge_) { - accessibility_bridge_->clearState(); + accessibility_bridge_.get()->clearState(); } if (!owner_controller_) { return;