Fix UIVisualEffectView leak in platform view filter #52591
Conversation
| if (_backdropFilterView != visualEffectView) { | ||
| _backdropFilterView = [visualEffectView retain]; | ||
| } | ||
| self.backdropFilterView = visualEffectView; |
There was a problem hiding this comment.
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.
| @@ -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; | |||
There was a problem hiding this comment.
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.
| XCTAssertNil(weakVisualEffectView1); | ||
| XCTAssertNil(weakVisualEffectView2); |
There was a problem hiding this comment.
This fails on main, passes on this PR.
| @@ -595,6 +620,7 @@ - (void)testAddBackdropFilters { | |||
| XCTAssertEqual(originalView, newView); | |||
| id mockOrignalView = OCMPartialMock(originalView); | |||
| OCMReject([mockOrignalView removeFromSuperview]); | |||
| [mockOrignalView stopMocking]; | |||
There was a problem hiding this comment.
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...
| } | ||
| XCTAssertTrue( | ||
| CGColorEqualToColor(visualEffectSubviewBackgroundColor, UIColor.clearColor.CGColor)); | ||
| XCTAssertNil(weakVisualEffectView); |
There was a problem hiding this comment.
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.
…148030) flutter/engine@bd58d2b...69f2d96 2024-05-08 [email protected] Roll Skia from 4e83fac12783 to 91088addfbd9 (1 revision) (flutter/engine#52691) 2024-05-08 [email protected] Roll Skia from df446841e3cd to 4e83fac12783 (2 revisions) (flutter/engine#52688) 2024-05-08 [email protected] Fix UIVisualEffectView leak in platform view filter (flutter/engine#52591) 2024-05-08 [email protected] [Impeller] initialize ahb swapchain with max entries. (flutter/engine#52670) 2024-05-08 [email protected] Roll Skia from 76f88de905be to df446841e3cd (3 revisions) (flutter/engine#52686) 2024-05-08 [email protected] Reland "Add host_profile_arm64 and host_release_arm64 local engine configurations." (flutter/engine#52633) 2024-05-08 [email protected] Roll Fuchsia Linux SDK from 27ZAbzJkEopmu0Emp... to RDVdjRvCxkJh2NLxS... (flutter/engine#52685) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 27ZAbzJkEopm to RDVdjRvCxkJh If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
I found this while migrating
FlutterPlatformViews_Internal.mmto ARC #52535. I'll land this first.should instead be something like:
But that's already what the built-in MRC
nonatomic, retainproperty setter does, so use that instead.Added a test that passes on this PR and fails on main.