Resolve Callback Race in ColdBootVisit

[apbendi]

With the change implemented in #45, the WKNavigationDelegate on ColdBootVisit is removed when the WebViewPageLoadDelegate method webView:didLoadPageWithRestorationIdentifier: is called. This callback originates from the WKScriptMessageHandler inside Turbolinks.WebView.

In production, I'm seeing cases where this method can be called before the webView:didFinish: callback of WKNavigationDelegate. As a result, the SessionDelegate sessionDidFinishRequest: method is never called. The behavior is indeterminate. It appears to be a race condition with no guarantee of which will execute first.

Ensuring the delegate is not removed until after the webview finishes loading prevents this callback from getting dropped.

[packagethief]

Thank-you @apbendi! This seems like a reasonable change to me. Would you mind making a PR against the master branch?

@zachwaugh, any objections?

[apbendi]

Thanks @packagethief, after some additional testing with a patched version of our app, we're seeing this issue is not completely resolved. I don't know if the issue we're now seeing is related to this patch or not, so I suggest we hold off on any action with it until I can do some more testing. I'll let you know.

[apbendi]

hey @packagethief, so I've determined why my patch does not seem to resolve the issue completely. While we're no longer removing the delegate prematurely, we are still reassigning it to the session via the default implementation of the sessionDidLoadWebView: method (https://github.com/turbolinks/turbolinks-ios/blob/swift-3.0/Turbolinks/Session.swift#L15).

That method is:

• called by the Session's implementation of visitDidInitializeWebView: method (https://github.com/turbolinks/turbolinks-ios/blob/swift-3.0/Turbolinks/Session.swift#L154)
• which is called by the ColdBootVisit implementation of completeVisit() (https://github.com/turbolinks/turbolinks-ios/blob/swift-3.0/Turbolinks/Session.swift#L154)
• which is called by the complete() method on Visit (https://github.com/turbolinks/turbolinks-ios/blob/swift-3.0/Turbolinks/Visit.swift#L70)
• which is called by ColdBootVisit's implementation of webView:didLoadPageWithRestorationIdentifier: (https://github.com/turbolinks/turbolinks-ios/blob/swift-3.0/Turbolinks/Visit.swift#L236).

Wow.

So, the race condition remains, and if the WKScriptMessageHandler fires before webView:didFinish:, the framework will drop the callback to sessionDidFinishRequest:

While I've identified this issue, I'm not 100% sure what the best way to resolve it is. Again, the order of operations here appears to be indeterminate. So how do we hand off the delegate cleanly, especially when the user might override the default sessionDidLoadWebView: and steal the delegate themselves?

[apbendi]

OK, so I patched this by implementing webView:didFinish navigation inside Session and calling SessionDelegate sessionDidFinishRequest: method there. This seems to work for our app, and we need to get a patch out to our customers ASAP. Here's what that looks like: https://github.com/apbendi/turbolinks-ios/commit/fd10b6e29f298da8a36b42194839eab491d4f6d6

The biggest problem I see with this is that it does not resolve the issue if a developer overrides sessionDidLoadWebView: and steals the delegate themselves. In this case Session is no longer the nav delegate, and they'd have to know to handle this condition.

Also, I worry that this patch might allow sessionDidFinishRequest: to get called twice in some case, although it seems that shouldn't be possible.

Fixing this issue comprehensively might require some bigger refactoring. Happy to hear thoughts @packagethief and @zachwaugh. Thanks!

[apbendi]

friendly ping @packagethief @zachwaugh

[packagethief]

Hey Ben! Thanks for the reminder. I need to think about this one some more.

I'm starting to question the change in #45 because I can't remember why it was necessary. If the order of operations is indeterminate, then it won't work like I intended. Does reverting that change make a difference for your case?

[apbendi]

Thanks @packagethief. Prior to #45, there was an implicit assumption that Turbolinks was installed and working on a cold boot when the web view finished loading. This isn't true, as Turbolinks isn't installed until the webView:didLoadPageWithRestorationIdentifier: callback completes (this originates from JavaScript).

The change in #45 resolved this assumption, but did so by assuming said callback would always occur after the webview webView:didFinish: callback. In reality, these callbacks happen in an indeterminate order, and as a result of #45, if the former occurs before the latter, the application will hang because the navigation delegate has been removed or moved to the session.

So to review:

• There are two callbacks that both need to complete on cold boot for Turbolinks to be considered installed:
  • webView:didLoadPageWithRestorationIdentifier: (from WebViewPageLoadDelegate originating from WKScriptMessageHandler)
  • webView:didFinish: (from WKNavigationDelegate)
  • The order in which these two callbacks will occur is indeterminate
  • Before #45, the SessionDelegate could begin receiving callbacks before Turbolinks was really installed
  • After #45, callbacks that should have gone to the SessionDelegate can be dropped

Seems to me that ColdBootVisit needs to be refactored to wait for both of these callbacks to return before transferring the navigation delegate to the Session and sending the session delegate callbacks.