Fix crash for GTMSessionFetcherService: in case of NSURSession.delegate was swizzled

[gelosi]

Well, basically, the topic.

I've met this problem when I've used TrustKit, which wraps NSURLSessionDelegate with own implementation.

So, I've re-created a test case, which crashes without the fix.

Have a look please if it works for all occasions (at least, tests went fine with me).

Looking forward to your comments.

[gelosi]

@thomasvl Hi there :) I need help with testing the thing, if you have a moment please.
Test are failing sometimes – there's no explicit pre-requirements on env, and I'm not sure where to look for.
testFetcherTestBlockSimulateDataCallbacks - this guy has failed at my latest attempt

[thomasvl]

So I've never looked at TrustKit before, but taking a quick skim of their getting started it seems to say you manually wire in the validation within your own delegate, where does swizzling come into play and what is actually going wrong? It is somehow replacing all the delegates so our delegate is no longer the expected one? i.e. - what exactly fails.

[thomasvl]

Calling it out specifically - swizzling doesn't change the type, it changes the methods on a class, so I'm not following why someone swizzling was causing problems in the existing code and this is needed instead.

[gelosi]

Thanks for finding a time for the MR review.
So, this is the actual problem pinpoint.

My experiments with TSKNSURLSessionDelegateProxy revealed it's pretty well made wrapper for url session delegate.

What is does, in case you creating own session instance – it's wrapping your delegate into that proxy object, and then uses forwardingTargetForSelector: for unknown selectors.

Should work by idea.
But the real thing happens in fetcherDidBeginFetching::

// PROBLEM STARTS HERE: this class is not dispatcher, it's TrustKit wrapper, and, maybe 
// subwrapped if you lucky who's using facebook's sdk as well... whatever
// the reason why it's crashing → GTMSessionFetcher expects there to be either parent 
// GTMSessionFetcher (who's sharing it's session) or GTMSessionFetcherSessionDelegateDispatcher

    GTMSessionFetcherSessionDelegateDispatcher *delegateDispatcher =
          [self delegateDispatcherForFetcher:fetcher];

// Sanity check that the fetcher's session is the delegate's shared session.
    NSURLSession *sharedSession = delegateDispatcher.session;
    NSURLSession *fetcherSession = fetcher.session;

    // so, we're asked for NSURLSession → and we got it, but clean from any GTM*** code! 
    if (sharedSession != nil && fetcherSession == sharedSession) {
       /* some code here */
       NSURLSessionTask *task = fetcher.sessionTask;
      if (task) {
        // CRASH AT THE NEXT LINE:
        [delegateDispatcher setFetcher:fetcher
                               forTask:task];
      }
    }

because of setFetcher:forTask: call is sent now to the instance of TrustKit swizzled object, which will try to forward it to the wrapped object, and will fail → Crash.

[thomasvl]

because of setFetcher:forTask: call is sent now to the instance of TrustKit swizzled object, which will try to forward it to the wrapped object, and will fail → Crash.

I'm still failing to see how what you describe has any swizzling. You seemed to have described using a proxy delegate that forwards, but that doesn't require any swizzling.

And if they are just providing a proxy that forwards, isn't the -isKindOfClass: going to stop us from connecting things up? i.e. - the proxy would come back false for that so we wouldn't return the delegate to have things relayed through.

[bryce-buchanan]

@thomasvl This ticket seems a little stale, but I've run into this issue as well with the New Relic iOS framework. We also swizzle NSURLSession and override the delegate object to inject a proxy. If I'm understanding this method correctly, BOOL hasDispatcher = (fetcherDelegate != nil && fetcherDelegate != fetcher);(GTMSessionFetcherService.m:384) is asserting if fetchDelegate is not the fetcher, then it must be a GTMSessionFetcherSessionDelegateDispatcher. However, when a framework which instruments NSURLSession is introduced, proxying the delegate, this assumption is invalid. This results in the method -setFetcher:forTask potentially being called on a GTMSessionFetcher object.

This is the issue I am currently experiencing. The proxied object is, in actuality, the fetcher, but because the check only compares the memory address, the memory address of fetcherDelegate will never match the memory address of fetcher. This results in the proxy object wrapping fetcher to be treated like a GTMSessionFetcherSessionDelegateDispatcher object. This results in -setFetcher:forTask to be called on the GTMSessionFetcher object.

I don't have full context, but using an 'isKindOfClass' maybe a better solution than a direct address check, unless the concern is there may be multiple unique instances of the GTMSessionFetcher object, but, if this was the case the logic flow would result in the same problem.

I can't speak for geliso, but our proxy object is smart enough to verify 'isKindOfClass' against the wrapped delegate, and forward methods to that proxied object. So doing a 'isKindOfClass' check would perform as expected.

[thomasvl]

What you describe makes some sense. But this PR should break things then, as for the setup you describe, the new maybeHasDispatcher would be False and we'd then hit the return nil case, which doesn't seem what would be desired.

So it sounds like the best bet might be to just do the class check hoping proxy code "gets it right", and that's it.

@mwyman does that sound reasonable to you?

[gelosi]

@thomasvl @mwyman looks like it is exactly desired behaviour in this case. So, swizzled delegate won't be used as gtm session internals.

[mwyman]

I think the class check in place of the pointer value check sounds reasonable.

[eyalevy]

@thomasvl @gelosi
Hi,
I have the same issue with TrustKit. I wrote example app that reproduce it: https://github.com/eyalevy/trustkit-firebase-sample.
When I add this PR it fix this issue.
So when can I expect this PR to be merged?
Thanks

[thomasvl]

So when can I expect this PR to be merged?

There are still open questions about the change, as the descriptions seem to imply it isn't really swizzling, but something hooking out the delegate with a proxy. But exactly why it fails isn't clear either since if it was forwarding things to the real class they would work, so it doesn't seem right to accept this patch that could be disabling the calls to the delegate without a proper understanding of why they were failing in the first place.

[gelosi]

@thomasvl agree. I need to build a nice proof on this change. Please, don't kill MR yet.

[gelosi]

@thomasvl and yes, it will take it's time. But issue is there 'cause this code does not check the class of the dispatcher delegate, assuming no one else can be a delegate of URLSession [especially, set itself as a delegate trough init method swizzling, as done by TrustKit for instance]

[mwyman]

I'm in agreement with @thomasvl that this PR has not made clear what it's fixing and why disabling delegate calls is a proper solution. If TrustKit expects to replace internal behavior of other code (via swizzling or whatever), it's their responsibility to properly proxy the delegate, which it sounds like isn't happening; I'm not sure it's reasonable for every networking library to need to guard against that.

[thomasvl]

Can folks hitting this see if #142 fixes it for them?

[thomasvl]

Hopefully #142 address the issues this was attempting to fix.