From 39d8620761e5e1f799ce3f6bec0d6da30c4fa63d Mon Sep 17 00:00:00 2001
From: Chris Bracken <chris@bracken.jp>
Date: Mon, 7 Aug 2023 13:05:10 -0700
Subject: [PATCH] [macOS] Fix engine/binaryMessenger retain cycle
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 https://github.com/flutter/engine/pull/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.
Issue: https://github.com/flutter/flutter/issues/116445
---
.../Source/FlutterBinaryMessengerRelay.h | 2 +-
.../macos/framework/Source/FlutterEngine.mm | 9 +++--
.../framework/Source/FlutterEngineTest.mm | 38 ++++++++++++++++++-
3 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/shell/platform/darwin/common/framework/Source/FlutterBinaryMessengerRelay.h b/shell/platform/darwin/common/framework/Source/FlutterBinaryMessengerRelay.h
index 1c17329498873..392d6bac3775a 100644
--- a/shell/platform/darwin/common/framework/Source/FlutterBinaryMessengerRelay.h
+++ b/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
diff --git a/shell/platform/darwin/macos/framework/Source/FlutterEngine.mm b/shell/platform/darwin/macos/framework/Source/FlutterEngine.mm
index f6764167dec4f..648c5d213b08c 100644
--- a/shell/platform/darwin/macos/framework/Source/FlutterEngine.mm
+++ b/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
diff --git a/shell/platform/darwin/macos/framework/Source/FlutterEngineTest.mm b/shell/platform/darwin/macos/framework/Source/FlutterEngineTest.mm
index fea57a9adf40f..9b3468a33c03c 100644
--- a/shell/platform/darwin/macos/framework/Source/FlutterEngineTest.mm
+++ b/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,6 +14,7 @@
#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"
@@ -21,6 +22,7 @@
#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.
//