1397799 - Invalid deep links crash the application.

Status: RESOLVED FIXED

(Firefox for iOS :: General, enhancement, P1)

Description

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Invalid deep links crash the application.

Comment 1

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

For example firefox://deep-link?url=cheese will crash the application.

Comment 2

[James Hugman [:jhugman] [@jhugman]]

Attachment: Link to Github pull-request: https://github.com/jhugman/SwiftRouter/pull/1

Comment 3

[James Hugman [:jhugman] [@jhugman]]

Attachment: Link to Github pull-request to SwiftRouter project.

This PR is sent upstream. We should probably review it, if nothing else, to make it easier to accept.

Comment 4

[James Hugman [:jhugman] [@jhugman]]

Attachment: Link to Github pull-request: https://github.com/mozilla-mobile/firefox-ios/pull/3153

Comment 5

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8906672 [details] [review]
Link to Github pull-request: https://github.com/jhugman/SwiftRouter/pull/1

LGTM

Comment 6

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8906676 [details] [review]
Link to Github pull-request: https://github.com/mozilla-mobile/firefox-ios/pull/3153

Looks good, just needs the update to `Cartfile.resolved` if this lands today. Was this crash verified? We don't get undefined behaviour by returning `nil` ?

Comment 7

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8906674 [details] [review]
Link to Github pull-request to SwiftRouter project.

James, do you need a review on this PR too? It looks like this one is for the upstream SwiftRouter repo.

Comment 8

[James Hugman [:jhugman] [@jhugman]]

Yes please. I'd like to make this easy for the upstream project to accept the PR. An extra set of our eyes might help with that.

Comment 9

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8906674 [details] [review]
Link to Github pull-request to SwiftRouter project.

Was this tested against our actual use case via Leanplum? I'm sending this branch the firefox://deep-link?url=settings link, and the app crashes. I'm pretty sure this is exactly the same spot where it crashed with the original SwiftRouter version.

Crash at https://github.com/jhugman/SwiftRouter/blob/master/Source/SwiftRouter.swift#L179

Comment 10

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BREAKPOINT (code=1, subcode=0x10362464c)
  * frame #0: 0x000000010362464c SwiftRouter`specialized Router.findRouteEntry(params=0x000000016f718b88, self=<unavailable>) at SwiftRouter.swift:179 [opt]
    frame #1: 0x000000010361c2d4 SwiftRouter`Router.routeURL(_:) [inlined] SwiftRouter.Router.(findRouteEntry in _3D4E6DC05F6DF78AA760E8AA3FC07D1F)(Swift.String, params: inout Swift.Dictionary<Swift.String, Swift.String>) -> Swift.Optional<SwiftRouter.RouteEntry> at SwiftRouter.swift:0 [opt]
    frame #2: 0x000000010361c2c0 SwiftRouter`Router.routeURL(_:) at SwiftRouter.swift:163 [opt]
    frame #3: 0x000000010361c2c0 SwiftRouter`Router.routeURL(_:) [inlined] SwiftRouter.Router.matchHandler(Swift.String) -> Swift.Optional<(Swift.Optional<Swift.Dictionary<Swift.String, Swift.String>>) -> Swift.Bool> at SwiftRouter.swift:161 [opt]
    frame #4: 0x000000010361c2c0 SwiftRouter`Router.routeURL(route="/settings", self=0x00000001c0242370) at SwiftRouter.swift:241 [opt]
    frame #5: 0x000000010091b1c8 Client`AppDelegate.application(application=0x0000000104504960, url="firefox://deep-link?url=/settings", sourceApplication="org.mozilla.ios.Fennec", annotation=Any @ 0x000000016f719890, self=0x00000001c01425d0) at AppDelegate.swift:401
    frame #6: 0x000000010091c108 Client`@objc AppDelegate.application(_:open:sourceApplication:annotation:) at AppDelegate.swift:0
    frame #7: 0x000000018a44233c UIKit`__58-[UIApplication _applicationOpenURLAction:payload:origin:]_block_invoke + 872
    frame #8: 0x000000018a441ca8 UIKit`-[UIApplication _applicationOpenURLAction:payload:origin:] + 640
    frame #9: 0x000000018a44b618 UIKit`-[UIApplication _handleNonLaunchSpecificActions:forScene:withTransitionContext:completion:] + 4464
    frame #10: 0x000000018ae05abc UIKit`__82-[_UIApplicationCanvas _transitionLifecycleStateWithTransitionContext:completion:]_block_invoke + 204
    frame #11: 0x000000018ae05998 UIKit`-[_UIApplicationCanvas _transitionLifecycleStateWithTransitionContext:completion:] + 448
    frame #12: 0x000000018ab8fab4 UIKit`__125-[_UICanvasLifecycleSettingsDiffAction performActionsForCanvas:withUpdatedScene:settingsDiff:fromSettings:transitionContext:]_block_invoke + 220
    frame #13: 0x000000018ad1fc94 UIKit`_performActionsWithDelayForTransitionContext + 112
    frame #14: 0x000000018ab8f964 UIKit`-[_UICanvasLifecycleSettingsDiffAction performActionsForCanvas:withUpdatedScene:settingsDiff:fromSettings:transitionContext:] + 252
    frame #15: 0x000000018a980730 UIKit`-[_UICanvas scene:didUpdateWithDiff:transitionContext:completion:] + 364
    frame #16: 0x000000018a828630 UIKit`-[UIApplicationSceneClientAgent scene:handleEvent:withCompletion:] + 468
    frame #17: 0x00000001834d641c FrontBoardServices`__80-[FBSSceneImpl updater:didUpdateSettings:withDiff:transitionContext:completion:]_block_invoke.362 + 212
    frame #18: 0x000000010427545c libdispatch.dylib`_dispatch_client_callout + 16
    frame #19: 0x0000000104281b74 libdispatch.dylib`_dispatch_block_invoke_direct + 268
    frame #20: 0x0000000183509b04 FrontBoardServices`__FBSSERIALQUEUE_IS_CALLING_OUT_TO_A_BLOCK__ + 36
    frame #21: 0x00000001835097a8 FrontBoardServices`-[FBSSerialQueue _performNext] + 404
    frame #22: 0x0000000183509d44 FrontBoardServices`-[FBSSerialQueue _performNextFromRunLoopSource] + 56
    frame #23: 0x0000000180de8358 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 24
    frame #24: 0x0000000180de82d8 CoreFoundation`__CFRunLoopDoSource0 + 88
    frame #25: 0x0000000180de7b60 CoreFoundation`__CFRunLoopDoSources0 + 204
    frame #26: 0x0000000180de5738 CoreFoundation`__CFRunLoopRun + 1048
    frame #27: 0x0000000180d062d8 CoreFoundation`CFRunLoopRunSpecific + 436
    frame #28: 0x0000000182b8bf84 GraphicsServices`GSEventRunModal + 100
    frame #29: 0x000000018a2375e0 UIKit`UIApplicationMain + 208
    frame #30: 0x0000000100aa1ba0 Client`main at main.swift:16
    frame #31: 0x000000018082a56c libdyld.dylib`start + 4

(lldb) po d["_entry"]
nil

Comment 11

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Landed on master, uplifted to v9.x

This bug is going to need a followup bug. Either to track if our PR is going to be accepted to SwiftRouter, or to point to a more official fork of the project if they don't take our patch.

Comment 12

[James Hugman [:jhugman] [@jhugman]]

Followup at Bug 1399083.

Removing needinfo