[macOS] Fix engine/binaryMessenger retain cycle (#44471)

Previously, FlutterEngine.binaryMessenger returned the FlutterEngine
instance itself, which meant that channels/plugins could hold a strong
reference to the engine and thus cause a retain cycle, preventing the
engine from being deallocated.

We introduce FlutterBinaryMessengerRelay, which implements the
FlutterBinaryMessenger protocol, by delegating back to the engine, to
which it holds a weak reference, thus avoiding the retain cycle.

This also changes the FlutterBinaryMessengerRelay.parent property from
an assign property to a weak property since that code is compiled with
ARC enabled as of #44395.

This patch also rearranges the headers to comply with our style guide:
related headers for this file, C headers, C++ headers, other Flutter
headers, in that order.

Fixes: flutter/flutter#116445

Author: cbracken

shell/platform/darwin/common/framework/Source/FlutterBinaryMessengerRelay.h

@@ -12,7 +12,7 @@
 FLUTTER_DARWIN_EXPORT
 #endif
 @interface FlutterBinaryMessengerRelay : NSObject <FlutterBinaryMessenger>
-@property(nonatomic, assign) NSObject<FlutterBinaryMessenger>* parent;
+@property(nonatomic, weak) NSObject<FlutterBinaryMessenger>* parent;
 - (instancetype)initWithParent:(NSObject<FlutterBinaryMessenger>*)parent;
 @end
 

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

@@ -13,6 +13,7 @@
 #include "flutter/shell/platform/common/engine_switches.h"
 #include "flutter/shell/platform/embedder/embedder.h"
 
+#import "flutter/shell/platform/darwin/common/framework/Source/FlutterBinaryMessengerRelay.h"
 #import "flutter/shell/platform/darwin/macos/framework/Headers/FlutterAppDelegate.h"
 #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterAppDelegate_Internal.h"
 #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterCompositor.h"
@@ -402,6 +403,9 @@ @implementation FlutterEngine {
 
   // Whether any portion of the application is currently visible.
   BOOL _visible;
+
+  // Proxy to allow plugins, channels to hold a weak reference to the binary messenger (self).
+  FlutterBinaryMessengerRelay* _binaryMessenger;
 }
 
 - (instancetype)initWithName:(NSString*)labelPrefix project:(FlutterDartProject*)project {
@@ -420,6 +424,7 @@ - (instancetype)initWithName:(NSString*)labelPrefix
   _currentMessengerConnection = 1;
   _allowHeadlessExecution = allowHeadlessExecution;
   _semanticsEnabled = NO;
+  _binaryMessenger = [[FlutterBinaryMessengerRelay alloc] initWithParent:self];
   _isResponseValid = [[NSMutableArray alloc] initWithCapacity:1];
   [_isResponseValid addObject:@YES];
   // kFlutterImplicitViewId is reserved for the implicit view.
@@ -723,9 +728,7 @@ - (FlutterCompositor*)createFlutterCompositor {
 }
 
 - (id<FlutterBinaryMessenger>)binaryMessenger {
-  // TODO(stuartmorgan): Switch to FlutterBinaryMessengerRelay to avoid plugins
-  // keeping the engine alive.
-  return self;
+  return _binaryMessenger;
 }
 
 #pragma mark - Framework-internal methods

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

@@ -2,10 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <objc/objc.h>
 #import "flutter/shell/platform/darwin/macos/framework/Headers/FlutterEngine.h"
 #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h"
-#include "gtest/gtest.h"
+
+#include <objc/objc.h>
 
 #include <functional>
 #include <thread>
@@ -14,13 +14,15 @@
 #include "flutter/lib/ui/window/platform_message.h"
 #include "flutter/shell/platform/common/accessibility_bridge.h"
 #import "flutter/shell/platform/darwin/common/framework/Headers/FlutterChannels.h"
+#import "flutter/shell/platform/darwin/common/framework/Source/FlutterBinaryMessengerRelay.h"
 #import "flutter/shell/platform/darwin/macos/framework/Headers/FlutterAppDelegate.h"
 #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterEngineTestUtils.h"
 #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTestUtils.h"
 #include "flutter/shell/platform/embedder/embedder.h"
 #include "flutter/shell/platform/embedder/embedder_engine.h"
 #include "flutter/shell/platform/embedder/test_utils/proc_table_replacement.h"
 #include "flutter/testing/test_dart_native_resolver.h"
+#include "gtest/gtest.h"
 
 // CREATE_NATIVE_ENTRY and MOCK_ENGINE_PROC are leaky by design
 // NOLINTBEGIN(clang-analyzer-core.StackAddressEscape)
@@ -496,6 +498,38 @@ - (NSApplicationTerminateReply)applicationShouldTerminate:(NSApplication* _Nonnu
   EXPECT_TRUE(called);
 }
 
+// Verify that the engine is not retained indirectly via the binary messenger held by channels and
+// plugins. Previously, FlutterEngine.binaryMessenger returned the engine itself, and thus plugins
+// could cause a retain cycle, preventing the engine from being deallocated.
+// FlutterEngine.binaryMessenger now returns a FlutterBinaryMessengerRelay whose pointer back to
+// the engine is cleared when the engine is deallocated.
+// Issue: https://github.com/flutter/flutter/issues/116445
+TEST_F(FlutterEngineTest, FlutterBinaryMessengerNullsParentOnEngineRelease) {
+  FlutterBinaryMessengerRelay* relay = nil;
+  @autoreleasepool {
+    // Create a test engine.
+    NSString* fixtures = @(flutter::testing::GetFixturesPath());
+    FlutterDartProject* project = [[FlutterDartProject alloc]
+        initWithAssetsPath:fixtures
+               ICUDataPath:[fixtures stringByAppendingString:@"/icudtl.dat"]];
+    FlutterEngine* engine = [[FlutterEngine alloc] initWithName:@"test"
+                                                        project:project
+                                         allowHeadlessExecution:true];
+
+    // Get the binary messenger for the engine.
+    id<FlutterBinaryMessenger> binaryMessenger = engine.binaryMessenger;
+    ASSERT_TRUE([binaryMessenger isKindOfClass:[FlutterBinaryMessengerRelay class]]);
+    relay = (FlutterBinaryMessengerRelay*)binaryMessenger;
+
+    // Verify the relay parent (the engine) is non-nil.
+    EXPECT_NE(relay.parent, nil);
+  }
+
+  // Once the engine has been deallocated, verify the relay parent is nil, and thus the engine is
+  // not retained by the holder of the relay.
+  EXPECT_EQ(relay.parent, nil);
+}
+
 // If a channel overrides a previous channel with the same name, cleaning
 // the previous channel should not affect the new channel.
 //