Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 11 additions & 0 deletions shell/platform/darwin/ios/framework/Source/FlutterEngine.mm
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ @interface FlutterEngine () <FlutterTextInputDelegate, FlutterBinaryMessenger>
@property(nonatomic, readonly) NSMutableDictionary* pluginPublications;

@property(nonatomic, readwrite, copy) NSString* isolateId;
@property(nonatomic, retain) id<NSObject> flutterViewControllerWillDeallocObserver;
@end

@interface FlutterEngineRegistrar : NSObject <FlutterPluginRegistrar>
Expand Down Expand Up @@ -111,6 +112,7 @@ - (void)dealloc {

NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
[center removeObserver:self name:UIApplicationDidReceiveMemoryWarningNotification object:nil];
[_flutterViewControllerWillDeallocObserver release];
Copy link
Member

Choose a reason for hiding this comment

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

I have a feeling if this happened before the view controller got dealloc'ed, it's probably a bug right? I don't think this can happen legitimately. Maybe we should print a message here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, that wouldn't be a bug. Releasing the observer detaches this object from the notification center for that message, so after that releasing the FlutterViewController will just not notify a FlutterEngine that he's being dealloc'd.


[super dealloc];
}
Expand Down Expand Up @@ -167,12 +169,21 @@ - (void)setViewController:(FlutterViewController*)viewController {
_viewController = [viewController getWeakPtr];
self.iosPlatformView->SetOwnerViewController(_viewController);
[self maybeSetupPlatformViewChannels];

self.flutterViewControllerWillDeallocObserver =
Copy link
Member

Choose a reason for hiding this comment

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

This is ok but consistency-wise seems a bit unnecessarily different. We already have _shell->GetPlatformView() all over the place in this file. Can't we just call _shell->GetPlatformView()->SetOwnerViewController(null) on notifyViewControllerDeallocated?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not doing that because that would be changing behavior outside the bounds of the problem I was fixing. I'm just moving this file over to the new mechanism I created so we don't have multiple ways to do the same thing. The only logical change I made here was calling _viewController.reset() in notifyViewControllerDeallocated . I didn't technically have to do that to solve the bug, but it will almost certainly cause a bug in a future to keep a weak pointer to a dealloced object. This will turn that future bug to a null pointer exception which will be easier for someone to fix than calling a method into random memory.

Copy link
Member

Choose a reason for hiding this comment

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

Discussed offline. Moving the direct plumbing to NSNotificationCenter is ok. If we're implicitly saying the precedent is also to promote moving other event plumbing to notification center.

[[NSNotificationCenter defaultCenter] addObserverForName:FlutterViewControllerWillDealloc
object:viewController
queue:[NSOperationQueue mainQueue]
usingBlock:^(NSNotification* note) {
[self notifyViewControllerDeallocated];
}];
}

- (void)notifyViewControllerDeallocated {
if (!_allowHeadlessExecution) {
[self destroyContext];
}
_viewController.reset();
}
Copy link
Member

Choose a reason for hiding this comment

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

This should only happen once right? Should we get rid of the observer here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could. I wouldn't recommend it. It would make more sense inside of the observer's block but it isn't necessary there either since the semantics of the message are that the object is dying, it shouldn't be dying twice.


- (void)destroyContext {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@
- (FlutterTextInputPlugin*)textInputPlugin;
- (void)launchEngine:(NSString*)entrypoint libraryURI:(NSString*)libraryOrNil;
- (BOOL)createShell:(NSString*)entrypoint libraryURI:(NSString*)libraryOrNil;
- (void)notifyViewControllerDeallocated;

@end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@

NSNotificationName const FlutterSemanticsUpdateNotification = @"FlutterSemanticsUpdate";

NSNotificationName const FlutterViewControllerWillDealloc = @"FlutterViewControllerWillDealloc";

// This is left a FlutterBinaryMessenger privately for now to give people a chance to notice the
// change. Unfortunately unless you have Werror turned on, incompatible pointers as arguments are
// just a warning.
Expand Down Expand Up @@ -521,7 +523,9 @@ - (void)flushOngoingTouches {
}

- (void)dealloc {
[_engine.get() notifyViewControllerDeallocated];
[[NSNotificationCenter defaultCenter] postNotificationName:FlutterViewControllerWillDealloc
object:self
userInfo:nil];
[[NSNotificationCenter defaultCenter] removeObserver:self];
[_ongoingTouches release];
[super dealloc];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,23 @@
#error ARC must be enabled!
#endif

extern NSNotificationName const FlutterViewControllerWillDealloc;

/// A simple mock class for FlutterEngine.
///
/// OCMockClass can't be used for FlutterEngine sometimes because OCMock retains arguments to
/// invocations and since the init for FlutterViewController calls a method on the
/// FlutterEngine it creates a retain cycle that stops us from testing behaviors related to
/// deleting FlutterViewControllers.
@interface MockEngine : NSObject
@end

@implementation MockEngine
- (void)setViewController:(FlutterViewController*)viewController {
// noop
}
@end

@interface FlutterViewControllerTest : XCTestCase
@end

Expand Down Expand Up @@ -244,4 +261,22 @@ - (UITraitCollection*)fakeTraitCollectionWithContrast:(UIAccessibilityContrast)c
return mockTraitCollection;
}

- (void)testWillDeallocNotification {
XCTestExpectation* expectation =
[[XCTestExpectation alloc] initWithDescription:@"notification called"];
id engine = [[MockEngine alloc] init];
FlutterViewController* realVC = [[FlutterViewController alloc] initWithEngine:engine
nibName:nil
bundle:nil];
id observer =
[[NSNotificationCenter defaultCenter] addObserverForName:FlutterViewControllerWillDealloc
object:nil
queue:[NSOperationQueue mainQueue]
usingBlock:^(NSNotification* _Nonnull note) {
[expectation fulfill];
}];
realVC = nil;
[self waitForExpectations:@[ expectation ] timeout:1.0];
}

@end
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
#include "flutter/shell/platform/darwin/ios/framework/Headers/FlutterViewController.h"
#include "flutter/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h"

FLUTTER_EXPORT
extern NSNotificationName const FlutterViewControllerWillDealloc;

@interface FlutterViewController ()

- (fml::WeakPtr<FlutterViewController>)getWeakPtr;
Expand Down
1 change: 1 addition & 0 deletions shell/platform/darwin/ios/platform_view_ios.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ class PlatformViewIOS final : public PlatformView {
std::unique_ptr<AccessibilityBridge> accessibility_bridge_;
fml::scoped_nsprotocol<FlutterTextInputPlugin*> text_input_plugin_;
fml::closure firstFrameCallback_;
fml::scoped_nsprotocol<NSObject*> dealloc_view_controller_observer_;

// |PlatformView|
void HandlePlatformMessage(fml::RefPtr<flutter::PlatformMessage> message) override;
Expand Down
13 changes: 13 additions & 0 deletions shell/platform/darwin/ios/platform_view_ios.mm
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,19 @@
accessibility_bridge_.reset();
Copy link
Member

Choose a reason for hiding this comment

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

should we get rid of that observer here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are getting rid of the observer a couple lines down, line 55. I don't believe there is any reason why it should be happening higher in the method.

}
owner_controller_ = owner_controller;

// Add an observer that will clear out the owner_controller_ ivar and
// the accessibility_bridge_ in case the view controller is deleted.
dealloc_view_controller_observer_.reset([[NSNotificationCenter defaultCenter]
addObserverForName:FlutterViewControllerWillDealloc
object:owner_controller_.get()
queue:[NSOperationQueue mainQueue]
usingBlock:^(NSNotification* note) {
// Implicit copy of 'this' is fine.
accessibility_bridge_.reset();
owner_controller_.reset();
}]);

if (owner_controller_) {
ios_surface_ =
[static_cast<FlutterView*>(owner_controller.get().view) createSurface:gl_context_];
Expand Down