[deep link][ios] Update openURL method to reflect the result from framework #52643

hangyujin · 2024-05-07T23:18:50Z

follow up on comments on #52350

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

hangyujin · 2024-05-08T23:51:35Z

i realized
openURL:options:completionHandler: is an async method
but application:openURL:options:(https://developer.apple.com/documentation/uikit/uiapplicationdelegate/1623112-application?language=objc ) is a non-async method so I cannot just add a completionHandler to it ,

So i guess the return value of it can only reflect the deep link flag value, not the bool result from framework then.

chunhtai · 2024-05-09T20:20:29Z

shell/platform/darwin/ios/framework/Source/FlutterAppDelegate.mm

+- (void)openURL:(NSURL*)url
+ options:(NSDictionary<UIApplicationOpenExternalURLOptionsKey, id>*)options
+ completionHandler:(void (^)(BOOL success))completion {
+ if (![self isFlutterDeepLinkingEnabled]) {


Even this is No, we still want to go through plugins, I think?

Can you explain why we want to do that?

for those who use uni-link or their own plugin to handle deeplink, they would set this to no, right?

Yes they would set this to no.
but for those using uni-link and didn't set isFlutterDeepLinkingEnabled flag, the current behavior is already return NO for these methods and don't invoke the framework method. This PR is not changing their experience.

you are right, I misread the code, looks like the plugin would have been called already before reaching here.

chunhtai · 2024-05-09T20:21:33Z

shell/platform/darwin/ios/framework/Source/FlutterAppDelegate.mm

@@ -171,7 +188,14 @@ - (BOOL)application:(UIApplication*)application
 if ([_lifeCycleDelegate application:application openURL:url options:options]) {
 return YES;
 }
- return [self openURL:url];
+ if (![self isFlutterDeepLinkingEnabled]) {


do we still ned this check if we go delegate to other selector in line 194?

I just add this so the return value reflect the value of the flag.

chunhtai · 2024-05-09T20:23:22Z

shell/platform/darwin/ios/framework/Source/FlutterAppDelegate.mm

+ options:options
+ completionHandler:^(BOOL success){
+ }];
+ return YES;


can this method get the result from completion handler only?

Also is implementing this selector optional if we already implement the one with completionhandler?

can this method get the result from completion handler only?>>

this application:openURL:options: method is non-async, i don't think its return value can be set from the completion handler so i used the deep link flag value.

But should this method return before the completion handler is complete? Before, this method didn't return until pushRouteInformation was actually sent, whereas now it would return instantly.
It seems like you'd want to spin the runloop until the completion handler was done, and then return the value from the completion handler.

what's the best practice to "spin the runloop"?

I updated the code to wait for the completion handler but got failures :

Failures for clang-tidy on /Volumes/Work/s/w/ir/cache/builder/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterAppDelegate.mm:
/Volumes/Work/s/w/ir/cache/builder/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterAppDelegate.mm:178:3: error: Waiting on a callback using a semaphore creates useless threads and is subject to priority inversion; consider using a synchronous API or changing the caller to be asynchronous [clang-analyzer-optin.performance.GCDAntipattern,-warnings-as-errors]
178 | dispatch_semaphore_wait(semaphore, 5 * NSEC_PER_SEC);

chunhtai

LGTM

chunhtai · 2024-05-13T16:19:47Z

shell/platform/darwin/ios/framework/Source/FlutterAppDelegate.mm

+- (void)openURL:(NSURL*)url
+ options:(NSDictionary<UIApplicationOpenExternalURLOptionsKey, id>*)options
+ completionHandler:(void (^)(BOOL success))completion {
+ if (![self isFlutterDeepLinkingEnabled]) {


you are right, I misread the code, looks like the plugin would have been called already before reaching here.

jmagman · 2024-05-13T20:46:23Z

shell/platform/darwin/ios/framework/Source/FlutterAppDelegate.mm

@@ -149,18 +155,29 @@ - (BOOL)openURL:(NSURL*)url {
 if (didTimeout) {


Now that this is async, can we get rid of the waitForFirstFrame?

I remembered this was added to workaround the issue that when we receive openurl, we don't know what the current state of flutter is in. If we send if before the first frame is drawn, the message will be dropped because the widgetbindingobserver is not yet registered. It is also too late to set initialroute at this point since it is already consumed by the framework.

but this was a long time ago, I am not sure if this is still the case

It seems like FlutterAppDelegate needs to know a lot of details about the engine, the navigationChannel, and the available method. What if this changed to:

- (void)openURL:(NSURL*)url options:(NSDictionary<UIApplicationOpenExternalURLOptionsKey, id>*)options completionHandler:(void (^)(BOOL success))completion { if (![self isFlutterDeepLinkingEnabled]) { completion(NO); } else { FlutterViewController* flutterViewController = [self rootFlutterViewController]; if (flutterViewController) { [flutterViewController openDeepLink:url completionHandler:completion]; } else { FML_LOG(ERROR) << "Attempting to open an URL without a Flutter RootViewController."; completion(NO); } } }

FlutterViewController_Internal.h

- (void)openDeepLink:(NSURL*)url completionHandler:(void (^)(BOOL success))completion;

and then in FlutterViewController:

- (void)openDeepLink:(NSURL*)url completionHandler:(void (^)(BOOL success))completion { [_engine.get() waitForFirstFrame:3.0 callback:^(BOOL didTimeout) { if (didTimeout) { FML_LOG(ERROR) << "Timeout waiting for the first frame when launching an URL."; completion(NO); } else { // invove the method and get the result [[_engine.get() navigationChannel] invokeMethod:@"pushRouteInformation" arguments:@{ @"location" : url.absoluteString ?: [NSNull null], } result:^(id _Nullable result) { BOOL success = [result isKindOfClass:[NSNumber class]] && [result boolValue]; if (!success) { // Logging the error if the result is not successful FML_LOG(ERROR) << "Failed to handle route information in Flutter."; } completion(success); }]; } }]; }

yup, It does seem like the viewcontroller may be a better place to put these logic

thanks, this looks good, updated!

jmagman · 2024-05-14T20:35:24Z

shell/platform/darwin/ios/framework/Source/FlutterAppDelegate.mm

+ // Use a dispatch semaphore to wait for the completion handler
+ dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);


You won't want a dispatch_semaphore on the main thread, that will cause hangs and potentially deadlocks.

The run loop is a processing loop for event handling, and there's one assigned to the main thread. I believe what you want is to kick off the request, and spin the main run loop until it completes, then return the value from this method.

I didn't run this code so you should test it, but something like:

__block BOOL openURLSuccess = NO; __block BOOL openURLCompleted = NO; [self openURL:url options:options completionHandler:^(BOOL success) { openURLSuccess = success; openURLCompleted = YES; }]; while (!openURLCompleted) { [NSRunLoop.currentRunLoop runUntilDate:[NSDate dateWithTimeIntervalSinceNow:0.1]]; } return openURLSuccess;

Thanks for educating me about the The run loop

I have some more questions :

is a while loop a form of busy waiting and will it cause some performance issues? Should i also add a timeout to prevent an infinite loop?

Looks like the async api [openURL:options:completionHandler:] is newer than the non-async api [application:openURL:options:] , is it possible the async one is replacing the non-async one? if so, it may be ok the async api returns the value from the framework but the old non-async api remains the old behavior to return the flag value.

is a while loop a form of busy waiting and will it cause some performance issues? Should i also add a timeout to prevent an infinite loop?

Are you concerned the code you're calling didTimeout logic won't work? You could pass in that timeout into the view controller method so it's obvious it handles timing out?

I'm also a bit concerned " navigationChannel invokeMethod:arguments:result: " will time out 😅 Is it possible?

You could add a timeout instead of the while loop, that would work too. And return NO if it times out.
There are a lot of timeouts going on here, it seems like it would be bad if it times out but the pushRouteInformation request has gone through, but not yet returned?

From the code, the delay comes from waitForFirstFrame (3 seconds) + pushRouteInformation (unknown delay). I'd run it and see how long it takes pushRouteInformation to call its result block (you can do so by printing out the time elapsed), then pick the shortest delay that you feel confident that will cover this + 3 seconds.

is a while loop a form of busy waiting and will it cause some performance issues?

From the API doc the UI shouldn't be blocked, but please test it out. It will be easier to test it by adding a dummy delay to your completion block, so you have enough time to scroll the screen after the open URL call. Something like:

[self openURL:url options:options completionHandler:^(BOOL success) { // Note: delay this call for 20 seconds for debugging purpose dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(20 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ openURLSuccess = success; openURLCompleted = YES; } }];

Sorry for the delay, i just got my iOS device registered under Google's Apple developer account so i can test deep link on it.

i tested pushRouteInformation' ant it's under a second. i think 5 seconds in total can cover it.

you're correct, adding delay will not cause UI to be blocked.

but it looks like my run loop will have a deadlock with the dispatch_async here: https://github.com/flutter/engine/blob/main/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm#L1428

engine/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

Line 1428 in 85f4429

dispatch_async(dispatch_get_main_queue(), ^{

because the dispatch_async waits for runloop to end and the runloop is waiting for the callback from dispatch_async. So I don't think i should use a run loop.

Let me try it out

basically it's like

- (void)fakedispatch:(void (^)(BOOL success))completion { dispatch_queue_t queue = dispatch_get_main_queue(); dispatch_async(queue, ^{ NSLog(@"fakedispatch:completionHandler: "); completion(YES); }); } - (void)fakeRunloop { __block BOOL completed = NO; CFTimeInterval startTime = CACurrentMediaTime(); [self fakedispatch:^(BOOL success) { completed = YES; }]; while (!completed) { NSLog(@"runing loop %f", CACurrentMediaTime() - startTime); [NSRunLoop.currentRunLoop runUntilDate:[NSDate dateWithTimeIntervalSinceNow:1.0]]; } }

and it will be a deadlock

shell/platform/darwin/ios/framework/Source/FlutterAppDelegate.mm

hangyujin · 2024-05-16T20:14:46Z

@jmagman ping for review

hellohuanlin · 2024-05-16T22:56:38Z

shell/platform/darwin/ios/framework/Source/FlutterAppDelegate.mm

- // Not set or NO.
- return NO;
+ // if not set, return NO
+ return isDeepLinkingEnabled ? [isDeepLinkingEnabled boolValue] : NO;


nit: this can be just isDeepLinkingEnabled.boolValue since sending message to nil will return NO.

I will keep this and flip NO to YES in #52350

hellohuanlin · 2024-05-16T23:00:26Z

shell/platform/darwin/ios/framework/Source/FlutterAppDelegate.mm

+ while (!openURLCompleted) {
+ [NSRunLoop.currentRunLoop runUntilDate:[NSDate dateWithTimeIntervalSinceNow:0.1]];
+ }
+ return openURLSuccess;


nit: can you write a helper to avoid duplicate code?

hellohuanlin · 2024-05-16T23:06:09Z

shell/platform/darwin/ios/framework/Source/FlutterAppDelegate.mm

+
+ while (!openURLCompleted) {
+ [NSRunLoop.currentRunLoop runUntilDate:[NSDate dateWithTimeIntervalSinceNow:0.1]];
+ }


Optional: If you are iffy about using BOOL flag for completeness, a semaphore version can be (pseudocode):

__block BOOL openURLSuccess = NO; dispatch_semaphore_t semaphore = dispatch_semaphore_create(0); [self openURL: url ... { openURLSuccess = success; dispatch_semaphore_signal(semaphore) }]; while (dispatch_semaphore_wait(semaphore, DISPATCH_TIME_NOW)) { [NSRunLoop.currentRunLoop runUntilDate:[NSDate dateWithTimeIntervalSinceNow:0.1]]; }

Though a BOOL flag would be equally as good, since callback happens on main thread. I don't have a preference here.

Oh I just saw @jmagman's comment on semaphore.

To use semaphore on the same thread, you can't block it due to deadlock, as jenn previously commented. Instead, you pass in DISPATCH_TIME_NOW in dispatch_semaphore_wait, and then kick off the runloop so it can pick up the completion handler.

This comment was marked as outdated.

Sign in to view

github-actions bot added the platform-ios label May 7, 2024

Update FlutterAppDelegate.mm

c762720

hangyujin force-pushed the 2405RouteMessage branch from 338a4e1 to c762720 Compare May 8, 2024 18:07

Update FlutterAppDelegate.mm

b8d8083

jmagman self-requested a review May 8, 2024 21:09

hangyujin added 5 commits May 8, 2024 17:41

remove completion

bbbb148

lint

50dbd75

Update FlutterAppDelegate.mm

4fec71f

Update FlutterAppDelegate.mm

6af454a

Update FlutterAppDelegate.mm

9b28f1e

hangyujin requested a review from chunhtai May 9, 2024 00:54

hangyujin added 4 commits May 8, 2024 17:57

Update FlutterAppDelegateTest.mm

1dcf138

Update FlutterAppDelegateTest.mm

ca5e4d8

Update FlutterAppDelegateTest.mm

7d00b0e

Update FlutterAppDelegate.mm

62b38db

hangyujin mentioned this pull request May 9, 2024

Set deep linking flag to true by default #52350

Open

8 tasks

hangyujin added 2 commits May 9, 2024 13:20

Update FlutterAppDelegateTest.mm

2c38727

Update FlutterAppDelegateTest.mm

b63243b

chunhtai reviewed May 9, 2024

View reviewed changes

chunhtai approved these changes May 13, 2024

View reviewed changes

jmagman reviewed May 13, 2024

View reviewed changes

hangyujin added 4 commits May 13, 2024 19:24

resolve comments

308fdac

Update FlutterAppDelegate.mm

17949bf

Update FlutterAppDelegate.mm

4ec56ec

Update FlutterAppDelegateTest.mm

77c56fe

hangyujin requested a review from jmagman May 14, 2024 18:26

jmagman requested changes May 14, 2024

View reviewed changes

runloop

7fa8d0c

hangyujin requested a review from jmagman May 14, 2024 23:19

jmagman requested a review from hellohuanlin May 16, 2024 22:54

hellohuanlin reviewed May 16, 2024

View reviewed changes

hangyujin added 3 commits May 16, 2024 16:55

Update FlutterAppDelegate.mm

2befe86

Update FlutterAppDelegate.mm

8c595b7

Update FlutterAppDelegate.mm

785fbc8

hangyujin requested a review from hellohuanlin May 17, 2024 00:35

hangyujin changed the title ~~[ios] Update openURL method to reflect the result from framework~~ [deep link][ios] Update openURL method to reflect the result from framework May 23, 2024

		@@ -149,18 +155,29 @@ - (BOOL)openURL:(NSURL*)url {
		if (didTimeout) {

		// Use a dispatch semaphore to wait for the completion handler
		dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);

[deep link][ios] Update openURL method to reflect the result from framework #52643

Are you sure you want to change the base?

[deep link][ios] Update openURL method to reflect the result from framework #52643

Conversation

hangyujin commented May 7, 2024 • edited

Pre-launch Checklist

This comment was marked as outdated.

hangyujin commented May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmagman May 16, 2024 • edited

Choose a reason for hiding this comment

hellohuanlin May 17, 2024 • edited

Choose a reason for hiding this comment

hellohuanlin May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hangyujin commented May 16, 2024

Choose a reason for hiding this comment

hangyujin May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellohuanlin May 16, 2024 • edited

Choose a reason for hiding this comment

hangyujin commented May 7, 2024 •

edited

hangyujin commented May 8, 2024 •

edited

chunhtai May 13, 2024 •

edited

jmagman May 16, 2024 •

edited

hellohuanlin May 17, 2024 •

edited

hellohuanlin May 17, 2024 •

edited

hangyujin May 16, 2024 •

edited

hellohuanlin May 16, 2024 •

edited