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

[hannah-hyj]

follow up on comments on #52350

framework pr : flutter/flutter#147901

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.

[hannah-hyj]

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]

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

[hannah-hyj]

Can you explain why we want to do that?

[chunhtai]

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

[hannah-hyj]

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.

[chunhtai]

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

[chunhtai]

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

[hannah-hyj]

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

[chunhtai]

can this method get the result from completion handler only?

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

[hannah-hyj]

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.

[jmagman]

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.

[hannah-hyj]

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]

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

[jmagman]

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

[chunhtai]

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.

[chunhtai]

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

[jmagman]

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);
                             }];
                 }
               }];
}

[chunhtai]

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

[hannah-hyj]

thanks, this looks good, updated!

[hannah-hyj]

@jmagman ping for review

[hellohuanlin]

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

[hannah-hyj]

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

[hellohuanlin]

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

[hellohuanlin]

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.

[hellohuanlin]

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.

[zanderso]

From PR triage: What is the status of this PR? @Hangyujin is this ready for another review pass from @hellohuanlin?

[hellohuanlin]

From PR triage: What is the status of this PR? @Hangyujin is this ready for another review pass from @hellohuanlin?

There's a deadlock issue that I am looking into and will unblock @Hangyujin when it's done.

[chinmaygarde]

Can we close this issue in the meantime?

[hellohuanlin]

follow up from meeting - can delete this.

[hannah-hyj]

Some notes from the offline meeting:

*Return value: it seems like returning no or yes in application:continueUserActivity:restorationHandler does not change its behavior, but we can throw back the url to iOS if the framework returns NO.

*Custom URL scheme:
Opening a URL with custom schemes calls application:openURL:options:, we won’t throw it back to iOS for custom URL because it will cause a inf loop.

*Add to app :
Looked up some examples under add_to_app code samples, (Example) they are not using FlutterAppDelegate, so this change won’t impact the add to app deep linking use cases.

[hellohuanlin]

LGTM after nit

[hellohuanlin]

@jmagman This PR is pending on your review now, could you take a look when you have time?

Jenn is OOO this week. Feel free to land it.

Jenn's OOO + I did a pretty detailed review

[jmagman]

Thank you so much for working on this @Hangyujin and for helping out @hellohuanlin, it got much trickier than I expected!

[hannah-hyj]

@jmagman Thank you!

[ma-pe]

fyi: I just stumbled upon an error that is related to this PR. No idea yet wether this is an issue with flutter or with auto_route (or with me 😄).

[hellohuanlin]

fyi: I just stumbled upon an error that is related to this PR. No idea yet wether this is an issue with flutter or with auto_route (or with me 😄).

Feel free to create an issue when more details.

[junka-0]

I encountered similar issue with Milad-Akarie/auto_route_library#2055.
I'm using flutter 3.24.2
but I'm using go_router 14.3.0.

Expected behaviour:
Click on universal link opens the app and the appropriate page.

Observed behaviour:
<When FlutterDeepLinkingEnabled is true>
Click on universal link opens the app. Quickly after that the native browser is opened with the link as target. In my logs I see a warning "Failed to handle route information in Flutter.".

<When FlutterDeepLinkingEnabled is false>
Click on universal link always opens first page of the app. But never navigate to other pages.

Steps to reproduce
<When FlutterDeepLinkingEnabled is true>

1. Click on universal link opens the app correctly.
2. Put the app to background. (Do not kill the app.)
3. Click on the same universal link opens the app. Quickly after that the native browser is opened with the link as target. In my logs I see a warning "Failed to handle route information in Flutter.".
4. Kill the app.
5. Click on the same universal link opens the app correctly.
   　　

<When FlutterDeepLinkingEnabled is false>

1. Click on universal link always opens first page of the app. But never navigate to other pages.

[hannah-hyj]

@junka-0 Thank you, the information you provided is very beneficial!

In flutter code

1. Framework should handle the route and return a boolean to indicate if the route is successfully handled with Future<bool> _handlePushRouteInformation
2. If framework doesn’t return success, engine will throw the link back to iOS(That why browser is opened) and print the error message "Failed to handle route information in Flutter.” that you observed

Looks like what happened might be when you click on the same universal link the second time, framework returns false for _handlePushRouteInformation

Not sure why it worked the first time clicking the universal link but it failed the second time, this might be a framework issue or a go_router issue or some issues in your project, let me investigate further.

[hannah-hyj]

So I tested with a simple go_router example, and a misconfigured go_router example.

The former one works well, but the latter one has the issue that it jumps to the browser again and again. 

If you have some misconfiguration in your app and the routing is not handled correctly and the framework returns false in function _handlePushRouteInformation. This PR (#52643) will escalate it by throwing it to the browser as a fallback.

But the best way to know if the issue is on the flutter side or the app side is if you can raise a separate issue with a minimal repo that can reproduce the issue, we can see where the error is.

example 1 with go_router

import 'package:flutter/material.dart';
import 'package:go_router/go_router.dart';

GlobalKey<NavigatorState> rootNavigatorKey = GlobalKey<NavigatorState>();

final router = GoRouter(
    debugLogDiagnostics: true,
    navigatorKey: rootNavigatorKey,
    initialLocation: "/",
    routes: [
      GoRoute(path: '/path1', builder: (context, state) => const PageOne()),
      GoRoute(path: '/2', builder: (context, state) => const PageTwo()),
      GoRoute(path: '/', builder: (context, state) => const PageTwo()),
    ]);

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  // This widget is the root of your application.
  @override
  Widget build(BuildContext context) {
    return MaterialApp.router(
      title: 'Flutter Demo',
      theme: ThemeData(
        colorScheme: ColorScheme.fromSeed(seedColor: Colors.deepPurple),
        useMaterial3: true,
      ),
      routerConfig: router,
    );
  }
}

class PageOne extends StatelessWidget {
  const PageOne({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: [
            const Text('PageOne'),
            const SizedBox(
              height: 30,
            ),
            ElevatedButton(
                onPressed: () {
                  context.go('/2');
                },
                child: const Text('Go To PageTwo'))
          ],
        ),
      ),
    );
  }
}

class PageTwo extends StatelessWidget {
  const PageTwo({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: [
            const Text('Page Two'),
            const SizedBox(
              height: 30,
            ),
            ElevatedButton(
                onPressed: () {
                  context.go('/');
                },
                child: const Text('Back to PageOne'))
          ],
        ),
      ),
    );
  }
}

example 2 with go_router and it's slightly misconfigured

import 'package:flutter/material.dart';
import 'package:go_router/go_router.dart';

GlobalKey<NavigatorState> rootNavigatorKey = GlobalKey<NavigatorState>();

final router = GoRouter(
    debugLogDiagnostics: true,
    navigatorKey: rootNavigatorKey,
    initialLocation: "/",
    routes: [
      GoRoute(path: '/path1', builder: (context, state) => const PageOne()),
      GoRoute(path: '/2', builder: (context, state) => const PageTwo()),
      GoRoute(path: '/', builder: (context, state) => const PageTwo()),
    ]);

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  // This widget is the root of your application.
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
        title: 'Flutter Demo',
        theme: ThemeData(
          colorScheme: ColorScheme.fromSeed(seedColor: Colors.deepPurple),
          useMaterial3: true,
        ),
        home: MaterialApp.router(
          routerConfig: router,
        ));
  }
}

class PageOne extends StatelessWidget {
  const PageOne({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: [
            const Text('PageOne'),
            const SizedBox(
              height: 30,
            ),
            ElevatedButton(
                onPressed: () {
                  context.go('/2');
                },
                child: const Text('Go To PageTwo'))
          ],
        ),
      ),
    );
  }
}

class PageTwo extends StatelessWidget {
  const PageTwo({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: [
            const Text('Page Two'),
            const SizedBox(
              height: 30,
            ),
            ElevatedButton(
                onPressed: () {
                  context.go('/');
                },
                child: const Text('Back to PageOne'))
          ],
        ),
      ),
    );
  }
}

[junka-0]

@hannah-hyj
I found cause of bug is in other package.
Thank you for investigating.

[hannah-hyj]

@junka-0
Thanks! Could you share what package it is or what type of error it is? This might help someone with a similar problem in the future.

[junka-0]

@hannah-hyj
When I use loader_overlay in deep link destination page, the bug I reported occurs.
If I remove loader_overlay in the page, the bug does not occur.

[hellohuanlin]

I highly recommend moving the conversation to a github issue as a closed PR isn't great for issue discussion.