Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Fix UIVisualEffectView leak in platform view filter #52591

Merged
merged 1 commit into from
May 8, 2024
Merged

Fix UIVisualEffectView leak in platform view filter #52591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 47 additions & 15 deletions shell/platform/darwin/ios/framework/Source/FlutterPlatformViewsTest.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,13 +251,38 @@ - (void)testChildClippingViewHitTests {

- (void)testReleasesBackdropFilterSubviewsOnChildClippingViewDealloc {
__weak NSMutableArray<UIVisualEffectView*>* weakBackdropFilterSubviews = nil;
__weak UIVisualEffectView* weakVisualEffectView1 = nil;
__weak UIVisualEffectView* weakVisualEffectView2 = nil;

@autoreleasepool {
ChildClippingView* clipping_view = [[ChildClippingView alloc] initWithFrame:CGRectZero];
weakBackdropFilterSubviews = clipping_view.backdropFilterSubviews;
ChildClippingView* clippingView = [[ChildClippingView alloc] initWithFrame:CGRectZero];
UIVisualEffectView* visualEffectView1 = [[UIVisualEffectView alloc]
initWithEffect:[UIBlurEffect effectWithStyle:UIBlurEffectStyleLight]];
weakVisualEffectView1 = visualEffectView1;
PlatformViewFilter* platformViewFilter1 =
[[PlatformViewFilter alloc] initWithFrame:CGRectMake(0, 0, 10, 10)
blurRadius:5
visualEffectView:visualEffectView1];

[clippingView applyBlurBackdropFilters:@[ platformViewFilter1 ]];

// Replace the blur filter to validate the original and new UIVisualEffectView are released.
UIVisualEffectView* visualEffectView2 = [[UIVisualEffectView alloc]
initWithEffect:[UIBlurEffect effectWithStyle:UIBlurEffectStyleDark]];
weakVisualEffectView2 = visualEffectView2;
PlatformViewFilter* platformViewFilter2 =
[[PlatformViewFilter alloc] initWithFrame:CGRectMake(0, 0, 10, 10)
blurRadius:5
visualEffectView:visualEffectView2];
[clippingView applyBlurBackdropFilters:@[ platformViewFilter2 ]];

weakBackdropFilterSubviews = clippingView.backdropFilterSubviews;
XCTAssertNotNil(weakBackdropFilterSubviews);
clipping_view = nil;
clippingView = nil;
}
XCTAssertNil(weakBackdropFilterSubviews);
XCTAssertNil(weakVisualEffectView1);
XCTAssertNil(weakVisualEffectView2);
Comment on lines +284 to +285
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails on main, passes on this PR.

}

- (void)testApplyBackdropFilter {
Expand Down Expand Up @@ -595,6 +620,7 @@ - (void)testAddBackdropFilters {
XCTAssertEqual(originalView, newView);
id mockOrignalView = OCMPartialMock(originalView);
OCMReject([mockOrignalView removeFromSuperview]);
[mockOrignalView stopMocking];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related but this test was causing issues because the original view would eventually be dealloced after the test, which would call removeFromSuperview and the reject would fail. I'm not convinced this is testing anything since the reject isn't set up before the code is exercised, but that's investigation for another day...

}
}

Expand Down Expand Up @@ -1302,20 +1328,26 @@ - (void)testApplyBackdropFilterAPIChangedInvalidInputRadius {
}

- (void)testBackdropFilterVisualEffectSubviewBackgroundColor {
UIVisualEffectView* visualEffectView = [[UIVisualEffectView alloc]
initWithEffect:[UIBlurEffect effectWithStyle:UIBlurEffectStyleLight]];
PlatformViewFilter* platformViewFilter =
[[PlatformViewFilter alloc] initWithFrame:CGRectMake(0, 0, 10, 10)
blurRadius:5
visualEffectView:visualEffectView];
CGColorRef visualEffectSubviewBackgroundColor = nil;
for (UIView* view in [platformViewFilter backdropFilterView].subviews) {
if ([NSStringFromClass([view class]) hasSuffix:@"VisualEffectSubview"]) {
visualEffectSubviewBackgroundColor = view.layer.backgroundColor;
__weak UIVisualEffectView* weakVisualEffectView;

@autoreleasepool {
UIVisualEffectView* visualEffectView = [[UIVisualEffectView alloc]
initWithEffect:[UIBlurEffect effectWithStyle:UIBlurEffectStyleLight]];
weakVisualEffectView = visualEffectView;
PlatformViewFilter* platformViewFilter =
[[PlatformViewFilter alloc] initWithFrame:CGRectMake(0, 0, 10, 10)
blurRadius:5
visualEffectView:visualEffectView];
CGColorRef visualEffectSubviewBackgroundColor = nil;
for (UIView* view in [platformViewFilter backdropFilterView].subviews) {
if ([NSStringFromClass([view class]) hasSuffix:@"VisualEffectSubview"]) {
visualEffectSubviewBackgroundColor = view.layer.backgroundColor;
}
}
XCTAssertTrue(
CGColorEqualToColor(visualEffectSubviewBackgroundColor, UIColor.clearColor.CGColor));
}
XCTAssertTrue(
CGColorEqualToColor(visualEffectSubviewBackgroundColor, UIColor.clearColor.CGColor));
XCTAssertNil(weakVisualEffectView);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This passes on main, so wasn't actually a problem, but hey I wrote the test before I knew that so may as well keep it. These kinds of "weak" tests have been helpful as I've done ARC migration.

}

- (void)testCompositePlatformView {
Expand Down
4 changes: 1 addition & 3 deletions shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,9 +172,7 @@ - (void)updateVisualEffectView:(UIVisualEffectView*)visualEffectView {
visualEffectSubview.layer.backgroundColor = UIColor.clearColor.CGColor;
visualEffectView.frame = _frame;

if (_backdropFilterView != visualEffectView) {
_backdropFilterView = [visualEffectView retain];
}
self.backdropFilterView = visualEffectView;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old _backdropFilterView value wasn't released when being set to a new visualEffectView. Instead of adding a release, instead use the existing retain property setter.

}

@end
Expand Down
2 changes: 0 additions & 2 deletions testing/ios/IosUnitTests/IosUnitTests.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,6 @@
BUNDLE_LOADER = "$(TEST_HOST)";
CLANG_ALLOW_NON_MODULAR_INCLUDES_IN_FRAMEWORK_MODULES = YES;
CLANG_CXX_LANGUAGE_STANDARD = "gnu++17";
CLANG_ENABLE_OBJC_ARC = NO;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test dylib is built with ARC from the BUILD.gn, so this doesn't do anything. But I found setting it explicitly to NO confusing, and it's unnecessary.

CODE_SIGN_STYLE = Automatic;
HEADER_SEARCH_PATHS = (
../../../..,
Expand Down Expand Up @@ -532,7 +531,6 @@
BUNDLE_LOADER = "$(TEST_HOST)";
CLANG_ALLOW_NON_MODULAR_INCLUDES_IN_FRAMEWORK_MODULES = YES;
CLANG_CXX_LANGUAGE_STANDARD = "gnu++17";
CLANG_ENABLE_OBJC_ARC = NO;
CODE_SIGN_STYLE = Automatic;
HEADER_SEARCH_PATHS = (
../../../..,
Expand Down