BranchMetrics / ios-branch-deep-linking-attribution

The Branch iOS SDK for deep linking and attribution. Branch helps mobile apps grow with deep links / deeplinks that power paid acquisition and re-engagement campaigns, referral programs, content sharing, deep linked emails, smart banners, custom user onboarding, and more.
https://help.branch.io/developers-hub/docs/ios-sdk-overview
MIT License
728 stars 229 forks source link

Branch SDK v1.42-1.43.2 causes crash on launch for iOS 10-12 even with LinkPresentation optionally linked #1207

Open AlexGingell opened 2 years ago

AlexGingell commented 2 years ago

Branch SDK v1.42 - 1.43 causes crash on launch for iOS 10-12 with:

dyld: Library not loaded: /System/Library/Frameworks/LinkPresentation.framework/LinkPresentation
  Referenced from: /Users/Hiro/Library/Developer/Xcode/DerivedData/REDACTED-fhchuwfdawvhjigaguzjpsyrsory/Build/Products/Debug-iphonesimulator/Branch.framework/Branch
  Reason: image not found
dyld: launch, loading dependent libraries
DYLD_FRAMEWORK_PATH=/Users/Hiro/Library/Developer/Xcode/DerivedData/REDACTED-fhchuwfdawvhjigaguzjpsyrsory/Build/Products/Debug-iphonesimulator
DYLD_FALLBACK_LIBRARY_PATH=/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS 12.4.simruntime/Contents/Resources/RuntimeRoot/usr/lib
DYLD_ROOT_PATH=/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS 12.4.simruntime/Contents/Resources/RuntimeRoot
DYLD_FALLBACK_FRAMEWORK_PATH=/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS 12.4.simruntime/Contents/Resources/RuntimeRoot/System/Library/Frameworks
DYLD_INSERT_LIBRARIES=/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS 12.4.simruntime/Contents/Resources/RuntimeRoot/usr/lib/libBacktraceRecording.dylib:/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS 12.4.simruntime/Contents/Resources/RuntimeRoot/usr/lib/libMainThreadChecker.dylib:/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS 12.4.simruntime/Contents/Resources/RuntimeRoot/Developer/Library/PrivateFrame

Ours is a manual integration. Note that the LinkPresentation framework has been added to Link Binary With Libraries with the 'Optional' status. LinkPresentation is only available from iOS 13.

We note a similar issue here:
https://github.com/fluttercommunity/plus_plugins/issues/222 which was fixed as follows: https://github.com/fluttercommunity/plus_plugins/pull/224

Is it possible the Branch SDK is strongly requiring a framework which should be weakly required because it is not available on iOS 10-12 ?

We've downgraded to v1.41.0 for now - this works fine. We note that 1.44 now requires iOS 11, and have not tested it as we support iOS 10+.

Steps to reproduce

  1. Integrate Branch v1.41 - 1.43
  2. Link the LinkPresentation framework optionally.
  3. Run on simulator or device iOS 10-12.

Expected behavior

The LinkPresentation framework should not be required on iOS 10 - 12 and Branch should launch and operate normally, omitting LinkPresentation functionality gracefully.

SDK Version

1.42.0 - 1.43.2

XCode Version

13.4.1

Device

All

OS

10-12

Additional Information/Context

No response

echo-branch commented 1 year ago

Sorry for the delay. This was a confusing documentation issue. Basically, the framework should be marked optional in xcode.

AlexGingell commented 1 year ago

Sorry for the delay. This was a confusing documentation issue. Basically, the framework should be marked optional in xcode.

As stated in my original post, I did have the LinkPresentation framework marked optional in Xcode's linking options. This does not prevent the crashing issue.

echo-branch commented 1 year ago

@AlexGingell Ah yea, I totally overlooked that detail. So it looks like it's an issue with the pre-built SDK library, as the error does not occur with other integration methods. Creating a ticket for this internally.

LeadAssimilator commented 11 months ago

Why hasn't this been fixed yet? It has been over a year for a 32 character insertion in 3 targets - aka -weak_framework LinkPresentation in other linker flags. This is affecting the xamarin version of the sdk because it uses the binary distributions! Do I need to file a support case for this? I probably will anyway to get a refund for some months given this is a known issue that has been ignored for such a long time.

echo-branch commented 11 months ago

@LeadAssimilator Unfortunately the fix you're suggesting does not work. I also thought it was a simple linker setting, but the original reporter pointed out that they did set the linker setting correctly and it still fails when using the pre-built binary.

On native iOS, the issue can be worked around by using Swift Package Manager or Cocoapods, so the ticket priority had fallen behind iOS 17 related work.

But for Xamarin, we'll likely need to provide a different binary without LinkPresentation.

LeadAssimilator commented 11 months ago

What do you mean it doesn't it work? And why? Because it should and does work!

Adding -weak_framework LinkPresentation to other linker flags for the 3 main targets in the project is no different than s.weak_framework = 'LinkPresentation' in the .podspec. If pods work then so does this fix. I think the original reporter was stating they set the linker flags on their project (as in they weren't strongly linking against LinkPresentation explicitly) which of course won't work to fix branch as it needs set on the Branch project itself given its a dynamic lib!

Just add the flags as described to the branch project and run the build script. You can confirm the framework is linked as weak by inspecting the output of otool -l for each dylib in the resulting xcframework.

otool -l BranchSDK.xcframework/ios-arm64/BranchSDK.framework/BranchSDK | grep -B 3 -A 3 LinkPresentation
otool -l BranchSDK.xcframework/ios-arm64_x86_64-simulator/BranchSDK.framework/BranchSDK | grep -B 3 -A 3 LinkPresentation
otool -l BranchSDK.xcframework/ios-arm64_x86_64-maccatalyst/BranchSDK.framework/BranchSDK | grep -B 3 -A 3 LinkPresentation
otool -l BranchSDK.xcframework/tvos-arm64/BranchSDK.framework/BranchSDK | grep -B 3 -A 3 LinkPresentation
LeadAssimilator commented 11 months ago

Since Xamarin is using 2.1.2, I suggest fixing this in main, backporting the fix to 2.1.2 to make 2.1.3 (even if it just remains internal) and use that build on Xamarin 9.0.0 to make 9.0.1. That way we can get an immediate fix for Xamarin without needing to do any sort of in depth testing/validation since the bits should be identical outside of the linker flags and timestamps.

Clearly this issue was never triaged or prioritized properly if it ended up behind new feature work for something that is so trivial to address. Hopefully you will recognize this error and address the problem appropriately in a timely manner this time.

LeadAssimilator commented 11 months ago

Also I recommend setting Link Frameworks Automatically to No.

Automatically linking frameworks via imports of their headers is not a good idea. I think Apple only made it Yes by default to reduce their support cases and/or to make it seem easier for new/inexperienced developers to make apps. But it ends up hiding a project's dependencies, makes inspection/audit of dependencies harder and encourages one to forget about linking entirely. All of which probably contributed to this particular bug in the first place.

For example, if one had imported LinkPresentation.h with LFA=No, then compile would have initially failed. To make it work an explicit reference to LinkPresentation would have to be added in Frameworks and Libraries. Being forced add the reference manually serves as a reminder to check the framework availability vs. the minimum deployment target and to adjust the linker flags accordingly.

While it isn't perfect, it is better than the alternative, because you can define standard practices/processes/procedures around the adding of new Frameworks and Libraries from ensuring legal/license compliance and entitlements to deployment target availability and linker flags. All of which would only have to apply when adding something new there, rather than having to be something you must remember to do every #import or @import. Nobody wants to have to remember to do that so frequently and most of the time it is useless because you are importing something in use already. Contrast that with leveraging the compiler to remind you extra thought is required shifts the burden to just in time, which is clearly the superior option.

echo-branch commented 11 months ago

@LeadAssimilator Thanks for pushing back! I am indeed mistaken in how deep the initial triage testing was, that's on me. Currently looking into how soon we can meet the overall goal of getting Xamarin updated.