Handle UISceneDelegate changes in Auth #3925

renkelvin · 2019-09-25T18:45:54Z

No description provided.

paulb777

Firestore still needs GoogleUtilities to build on Xcode 8, so @available checks need to be wrapped in #if __has_builtin(__builtin_available) like https://github.com/firebase/firebase-ios-sdk/blob/master/GoogleUtilities/SecureCoding/GULSecureCoding.m#L55

maksymmalyhin

@renkelvin Thanks a lot for working on it!

There are couple comments from my side to improve the implementation. Also, I feel like this code really needs unit tests (at least basic ones).

GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m

maksymmalyhin · 2019-10-15T18:37:25Z

GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m

+                             [sceneDelegateIMPDict copy], OBJC_ASSOCIATION_RETAIN);
+
+    BOOL didAddMethod =
+    class_addMethod(sceneDelegateClass,


As far as I can see, the method swizzling has been implemented here. ISA swizzling is used to proxy App Delegate which is a preferable way (method swizzling will affect any object of the class in contrast to ISA swizzling which will affect only a particular instance, so the impact on the host app will be smaller as well as risks of issues).

I think, GULObjectSwizzler can be used here to create a subclass for the object. Then proxyDestinationSelector... method may be used to update the method implementation.

Feel free to reach out to me to clarify the details.

Please see the updated pr. Seems works well. I'll add unit tests later.

Can unit tests be added before this lands? It seems like a pretty significant change that we should be testing before publishing.

Yeah, I also would feel more comfortable with the changes having tests in place.

I mean this pr is more of a draft. The solution is not finalized yet.

Ah, sure, no problem

ryanwilson · 2019-10-17T14:22:22Z

GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m

        [GULAppDelegateSwizzler sharedApplication].delegate;
    [GULAppDelegateSwizzler proxyAppDelegate:originalDelegate];
+
+#if __IPHONE_OS_VERSION_MAX_ALLOWED >= 130000


We've determined that this won't work in our zip file. @paulb777 do we have a better approach, or do we need to start packaging with Xcode 11 (either in addition to Xcode 10.1, or as a replacement)?

What's the current status of this?

GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m

ryanwilson · 2019-10-17T14:40:09Z

GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m

+                @"Cannot create subclass of App Delegate, because the created subclass is not the "
+                @"same size. %@",
+                NSStringFromClass(realClass));
+    NSAssert(NO, @"Classes must be the same size to swizzle isa");


Can we avoid asserting here? Although functionality will be broken, we shouldn't be asserting and crashing developers' apps.

@maksymmalyhin Asserting seems used a lot for AppDelegate, what do you think?

ryanwilson · 2019-10-17T14:40:49Z

GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m

+                [GULAppDelegateSwizzler correctAppDelegateProxyKey]);
+  }
+
+  // TODO: reassign delegate as app delegate above?


Is there something that needs to be done here still? If so, please file an issue an update the comment.

Yes, it's required for AppDelegate, while we need discuss more to see if it's needed for SceneDelegate.
See:

firebase-ios-sdk/GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m

Line 545 in 2ebb82d

/// We have to do this to invalidate the cache that caches the original respondsToSelector of

Removed it for now. I didn't see any issue without reassigning.

ryanwilson · 2019-10-17T14:41:37Z

GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m

+                             [sceneDelegateIMPDict copy], OBJC_ASSOCIATION_RETAIN);
+
+    BOOL didAddMethod =
+    class_addMethod(sceneDelegateClass,


Can unit tests be added before this lands? It seems like a pretty significant change that we should be testing before publishing.

ryanwilson

Please run style.sh as well.

maksymmalyhin · 2019-10-17T16:27:16Z

GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m

+
+  // Register the new class as subclass of the real one. Do not allocate more than the real class
+  // size.
+  Class sceneDelegateSubClass = objc_allocateClassPair(realClass, newClassName.UTF8String, 0);


Looks like we don't have a protection from multiple swizzling of the same object (not sure though how often it may happen, but it looks like a possible case). I think, we do our best guess based on the current object class name prefix - if it is equal to classNameWithPrefix then skip proxying.

Yeah, it's a good point. If developer's app uses a prefix of "GUL_" like us, then there would be collision issue any way.

1. Reuse `kGULRealIMPBySelectorKey` dict to retrieve the original IMP. Remove the `kGULSceneDelegateIMPDictKey` and associated dict. 2. Check the prefix of scene delegate class name. If it’s “GUL_”, then skip proxying to avoid proxying for more than once.

Initial changes

8cf2ebe

renkelvin requested review from maksymmalyhin, paulb777 and ryanwilson September 25, 2019 18:45

googlebot added the cla: yes label Sep 25, 2019

paulb777 reviewed Sep 25, 2019

View reviewed changes

renkelvin added 3 commits October 9, 2019 16:37

Use UISceneWillConnectNotification

716f6d3

Add __has_builtin notation

bf8fdb5

Guad isAppDelegateProxyEnabled

15bb043

renkelvin marked this pull request as ready for review October 10, 2019 00:31

maksymmalyhin reviewed Oct 15, 2019

View reviewed changes

Use ISA swizzling

4d33542

ryanwilson changed the title ~~Initial changes~~ Handle UISceneDelegate changes in Auth Oct 17, 2019

ryanwilson reviewed Oct 17, 2019

View reviewed changes

maksymmalyhin reviewed Oct 17, 2019

View reviewed changes

morganchen12 added the api: auth label Oct 28, 2019

renkelvin added 2 commits November 5, 2019 11:52

Format

c5b9861

Multiple fixes

d0a3267

1. Reuse `kGULRealIMPBySelectorKey` dict to retrieve the original IMP. Remove the `kGULSceneDelegateIMPDictKey` and associated dict. 2. Check the prefix of scene delegate class name. If it’s “GUL_”, then skip proxying to avoid proxying for more than once.

google-oss-bot added the GoogleUtilities label Nov 6, 2019

renkelvin added 6 commits November 5, 2019 17:42

Format

b6169b0

Firebase Auth can’t handle URL on tvOS

940673a

Add unit tests

555a550

Fix for 10.3

c541bc0

Format

44008b8

Fix GULUltilities building issue

0bf36bd

renkelvin mentioned this pull request Nov 22, 2019

Handle UISceneDelegate changes in Auth #4380

Merged

renkelvin closed this Dec 18, 2019

renkelvin deleted the swift-ui branch December 18, 2019 18:54

firebase locked and limited conversation to collaborators Jan 18, 2020

Handle UISceneDelegate changes in Auth #3925

Handle UISceneDelegate changes in Auth #3925

Uh oh!

Conversation

renkelvin commented Sep 25, 2019

Uh oh!

paulb777 left a comment

Choose a reason for hiding this comment

Uh oh!

maksymmalyhin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

renkelvin Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

renkelvin Oct 17, 2019 •

edited

Loading