Skip to content

Commit

Permalink
[macOS] Improve engine retain cycle testing (flutter#44509)
Browse files Browse the repository at this point in the history
Adds testing that verifies that the engine is not retained via the FlutterTextureRegistry that is returned from the FlutterTextureRegistrar returned by the engine.

This also simplifies the existing test for a retain cycle via the FlutterBinaryMessenger by avoiding manually casting the binary messenger to a FlutterBinaryMessengerRelay, or knowing any of its implementation details.

Issue: flutter/flutter#116445

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
  • Loading branch information
cbracken authored Aug 8, 2023
1 parent eda0166 commit b5f46d3
Showing 1 changed file with 33 additions and 13 deletions.
46 changes: 33 additions & 13 deletions shell/platform/darwin/macos/framework/Source/FlutterEngineTest.mm
Original file line number Diff line number Diff line change
Expand Up @@ -501,11 +501,12 @@ - (NSApplicationTerminateReply)applicationShouldTerminate:(NSApplication* _Nonnu
// 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.
// FlutterEngine.binaryMessenger now returns a FlutterBinaryMessengerRelay whose weak 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;
TEST_F(FlutterEngineTest, FlutterBinaryMessengerDoesNotRetainEngine) {
__weak FlutterEngine* weakEngine;
id<FlutterBinaryMessenger> binaryMessenger = nil;
@autoreleasepool {
// Create a test engine.
NSString* fixtures = @(flutter::testing::GetFixturesPath());
Expand All @@ -515,19 +516,38 @@ - (NSApplicationTerminateReply)applicationShouldTerminate:(NSApplication* _Nonnu
FlutterEngine* engine = [[FlutterEngine alloc] initWithName:@"test"
project:project
allowHeadlessExecution:true];
weakEngine = engine;
binaryMessenger = engine.binaryMessenger;
}

// Get the binary messenger for the engine.
id<FlutterBinaryMessenger> binaryMessenger = engine.binaryMessenger;
ASSERT_TRUE([binaryMessenger isKindOfClass:[FlutterBinaryMessengerRelay class]]);
relay = (FlutterBinaryMessengerRelay*)binaryMessenger;
// Once the engine has been deallocated, verify the weak engine pointer is nil, and thus not
// retained by the relay.
EXPECT_NE(binaryMessenger, nil);
EXPECT_EQ(weakEngine, nil);
}

// Verify the relay parent (the engine) is non-nil.
EXPECT_NE(relay.parent, nil);
// Verify that the engine is not retained indirectly via the texture registry held by plugins.
// Issue: https://github.com/flutter/flutter/issues/116445
TEST_F(FlutterEngineTest, FlutterTextureRegistryDoesNotReturnEngine) {
__weak FlutterEngine* weakEngine;
id<FlutterTextureRegistry> textureRegistry;
@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];
id<FlutterPluginRegistrar> registrar = [engine registrarForPlugin:@"MyPlugin"];
textureRegistry = registrar.textures;
}

// 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);
// Once the engine has been deallocated, verify the weak engine pointer is nil, and thus not
// retained via the texture registry.
EXPECT_NE(textureRegistry, nil);
EXPECT_EQ(weakEngine, nil);
}

// If a channel overrides a previous channel with the same name, cleaning
Expand Down

0 comments on commit b5f46d3

Please sign in to comment.