performance issue in every iOS app that uses ADAL

[SergeyADoroshenko]

On WWDC 2016, Apple had a session explaining how iOS apps start and what happens before main() is called. Here is the link on that: https://developer.apple.com/videos/play/wwdc2016/406/. At the end of session, Apple covered the issues that could be the reasons of slow app start up time. They described that before main() function is called, only one thread should be running: the main thread. However, every time ADAL is imported to an iOS app, at least two extra threads are created. The reason for that is the implementation of [ADBrokerHelper load] in ADBrokerHelper.m. As soon as [NSNotificationCenter defaultCenter] is accessed there, extra threads are created.

Expected solution: find another way to execute the callback defined in [NSNotificationCenter defaultCenter] addObserverForName:object:queue:usingBlock: call. Replacing [NSNotificationCenter defaultCenter] addObserverForName:object:queue:usingBlock: with dispatch_async(dispatch_get_main_queue(), <the block>) may work. Right now the [NSNotificationCenter defaultCenter] addObserverForName:object:queue:usingBlock: callback is called after [UIApplicationDelegate application:didFinishLaunchingWithOptions:]. The usage of dispatch_async will keep the call in the same order.

[RPangrle]

That code path is extremely sensitive, as messing up the timing between that code executing and openURL can cause broker messages to get dropped on the floor, breaking broker auth in scenarios where the application was terminated during auth.

[SergeyADoroshenko]

@RPangrle: yes, it's very sensitive. As I understand, the code is trying to swizzle openURL: callback of App Delegate in advance. So, if the app is opened with openURL: call from another app, every call to the openURL: callback, including the first one, can be received by ADAL. Is it correct?

[ogkent]

As discussed a few months ago when we were dealing with a crash, the proper fix is to stop swizzling openURL. I don't know any other SDK that swizzles that call. Facebook, for example, asks the developer to call

[[FBSDKApplicationDelegate sharedInstance] application:application openURL:url sourceApplication:sourceApplication annotation:annotation];

Why doesn't ADAL do something similar?

[SergeyADoroshenko]

What will happen if an iOS app changes its app delegate in runtime? will ADAL stop working?

[Boris-Em]

This issue is really impacting us. As @ogkent is saying, giving the user the responsibility to implement openURL sounds like a sound way of fixing it.

[eddiekim]

AppAuth and Google Sign-In follow the same pattern as FB and ask the client to handle openURL themselves.

[ogkent]

@RPangrle we'd really like some visibility on this issue. Is there a way you can make a fix without changing the interface, so we don't have to wait for a full version bump?

[partnerinflight]

@ogkent to provide profiling info on how/whether this is affecting Outlook. Then will revisit priority/approach.

[ogkent]

@SergeyADoroshenko if you have any data on the perf impact, we'd love to see it.

[sergeyDoroshenko]

@partnerinflight: I don't have access to Outlook codebase to evaluate that now. However, the bug was filed when my former team worked on one of different Office products. But, why should we spend time on re-evaluating this now. It makes sense to do that only if we are uncertain about details. Apple clearly explained in the video (the link is in the first post) that the approach taken here is not correct. I guess we can trust them in that and should just fix the code, shouldn't we?

More than, I carefully looked at the code in ADBrokerHelper again. The code in +(void)load doesn't really do what was planned there. The action performed by ADBrokerHelper is swizzling application:openURL:sourceApplication:annotation: method of UIApplicationDelegate. The correct event for triggering the swizzle action is setting the UIApplication delegate. But ADBrokerHelper uses UIApplicationDidFinishLaunchingNotification event instead. Setting delegate event is not the same as launching the app event. So, technically, I found one more bug in ADBrokerHelper code. According to the documentation at UIApplicationDelegate, the delegate property is not just a getter, it's also a setter. That means an app can change its app delegate on the fly. But if that happens, ADBrokerHelper needs to re-swizzle new app delegate application:openURL:sourceApplication:annotation: method (and definitely un-swizzle the previous one).

Please let me know if the second issue needs to be filed as a separate issue or it will be addressed when the current issue is resolved.

[oldalton]

Thank you for taking time and reporting those 2 issues.

1. I agree that in an unlikely event of application setting a new delegate, current ADAL code won’t handle it, because notification won’t be triggered then. However, it's a very unlikely and not officially supported.
2. To understand performance implications I did some additional profiling. The time is collected using DYLD_PRINT_STATISTICS debug flag that Apple recommends and it’s an average value over 20 runs of iOS test app on iPhone 6s.

With openURL: interception:

Average total time: 76.25ms
Average binding time: 20.00ms
Average ObjC setup: 4.24ms
Average initializer: 30.14ms

Without openURL: interception:

Average total time: 61.93ms
Average binding time: 19.94ms
Average ObjC setup: 3.91ms
Average initializer: 22.8ms

As you see, the mentioned problematic code is costing 7ms of the initializer time. To compare, the average Outlook for iOS initializer time is 93.54ms and average total start time 846ms according to my testing.

In general I believe that it’s worth looking further into this issue, however I think it’s not a critical performance issue.

Possible solutions offered in this thread:

• Don’t intercept openURL:, let developer implement it.

This is a good solution if it was a new library. However, changing it now would require all the developers to implement it when updating and will break a lot of apps for developers.

• Intercept setDelegate: call.

This is a good solution as it would also solve the problem with unlikely event of application dynamically changing delegates. However, this would require also swizzling the setDelegate: method (another swizzle) or relying on KVO of delegate property (unreliable). Both options are far from elegant.

@partnerinflight I believe we have now enough information to discuss all together next steps and priority of this issue.