Changed iOS channels to start cleaning up the accessibility handler when the...

Changed iOS channels to start cleaning up the accessibility handler when the bridge is deleted (#19556)

Started cleaning up the accessibility handler when the bridge is
deleted and made nilling out channels safer by making sure they don't
overwrite newly setup handlers.

ci/licenses_golden/licenses_flutter

@@ -934,6 +934,9 @@ FILE: ../../../flutter/shell/platform/darwin/ios/framework/Source/accessibility_
 FILE: ../../../flutter/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm
 FILE: ../../../flutter/shell/platform/darwin/ios/framework/Source/accessibility_text_entry.h
 FILE: ../../../flutter/shell/platform/darwin/ios/framework/Source/accessibility_text_entry.mm
+FILE: ../../../flutter/shell/platform/darwin/ios/framework/Source/connection_collection.cc
+FILE: ../../../flutter/shell/platform/darwin/ios/framework/Source/connection_collection.h
+FILE: ../../../flutter/shell/platform/darwin/ios/framework/Source/connection_collection_test.mm
 FILE: ../../../flutter/shell/platform/darwin/ios/framework/Source/platform_message_response_darwin.h
 FILE: ../../../flutter/shell/platform/darwin/ios/framework/Source/platform_message_response_darwin.mm
 FILE: ../../../flutter/shell/platform/darwin/ios/framework/Source/platform_message_router.h

shell/platform/darwin/common/framework/Headers/FlutterBinaryMessenger.h

@@ -29,6 +29,8 @@ typedef void (^FlutterBinaryReply)(NSData* _Nullable reply);
  */
 typedef void (^FlutterBinaryMessageHandler)(NSData* _Nullable message, FlutterBinaryReply reply);
 
+typedef int64_t FlutterBinaryMessengerConnection;
+
 /**
  * A facility for communicating with the Flutter side using asynchronous message
  * passing with binary messages.
@@ -72,9 +74,20 @@ FLUTTER_EXPORT
  *
  * @param channel The channel name.
  * @param handler The message handler.
+ * @return An identifier that represents the connection that was just created to the channel.
+ */
+- (FlutterBinaryMessengerConnection)setMessageHandlerOnChannel:(NSString*)channel
+                                          binaryMessageHandler:
+                                              (FlutterBinaryMessageHandler _Nullable)handler;
+
+/**
+ * Clears out a channel's message handler if that handler is still the one that
+ * was created as a result of
+ * `setMessageHandlerOnChannel:binaryMessageHandler:`.
+ *
+ * @param connection The result from `setMessageHandlerOnChannel:binaryMessageHandler:`.
  */
-- (void)setMessageHandlerOnChannel:(NSString*)channel
-              binaryMessageHandler:(FlutterBinaryMessageHandler _Nullable)handler;
+- (void)cleanupConnection:(FlutterBinaryMessengerConnection)connection;
 @end
 NS_ASSUME_NONNULL_END
 #endif  // FLUTTER_FLUTTERBINARYMESSENGER_H_

shell/platform/darwin/common/framework/Source/FlutterChannels.mm

@@ -20,6 +20,7 @@ static void ResizeChannelBuffer(NSObject<FlutterBinaryMessenger>* binaryMessenge
   NSObject<FlutterBinaryMessenger>* _messenger;
   NSString* _name;
   NSObject<FlutterMessageCodec>* _codec;
+  FlutterBinaryMessengerConnection _connection;
 }
 + (instancetype)messageChannelWithName:(NSString*)name
                        binaryMessenger:(NSObject<FlutterBinaryMessenger>*)messenger {
@@ -68,7 +69,12 @@ static void ResizeChannelBuffer(NSObject<FlutterBinaryMessenger>* binaryMessenge
 
 - (void)setMessageHandler:(FlutterMessageHandler)handler {
   if (!handler) {
-    [_messenger setMessageHandlerOnChannel:_name binaryMessageHandler:nil];
+    if (_connection > 0) {
+      [_messenger cleanupConnection:_connection];
+      _connection = 0;
+    } else {
+      [_messenger setMessageHandlerOnChannel:_name binaryMessageHandler:nil];
+    }
     return;
   }
   // Grab reference to avoid retain on self.
@@ -78,7 +84,7 @@ static void ResizeChannelBuffer(NSObject<FlutterBinaryMessenger>* binaryMessenge
       callback([codec encode:reply]);
     });
   };
-  [_messenger setMessageHandlerOnChannel:_name binaryMessageHandler:messageHandler];
+  _connection = [_messenger setMessageHandlerOnChannel:_name binaryMessageHandler:messageHandler];
 }
 
 - (void)resizeChannelBuffer:(NSInteger)newSize {
@@ -168,6 +174,7 @@ NSObject const* FlutterMethodNotImplemented = [NSObject new];
   NSObject<FlutterBinaryMessenger>* _messenger;
   NSString* _name;
   NSObject<FlutterMethodCodec>* _codec;
+  FlutterBinaryMessengerConnection _connection;
 }
 
 + (instancetype)methodChannelWithName:(NSString*)name
@@ -222,7 +229,12 @@ NSObject const* FlutterMethodNotImplemented = [NSObject new];
 
 - (void)setMethodCallHandler:(FlutterMethodCallHandler)handler {
   if (!handler) {
-    [_messenger setMessageHandlerOnChannel:_name binaryMessageHandler:nil];
+    if (_connection > 0) {
+      [_messenger cleanupConnection:_connection];
+      _connection = 0;
+    } else {
+      [_messenger setMessageHandlerOnChannel:_name binaryMessageHandler:nil];
+    }
     return;
   }
   // Make sure the block captures the codec, not self.
@@ -238,7 +250,7 @@ NSObject const* FlutterMethodNotImplemented = [NSObject new];
         callback([codec encodeSuccessEnvelope:result]);
     });
   };
-  [_messenger setMessageHandlerOnChannel:_name binaryMessageHandler:messageHandler];
+  _connection = [_messenger setMessageHandlerOnChannel:_name binaryMessageHandler:messageHandler];
 }
 
 - (void)resizeChannelBuffer:(NSInteger)newSize {

shell/platform/darwin/common/framework/Source/FlutterChannelsTest.m

@@ -35,10 +35,16 @@
   self.message = message;
 }
 
-- (void)setMessageHandlerOnChannel:(NSString*)channel
-              binaryMessageHandler:(FlutterBinaryMessageHandler _Nullable)handler {
+- (FlutterBinaryMessengerConnection)setMessageHandlerOnChannel:(NSString*)channel
+                                          binaryMessageHandler:
+                                              (FlutterBinaryMessageHandler _Nullable)handler {
   [self.handlers setObject:handler forKey:channel];
+  return 0;
 }
+
+- (void)cleanupConnection:(FlutterBinaryMessengerConnection)connection {
+}
+
 @end
 
 @interface FlutterChannelsTest : XCTestCase
@@ -155,6 +161,53 @@
   OCMExpect([binaryMessenger sendOnChannel:@"dev.flutter/channel-buffers" message:expectedMessage]);
   [channel resizeChannelBuffer:100];
   OCMVerifyAll(binaryMessenger);
+  [binaryMessenger stopMocking];
+}
+
+- (void)testBasicMessageChannelCleanup {
+  NSString* channelName = @"foo";
+  FlutterBinaryMessengerConnection connection = 123;
+  id binaryMessenger = OCMProtocolMock(@protocol(FlutterBinaryMessenger));
+  id codec = OCMProtocolMock(@protocol(FlutterMethodCodec));
+  FlutterBasicMessageChannel* channel =
+      [[FlutterBasicMessageChannel alloc] initWithName:channelName
+                                       binaryMessenger:binaryMessenger
+                                                 codec:codec];
+  FlutterMessageHandler handler = ^(id _Nullable message, FlutterReply callback) {
+    NSLog(@"hey");
+  };
+  OCMStub([binaryMessenger setMessageHandlerOnChannel:channelName
+                                 binaryMessageHandler:[OCMArg any]])
+      .andReturn(connection);
+  [channel setMessageHandler:handler];
+  OCMVerify([binaryMessenger setMessageHandlerOnChannel:channelName
+                                   binaryMessageHandler:[OCMArg isNotNil]]);
+  [channel setMessageHandler:nil];
+  OCMVerify([binaryMessenger cleanupConnection:connection]);
+}
+
+- (void)testMethodChannelCleanup {
+  NSString* channelName = @"foo";
+  FlutterBinaryMessengerConnection connection = 123;
+  id binaryMessenger = OCMProtocolMock(@protocol(FlutterBinaryMessenger));
+  id codec = OCMProtocolMock(@protocol(FlutterMethodCodec));
+  FlutterMethodChannel* channel = [[FlutterMethodChannel alloc] initWithName:channelName
+                                                             binaryMessenger:binaryMessenger
+                                                                       codec:codec];
+  XCTAssertNotNil(channel);
+
+  OCMStub([binaryMessenger setMessageHandlerOnChannel:channelName
+                                 binaryMessageHandler:[OCMArg any]])
+      .andReturn(connection);
+
+  FlutterMethodCallHandler handler =
+      ^(FlutterMethodCall* _Nonnull call, FlutterResult _Nonnull result) {
+      };
+  [channel setMethodCallHandler:handler];
+  OCMVerify([binaryMessenger setMessageHandlerOnChannel:channelName
+                                   binaryMessageHandler:[OCMArg isNotNil]]);
+  [channel setMethodCallHandler:nil];
+  OCMVerify([binaryMessenger cleanupConnection:connection]);
 }
 
 @end

shell/platform/darwin/ios/BUILD.gn

@@ -74,6 +74,8 @@ source_set("flutter_framework_source") {
     "framework/Source/accessibility_bridge.mm",
     "framework/Source/accessibility_text_entry.h",
     "framework/Source/accessibility_text_entry.mm",
+    "framework/Source/connection_collection.cc",
+    "framework/Source/connection_collection.h",
     "framework/Source/platform_message_response_darwin.h",
     "framework/Source/platform_message_response_darwin.mm",
     "framework/Source/platform_message_router.h",
@@ -208,6 +210,7 @@ shared_library("ios_test_flutter") {
     "framework/Source/FlutterTextInputPluginTest.m",
     "framework/Source/FlutterViewControllerTest.mm",
     "framework/Source/SemanticsObjectTest.mm",
+    "framework/Source/connection_collection_test.mm",
   ]
   deps = [
     ":flutter_framework_source",

shell/platform/darwin/ios/framework/Source/FlutterBinaryMessengerRelay.mm

@@ -35,10 +35,20 @@
   }
 }
 
-- (void)setMessageHandlerOnChannel:(NSString*)channel
-              binaryMessageHandler:(FlutterBinaryMessageHandler)handler {
+- (FlutterBinaryMessengerConnection)setMessageHandlerOnChannel:(NSString*)channel
+                                          binaryMessageHandler:
+                                              (FlutterBinaryMessageHandler)handler {
   if (self.parent) {
-    [self.parent setMessageHandlerOnChannel:channel binaryMessageHandler:handler];
+    return [self.parent setMessageHandlerOnChannel:channel binaryMessageHandler:handler];
+  } else {
+    FML_LOG(WARNING) << "Communicating on a dead channel.";
+    return -1;
+  }
+}
+
+- (void)cleanupConnection:(FlutterBinaryMessengerConnection)connection {
+  if (self.parent) {
+    return [self.parent cleanupConnection:connection];
   } else {
     FML_LOG(WARNING) << "Communicating on a dead channel.";
   }

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

@@ -23,6 +23,7 @@
 #import "flutter/shell/platform/darwin/ios/framework/Source/FlutterPlatformPlugin.h"
 #import "flutter/shell/platform/darwin/ios/framework/Source/FlutterTextInputDelegate.h"
 #import "flutter/shell/platform/darwin/ios/framework/Source/FlutterViewController_Internal.h"
+#import "flutter/shell/platform/darwin/ios/framework/Source/connection_collection.h"
 #import "flutter/shell/platform/darwin/ios/framework/Source/platform_message_response_darwin.h"
 #import "flutter/shell/platform/darwin/ios/framework/Source/profiler_metrics_ios.h"
 #import "flutter/shell/platform/darwin/ios/ios_surface.h"
@@ -78,6 +79,7 @@ static constexpr int kNumProfilerSamplesPerSec = 5;
 
   BOOL _allowHeadlessExecution;
   FlutterBinaryMessengerRelay* _binaryMessenger;
+  std::unique_ptr<flutter::ConnectionCollection> _connections;
 }
 
 - (instancetype)initWithName:(NSString*)labelPrefix {
@@ -110,6 +112,7 @@ static constexpr int kNumProfilerSamplesPerSec = 5;
   _platformViewsController.reset(new flutter::FlutterPlatformViewsController());
 
   _binaryMessenger = [[FlutterBinaryMessengerRelay alloc] initWithParent:self];
+  _connections.reset(new flutter::ConnectionCollection());
 
   NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
   [center addObserver:self
@@ -693,14 +696,26 @@ static constexpr int kNumProfilerSamplesPerSec = 5;
   _shell->GetPlatformView()->DispatchPlatformMessage(platformMessage);
 }
 
-- (void)setMessageHandlerOnChannel:(NSString*)channel
-              binaryMessageHandler:(FlutterBinaryMessageHandler)handler {
+- (FlutterBinaryMessengerConnection)setMessageHandlerOnChannel:(NSString*)channel
+                                          binaryMessageHandler:
+                                              (FlutterBinaryMessageHandler)handler {
   NSParameterAssert(channel);
   if (_shell && _shell->IsSetup()) {
     self.iosPlatformView->GetPlatformMessageRouter().SetMessageHandler(channel.UTF8String, handler);
+    return _connections->AquireConnection(channel.UTF8String);
   } else {
     NSAssert(!handler, @"Setting a message handler before the FlutterEngine has been run.");
     // Setting a handler to nil for a not setup channel is a noop.
+    return flutter::ConnectionCollection::MakeErrorConnection(-1);
+  }
+}
+
+- (void)cleanupConnection:(FlutterBinaryMessengerConnection)connection {
+  if (_shell && _shell->IsSetup()) {
+    std::string channel = _connections->CleanupConnection(connection);
+    if (!channel.empty()) {
+      self.iosPlatformView->GetPlatformMessageRouter().SetMessageHandler(channel.c_str(), nil);
+    }
   }
 }
 

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

@@ -1204,10 +1204,16 @@ static flutter::PointerData::DeviceKind DeviceKindFromTouchType(UITouch* touch)
   [_engine.get().binaryMessenger sendOnChannel:channel message:message binaryReply:callback];
 }
 
-- (void)setMessageHandlerOnChannel:(NSString*)channel
-              binaryMessageHandler:(FlutterBinaryMessageHandler)handler {
+- (FlutterBinaryMessengerConnection)setMessageHandlerOnChannel:(NSString*)channel
+                                          binaryMessageHandler:
+                                              (FlutterBinaryMessageHandler)handler {
   NSAssert(channel, @"The channel must not be null");
-  [_engine.get().binaryMessenger setMessageHandlerOnChannel:channel binaryMessageHandler:handler];
+  return [_engine.get().binaryMessenger setMessageHandlerOnChannel:channel
+                                              binaryMessageHandler:handler];
+}
+
+- (void)cleanupConnection:(FlutterBinaryMessengerConnection)connection {
+  [_engine.get().binaryMessenger cleanupConnection:connection];
 }
 
 #pragma mark - FlutterTextureRegistry

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

@@ -72,6 +72,7 @@ AccessibilityBridge::AccessibilityBridge(UIView* view,
 }
 
 AccessibilityBridge::~AccessibilityBridge() {
+  [accessibility_channel_.get() setMessageHandler:nil];
   clearState();
   view_.accessibilityElements = nil;
 }

shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm

@@ -4,6 +4,7 @@
 
 #import <XCTest/XCTest.h>
 
+#import "flutter/shell/platform/darwin/common/framework/Headers/FlutterBinaryMessenger.h"
 #import "flutter/shell/platform/darwin/common/framework/Headers/FlutterMacros.h"
 #import "flutter/shell/platform/darwin/ios/framework/Headers/FlutterPlatformViews.h"
 #import "flutter/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h"
@@ -368,4 +369,46 @@ fml::RefPtr<fml::TaskRunner> CreateNewThread(std::string name) {
   XCTAssertEqual([accessibility_notifications count], 0ul);
 }
 
+- (void)testAccessibilityMessageAfterDeletion {
+  flutter::MockDelegate mock_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);
+  id messenger = OCMProtocolMock(@protocol(FlutterBinaryMessenger));
+  id engine = OCMClassMock([FlutterEngine class]);
+  id flutterViewController = OCMClassMock([FlutterViewController class]);
+
+  OCMStub([flutterViewController engine]).andReturn(engine);
+  OCMStub([engine binaryMessenger]).andReturn(messenger);
+  FlutterBinaryMessengerConnection connection = 123;
+  OCMStub([messenger setMessageHandlerOnChannel:@"flutter/accessibility"
+                           binaryMessageHandler:[OCMArg any]])
+      .andReturn(connection);
+
+  auto platform_view = std::make_unique<flutter::PlatformViewIOS>(
+      /*delegate=*/mock_delegate,
+      /*rendering_api=*/flutter::IOSRenderingAPI::kSoftware,
+      /*task_runners=*/runners);
+  fml::AutoResetWaitableEvent latch;
+  thread_task_runner->PostTask([&] {
+    auto weakFactory =
+        std::make_unique<fml::WeakPtrFactory<FlutterViewController>>(flutterViewController);
+    platform_view->SetOwnerViewController(weakFactory->GetWeakPtr());
+    auto bridge =
+        std::make_unique<flutter::AccessibilityBridge>(/*view=*/nil,
+                                                       /*platform_view=*/platform_view.get(),
+                                                       /*platform_views_controller=*/nil);
+    XCTAssertTrue(bridge.get());
+    OCMVerify([messenger setMessageHandlerOnChannel:@"flutter/accessibility"
+                               binaryMessageHandler:[OCMArg isNotNil]]);
+    bridge.reset();
+    latch.Signal();
+  });
+  latch.Wait();
+  OCMVerify([messenger cleanupConnection:connection]);
+}
 @end

shell/platform/darwin/ios/framework/Source/connection_collection.cc

+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "flutter/shell/platform/darwin/ios/framework/Source/connection_collection.h"
+
+namespace flutter {
+ConnectionCollection::Connection ConnectionCollection::AquireConnection(
+    const std::string& name) {
+  Connection nextConnection = ++counter_;
+  connections_[name] = nextConnection;
+  return nextConnection;
+}
+
+std::string ConnectionCollection::CleanupConnection(
+    ConnectionCollection::Connection connection) {
+  if (connection > 0) {
+    std::string channel;
+    for (auto& keyValue : connections_) {
+      if (keyValue.second == connection) {
+        channel = keyValue.first;
+        break;
+      }
+    }
+    if (channel.length() > 0) {
+      connections_.erase(channel);
+      return channel;
+    }
+  }
+  return "";
+}
+
+bool ConnectionCollection::IsValidConnection(
+    ConnectionCollection::Connection connection) {
+  return connection > 0;
+}
+
+ConnectionCollection::Connection ConnectionCollection::MakeErrorConnection(
+    int errCode) {
+  if (errCode < 0) {
+    return -1 * errCode;
+  }
+  return errCode;
+}
+
+}  // namespace flutter

shell/platform/darwin/ios/framework/Source/connection_collection.h

+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SHELL_PLATFORM_IOS_FRAMEWORK_SOURCE_CONNECTION_COLLECTION_H_
+#define SHELL_PLATFORM_IOS_FRAMEWORK_SOURCE_CONNECTION_COLLECTION_H_
+
+#include <cstdint>
+#include <map>
+#include <string>
+
+namespace flutter {
+
+/// Maintains a current integer assigned to a name (connections).
+class ConnectionCollection {
+ public:
+  typedef int64_t Connection;
+  static const Connection kInvalidConnection = 0;
+
+  Connection AquireConnection(const std::string& name);
+  ///\returns the name of the channel when cleanup is successful, otherwise
+  ///         the empty string.
+  std::string CleanupConnection(Connection connection);
+
+  static bool IsValidConnection(Connection connection);
+
+  static Connection MakeErrorConnection(int errCode);
+
+ private:
+  std::map<std::string, Connection> connections_;
+  Connection counter_ = 0;
+};
+
+}  // namespace flutter
+
+#endif  // SHELL_PLATFORM_IOS_FRAMEWORK_SOURCE_CONNECTION_COLLECTION_H_

shell/platform/darwin/ios/framework/Source/connection_collection_test.mm

+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import <OCMock/OCMock.h>
+#import <XCTest/XCTest.h>
+
+#import "flutter/shell/platform/darwin/ios/framework/Source/connection_collection.h"
+
+@interface ConnectionCollectionTest : XCTestCase
+@end
+
+@implementation ConnectionCollectionTest
+
+- (void)testSimple {
+  auto connections = std::make_unique<flutter::ConnectionCollection>();
+  flutter::ConnectionCollection::Connection connection = connections->AquireConnection("foo");
+  XCTAssertTrue(connections->CleanupConnection(connection) == "foo");
+  XCTAssertTrue(connections->CleanupConnection(connection).empty());
+}
+
+@end

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm

@@ -447,9 +447,15 @@ static bool OnAcquireExternalTexture(FlutterEngine* engine,
   }
 }
 
-- (void)setMessageHandlerOnChannel:(nonnull NSString*)channel
-              binaryMessageHandler:(nullable FlutterBinaryMessageHandler)handler {
+- (FlutterBinaryMessengerConnection)setMessageHandlerOnChannel:(nonnull NSString*)channel
+                                          binaryMessageHandler:
+                                              (nullable FlutterBinaryMessageHandler)handler {
   _messageHandlers[channel] = [handler copy];
+  return 0;
+}
+
+- (void)cleanupConnection:(FlutterBinaryMessengerConnection)connection {
+  // There hasn't been a need to implement this yet for macOS.
 }
 
 #pragma mark - FlutterPluginRegistry