session.reload() will sometimes open in Mobile Safari instead

[ghost]

We're seeing what appears to be a timing issue, where if we trigger session.reload() at the wrong time, it will open the page in Mobile Safari instead of in the app.

We've seen it in our own app by triggering session.reload() on tab changes and then changing tabs quickly. We managed to come up with a repro in the demo app, but it's not for the faint-fingered. We added a nav bar button that calls session.reload(). Then we pressed it a lot and managed to reproduce the issue:

Here is the diff:

diff --git a/TurbolinksDemo/ApplicationController.swift b/TurbolinksDemo/ApplicationController.swift
index 668bab1..0c607ca 100644
--- a/TurbolinksDemo/ApplicationController.swift
+++ b/TurbolinksDemo/ApplicationController.swift
@@ -36,6 +36,8 @@ class ApplicationController: UINavigationController {
     fileprivate func presentVisitableForSession(_ session: Session, url: URL, action: Action = .Advance) {
         let visitable = DemoViewController(url: url)

+        visitable.navigationItem.rightBarButtonItem = UIBarButtonItem(barButtonSystemItem: .add, target: self, action: #selector(reload))
+
         if action == .Advance {
             pushViewController(visitable, animated: true)
         } else if action == .Replace {
@@ -46,6 +48,10 @@ class ApplicationController: UINavigationController {
         session.visit(visitable)
     }

+    @objc func reload() {
+        session.reload()
+    }
+
     fileprivate func presentNumbersViewController() {
         let viewController = NumbersViewController()
         pushViewController(viewController, animated: true)

Our impression is that this seems to happen if we reload right at the start of the page rendering, but we're not at all sure.

[henrik]

(I'm part of @auctionet-mob.) We tried reproducing it by programmatically calling session.reload() a bunch of times, with or without delays, but couldn't. Perhaps we didn't manage to time it just right/wrong. That's why we ended up with this fairly manual repro.

[samoli]

I reproduced this bug in iOS 12.1 simulator on the current master branch (8b0ff21)

It took a lot of effort. I clicked manually hundreds of times, tried using automator to click about 1000 times. I was about to give up, pressed the button a few more times and Safari finally opened.

[kaifucious]

I notice this issue as well, instead of loading a url in a WKWebView, Turbolinks instead hands its off to Safari. Is this still an ongoing issue?

Xcode 10, iOS 12.1.3

[henrik]

It's still an ongoing issue – I've seen it in the wild recently on Turbolinks 5.2.0, Xcode 10.2, iOS 12.2. Just tried in our app on my phone and I was able to trigger it in a matter of seconds of going back and forth between self-reloading tabs.

[calleluks]

I investigated this further today and think I might have found why this is happening. I wasn't able to fully solve it but hope that my findings can help or inspire someone else who wants to give this a try.

When doing two session.reload()s in quick succession, it appears as if the second ColdBootVisit sometimes gets a webView(_:didLoadPageWithRestorationIdentifier:) message for the first ColdBootVisit before it receives the webView(_:decidePolicyFor:decisionHandler:) message that is related to its own call to webView.load(_:).

When webView(_:didLoadPageWithRestorationIdentifier:) is called for ColdBootVisit, the default implementation of SessionDelegate.sessionDidLoadWebView(_:) is called.

This method will then replace the ColdBootVisit with the Session as the webViews navigationDelegate.

When the webView(_:decidePolicyFor:decisionHandler:) method then is called on Session instead of the ColdBootVisit for the navigation caused by calling webView.load(_:), the navigationAction passed to it is a main frame navigation with navigationType == .other.

This causes the Session to call the default implementation of session(_:openExternalURL:) which in turn calls UIApplication.openURL(_) to open the URL in Safari.

Here's the order of the events I observed:

1. session.reload() is called. It will start a ColdBootVisit which then sets itself as both navigationDelegate and pageLoadDelegate of the webView.
2. The webView finishes loading the page but has yet to execute the WebView.js user script. This means that the page loaded event has not yet been sent and the webView(_:didLoadPageWithRestorationIdentifier:) has not been called for the ColdBootVisit.
3. session.reload() is called again calling cancel on the existing ColdBootVisit. Since the webView has already finished loading the page, calling webView.stopLoading() does nothing. Then another ColdBootVisit is started. This ColdBootVisit will replace the existing ColdBootVisit as pageLoadDelegate just in time to receive the webView(_:didLoadPageWithRestorationIdentifier:) message, triggering the calls to make Session the new navigationDelegate.
4. The new ColdBootVisit calls webView.load(_:) and the webView calls webView(_:decidePolicyFor:decisionHandler:) on its navigationDelegate which happens to be the Session. Since navigationAction.navigationType == .other and the navigation is targeted at the main frame, Session (through its delegate) calls UIApplication.openURL.

I'm not sure how to best solve this but one idea would be to have calls to webView(_:didLoadPageWithRestorationIdentifier:) also pass back an identifier that the visit can compare to an identifier of its own to decide whether the page load event was related to its call to webView.load(_:). If not, it could simply ignore the call.

I tried to implement this solution but couldn't find a way to pass an identifier when starting a visit that WebView.js could then pass back when the page loaded.

[petrsigut]

@calleerlandsson we are seeing this issue as well in our app, but never happens in the Basecamp app I am using daily. I would love to hear any thoughts on how to mitigate this!

[zachwaugh]

Sorry, I could have swore I responded to this thread before.

We've never seen it in Basecamp, and as far as I know, no reports of it. There are only two places Turbolinks opens a link in Safari, and it would be helpful to know which of these is being triggered here:

• turbolinks-ios/Turbolinks/Session.swift

  Line 277 in 0cdd4bd

  openExternalURL(URL)

• turbolinks-ios/Turbolinks/Visit.swift

  Line 186 in 8b0ff21

  UIApplication.shared.openURL(URL)

I'm curious for the people that are experiencing this issue, do you have your own WKNavigationDelegate set? In Basecamp, on sessionDidLoadWebView(), we set our own object to be the WKNavigationDelegate. That object handles a bunch of cases and passes everything through our router. That means if something did get through to that method that should have gone through Turbolinks, our router would still handle it correctly, which may be why we haven't seen this issue.

As a workaround, you could try implementing the Session delegate's openExternalURL method which is called here -

turbolinks-ios/Turbolinks/Session.swift

Line 314 in 0cdd4bd

delegate?.session(self, openExternalURL: URL)

, and check the domain of the url before deciding whether to open it in Safari or not.

[henrik]

Thank you very much, @zachwaugh! Next time @calleerlandsson and I work on our Turbolinks app we'll try to delve into this again and report back.

[tony42]

Just a note here. If what @calleerlandsson reported is correct, the workaround suggested by @zachwaugh will not work. As it looks like it's the Visit.swift UIApplication.shared.openURL(URL) which gets called. We also have this issue and we do use our own WKNavigationDelegate.

[Bramjetten]

We're experiencing this issue as well. I followed @zachwaugh 's suggestion and implemented the openExternalURL method that now checks if the host is actually an external host or not. Looks like it's working (but not sure because I find this bug hard to reproduce)

As an aside: as a Rails developer (and novice iOS developer at best) I'd pay a good amount to take a look at a more elaborate example app (like Basecamp's app). The included example in turbolinks-ios is very helpful, but it's also really bare-bones. It'd really help me to have an example with more use cases for typical Rails apps.

[calleluks]

@zachwaugh, yes we are also setting our own WKNavigationDelegate in sessionDidLoadWebView(_:). Unfortunately, this is not called in this situation, since the call happens at a point when the session has set itself as the web view's navigation delegate, before sessionDidLoadWebView(_:) is called.

I can confirm that it is the SessionDelegate's default implementation of session(_:, openExternalURL:) that triggers the call to UIApplication.shared.openURL. I think this means we should be able to avoid URLs opening in Safari by ignoring calls to session(_:, openExternalURL:) when the URL is on our own domain. We'll try that!

Do you think the root cause of this issue will be resolved in an upcoming version of Turbolinks iOS?

[henrik]

I work with @calleerlandsson – the fix we tried was

func session(_ session: Session, openExternalURL URL: URL) {
    // This attempts to work around a bug in Turbolinks that causes session.reload() to sometimes open a page in Safari. For more info on this, see this issue: https://github.com/turbolinks/turbolinks-ios/issues/121
    if MyAppURLChecker.isMyAppURL(URL) {  // <- Replace this, obviously…
        assertionFailure("This URL should not be opened in Safari")
        return
    }

    UIApplication.shared.open(URL)
}

We could reproduce the issue just prior to the fix, and after adding the fix we could trigger the assertion failure (which would just be a silent return in production, of course) – so hopefully this successfully worked around the issue.

UPDATE:

One fairly big gotcha with this solution is that it also applies if you ever do <a href="https://myapp.com" target="_blank">I want this link to open in Mobile Safari</a> – meaning clicking that link would cause a crash in production. One workaround for that would be to never use target=_blank like that, making sure to e.g. open modals instead. But we ended up reverting this fix – new plan is to try to reduce the risk of the original bug by rewriting some code we have that currently reloads the page when we swich to that tab. That should dramatically reduce how often the bug happens.