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

Migrate FlutterEmbedderKeyResponder to ARC #52048

Merged
merged 2 commits into from
Apr 12, 2024
Merged

Migrate FlutterEmbedderKeyResponder to ARC #52048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cfbd14e
Migrate FlutterEmbedderKeyResponder to ARC
jmagman Apr 11, 2024
47254ff
copies
jmagman Apr 12, 2024
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
10 changes: 6 additions & 4 deletions shell/platform/darwin/ios/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ source_set("flutter_framework_source_arc") {
public_configs = [ "//flutter:config" ]

sources = [
"framework/Source/FlutterEmbedderKeyResponder.h",
"framework/Source/FlutterEmbedderKeyResponder.mm",
"framework/Source/FlutterKeyPrimaryResponder.h",
"framework/Source/FlutterKeySecondaryResponder.h",
"framework/Source/FlutterMetalLayer.h",
"framework/Source/FlutterMetalLayer.mm",
"framework/Source/FlutterRestorationPlugin.h",
Expand All @@ -78,6 +82,8 @@ source_set("flutter_framework_source_arc") {
"UIKit.framework",
"IOSurface.framework",
]

deps += [ "//flutter/shell/platform/embedder:embedder_as_internal_library" ]
}

source_set("flutter_framework_source") {
Expand All @@ -100,14 +106,10 @@ source_set("flutter_framework_source") {
"framework/Source/FlutterDartProject_Internal.h",
"framework/Source/FlutterDartVMServicePublisher.h",
"framework/Source/FlutterDartVMServicePublisher.mm",
"framework/Source/FlutterEmbedderKeyResponder.h",
"framework/Source/FlutterEmbedderKeyResponder.mm",
"framework/Source/FlutterEngine.mm",
"framework/Source/FlutterEngineGroup.mm",
"framework/Source/FlutterEngine_Internal.h",
"framework/Source/FlutterHeadlessDartRunner.mm",
"framework/Source/FlutterKeyPrimaryResponder.h",
"framework/Source/FlutterKeySecondaryResponder.h",
"framework/Source/FlutterKeyboardManager.h",
"framework/Source/FlutterKeyboardManager.mm",
"framework/Source/FlutterOverlayView.h",
Expand Down
6 changes: 1 addition & 5 deletions shell/platform/darwin/ios/framework/Source/FlutterEmbedderKeyResponder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,8 @@
#ifndef FLUTTER_SHELL_PLATFORM_DARWIN_IOS_FRAMEWORK_SOURCE_FLUTTEREMBEDDERKEYRESPONDER_H_
#define FLUTTER_SHELL_PLATFORM_DARWIN_IOS_FRAMEWORK_SOURCE_FLUTTEREMBEDDERKEYRESPONDER_H_

#import <Foundation/NSObject.h>
#import <UIKit/UIKit.h>
#import <Foundation/Foundation.h>

#include "fml/memory/weak_ptr.h"

#import "flutter/shell/platform/darwin/ios/framework/Headers/FlutterViewController.h"
#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterKeyPrimaryResponder.h"
#import "flutter/shell/platform/embedder/embedder.h"

Expand Down
35 changes: 13 additions & 22 deletions shell/platform/darwin/ios/framework/Source/FlutterEmbedderKeyResponder.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@

#import "KeyCodeMap_Internal.h"
#import "flutter/shell/platform/darwin/common/framework/Headers/FlutterCodecs.h"
#import "flutter/shell/platform/darwin/common/framework/Headers/FlutterMacros.h"

FLUTTER_ASSERT_ARC

namespace {

Expand Down Expand Up @@ -165,7 +168,7 @@ static uint64_t GetLogicalKeyForEvent(FlutterUIPressProxy* press, NSNumber* mayb
const char* characters =
getEventCharacters(press.key.charactersIgnoringModifiers, press.key.keyCode);
NSString* keyLabel =
characters == nullptr ? nil : [[[NSString alloc] initWithUTF8String:characters] autorelease];
characters == nullptr ? nil : [[NSString alloc] initWithUTF8String:characters];
NSUInteger keyLabelLength = [keyLabel length];
// If this key is printable, generate the logical key from its Unicode
// value. Control keys such as ESC, CTRL, and SHIFT are not printable. HOME,
Expand Down Expand Up @@ -252,7 +255,7 @@ static bool isKeyDown(FlutterUIPressProxy* press) API_AVAILABLE(ios(13.4)) {
*/
@interface FlutterKeyPendingResponse : NSObject

@property(readonly) FlutterEmbedderKeyResponder* responder;
@property(readonly, weak) FlutterEmbedderKeyResponder* responder;

@property(nonatomic) uint64_t responseId;

Expand Down Expand Up @@ -302,7 +305,7 @@ - (void)resolveTo:(BOOL)handled;
* Only set in debug mode. Nil in release mode, or if the callback has not been
* handled.
*/
@property(readonly) NSString* debugHandleSource;
@property(readonly, copy) NSString* debugHandleSource;
@end

@implementation FlutterKeyCallbackGuard {
Expand All @@ -319,11 +322,6 @@ - (nonnull instancetype)initWithCallback:(FlutterAsyncKeyCallback)callback {
return self;
}

- (void)dealloc {
[_callback release];
[super dealloc];
}

- (void)pendTo:(nonnull NSMutableDictionary<NSNumber*, FlutterAsyncKeyCallback>*)pendingResponses
withId:(uint64_t)responseId {
NSAssert(!_handled, @"This callback has been handled by %@.", _debugHandleSource);
Expand Down Expand Up @@ -364,7 +362,7 @@ @interface FlutterEmbedderKeyResponder ()
* The keys of the dictionary are physical keys, while the values are the logical keys
* of the key down event.
*/
@property(nonatomic, retain, readonly) NSMutableDictionary<NSNumber*, NSNumber*>* pressingRecords;
@property(nonatomic, copy, readonly) NSMutableDictionary<NSNumber*, NSNumber*>* pressingRecords;

/**
* A constant mask for NSEvent.modifierFlags that Flutter synchronizes with.
Expand Down Expand Up @@ -403,7 +401,7 @@ @interface FlutterEmbedderKeyResponder ()
* Its values are |responseId|s, and keys are the callback that was received
* along with the event.
*/
@property(nonatomic, retain, readonly)
@property(nonatomic, copy, readonly)
NSMutableDictionary<NSNumber*, FlutterAsyncKeyCallback>* pendingResponses;

/**
Expand Down Expand Up @@ -502,13 +500,6 @@ - (nonnull instancetype)initWithSendEvent:(FlutterSendKeyEvent)sendEvent {
return self;
}

- (void)dealloc {
[_sendEvent release];
[_pressingRecords release];
[_pendingResponses release];
[super dealloc];
}

- (void)handlePress:(nonnull FlutterUIPressProxy*)press
callback:(FlutterAsyncKeyCallback)callback API_AVAILABLE(ios(13.4)) {
if (@available(iOS 13.4, *)) {
Expand All @@ -522,11 +513,11 @@ - (void)handlePress:(nonnull FlutterUIPressProxy*)press
FlutterKeyCallbackGuard* guardedCallback = nil;
switch (press.phase) {
case UIPressPhaseBegan:
guardedCallback = [[[FlutterKeyCallbackGuard alloc] initWithCallback:callback] autorelease];
guardedCallback = [[FlutterKeyCallbackGuard alloc] initWithCallback:callback];
[self handlePressBegin:press callback:guardedCallback];
break;
case UIPressPhaseEnded:
guardedCallback = [[[FlutterKeyCallbackGuard alloc] initWithCallback:callback] autorelease];
guardedCallback = [[FlutterKeyCallbackGuard alloc] initWithCallback:callback];
[self handlePressEnd:press callback:guardedCallback];
break;
case UIPressPhaseChanged:
Expand Down Expand Up @@ -632,9 +623,9 @@ - (void)sendPrimaryFlutterEvent:(const FlutterKeyEvent&)event
_responseId += 1;
uint64_t responseId = _responseId;
FlutterKeyPendingResponse* pending =
[[[FlutterKeyPendingResponse alloc] initWithHandler:self responseId:responseId] autorelease];
[[FlutterKeyPendingResponse alloc] initWithHandler:self responseId:responseId];
[callback pendTo:_pendingResponses withId:responseId];
_sendEvent(event, HandleResponse, pending);
_sendEvent(event, HandleResponse, (__bridge_retained void* _Nullable)pending);
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 had tests that crashed when this was just __bridge (reminder about what this does: https://medium.com/@hadhi631/when-to-use-bridge-bridge-transfer-cfbridgingrelease-and-bridge-retained-cfbridgingretain-4b3d2fc932df)

}

- (void)sendEmptyEvent {
Expand Down Expand Up @@ -883,7 +874,7 @@ - (UInt32)adjustModifiers:(nonnull FlutterUIPressProxy*)press API_AVAILABLE(ios(

namespace {
void HandleResponse(bool handled, void* user_data) {
FlutterKeyPendingResponse* pending = reinterpret_cast<FlutterKeyPendingResponse*>(user_data);
FlutterKeyPendingResponse* pending = (__bridge_transfer FlutterKeyPendingResponse*)user_data;
Copy link
Member Author

Choose a reason for hiding this comment

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

Same, tests crashed with just a __bridge.

Copy link
Contributor

Choose a reason for hiding this comment

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

What crash did you get? Does the owner of user_data release it too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh i saw the caller uses autorelease to keep it alive for current runloop. So in ARC it makes sense to bridge retain, and bridge release afterwards.

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 crash was an overrelease, it was zombied.

Copy link
Contributor

@hellohuanlin hellohuanlin Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ya what you did here should be fine. In the new code, the caller site doesn't release it, so won't be over-released.

[pending.responder handleResponse:handled forId:pending.responseId];
}
} // namespace
9 changes: 2 additions & 7 deletions shell/platform/darwin/ios/framework/Source/FlutterEmbedderKeyResponderTest.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,13 @@
@interface TestKeyEvent : NSObject
@property(nonatomic) FlutterKeyEvent* data;
@property(nonatomic) FlutterKeyEventCallback callback;
@property(nonatomic) void* _Nullable userData;
- (nonnull instancetype)initWithEvent:(const FlutterKeyEvent*)event
callback:(nullable FlutterKeyEventCallback)callback
userData:(void* _Nullable)userData;
- (BOOL)hasCallback;
- (void)respond:(BOOL)handled;
@property(nonatomic, nullable) void* userData;
@end

@implementation TestKeyEvent
- (instancetype)initWithEvent:(const FlutterKeyEvent*)event
callback:(nullable FlutterKeyEventCallback)callback
userData:(void* _Nullable)userData {
userData:(nullable void*)userData {
self = [super init];
_data = new FlutterKeyEvent(*event);
if (event->character != nullptr) {
Expand Down