Fix container instantiation timing, IID startup. #4030

ryanwilson · 2019-10-09T19:51:43Z

The container instantiation could happen at the wrong time, before all
components have been added to the container. This fixes it, and also
moves IID to use the proper instantiation timing instead of relying on
the configureWithApp: call. This is part continuation of #3147, where
I'll be fixing the rest of the SDKs in follow up PRs.

Edit: Further work...

This moves the instantiation of components to after the FirebaseApp is
completely configured and assigned, as it should be. This also exposed
a recursive issue with IID calling the Messaging singleton, which in
turn called IID and instantiated multiple instances.

A change was made to break the cycle: the Messaging isAutoInitEnabled
call was changed to a static method vs an instance method, allowing IID
to call it during initialization without instantiating the Messaging
instance (since it doesn't have a direct dependency on it).

The container instantiation could happen at the wrong time, before all components have been added to the container. This fixes it, and also moves IID to use the proper instantiation timing instead of relying on the `configureWithApp:` call. This is part continuation of #3147, where I'll be fixing the rest of the SDKs in follow up PRs.

This moves the instantiation of components to after the FirebaseApp is completely configured and assigned, as it should be. This also exposed a recursive issue with IID calling the Messaging singleton, which in turn called IID and instantiated multiple instances. A change was made to break the cycle: the Messaging `isAutoInitEnabled` call was changed to a static method vs an instance method, allowing IID to call it during initialization without instantiating the Messaging instance (since it doesn't have a direct dependency on it).

jasonhu-g · 2019-10-11T13:42:23Z

Example/Messaging/Tests/FIRMessagingTest.m

@@ -22,6 +22,7 @@
 #import <FirebaseInstanceID/FirebaseInstanceID.h>
 #import <FirebaseAnalyticsInterop/FIRAnalyticsInterop.h>
 #import <FirebaseMessaging/FIRMessaging.h>
+#import <GoogleUtilities/GULUserDefaults.h>


This import doesn't seem to be needed since the class uses NSUserDefaults directly

paulb777 · 2019-10-11T14:04:32Z

Firebase/Messaging/FIRMessaging.m

+/// exposed as a class property for IID to fetch the property without instantiating an instance of
+/// Messaging. Since Messaging can only be used with the default FIRApp, we can have one point of
+/// entry without context of which FIRApp instance is being used.
+/// ** THIS METHOD IS DEPENDED ON INTERNALLY BY IID. PLEASE DO NOT CHANGE THE SIGNATURE. **


From my read it looks like isAutoInitEnabled is depended upon by Messaging. isAutoInitEnabledWithUserDefaults is only depended upon by the tests.

nvm, I missed the call.

This comment doesn't strongly enough indicate that the call is made reflectively so it won't necessarily show up in find usages.

paulb777 · 2019-10-11T14:27:02Z

Firebase/Messaging/FIRMessaging.m

+/// exposed as a class property for IID to fetch the property without instantiating an instance of
+/// Messaging. Since Messaging can only be used with the default FIRApp, we can have one point of
+/// entry without context of which FIRApp instance is being used.
+/// ** THIS METHOD IS DEPENDED ON INTERNALLY BY IID. PLEASE DO NOT CHANGE THE SIGNATURE. **


nvm, I missed the call.

ryanwilson · 2019-10-11T16:49:09Z

Discussed offline with @paulb777 - we'll look at fixing the IID dependency on Messaging in a follow up PR by making Messaging instantiate first and initialize IID appropriately.

wilhuff · 2019-10-11T17:06:46Z

Firebase/InstanceID/FIRInstanceID.m

- BOOL (*isAutoInitEnabled)(id, SEL) = (BOOL(*)(id, SEL))isAutoInitEnabledIMP;
+ // Get the autoInitEnabled class method.
+ IMP isAutoInitEnabledIMP = [messagingClass methodForSelector:autoInitSelector];
+ BOOL(*isAutoInitEnabled)


I believe you can cast through (void *) to avoid repeating yourself on the right hand side of this statement.

wilhuff · 2019-10-11T17:17:05Z

Firebase/Core/FIRComponentContainer.m

+ }
+
+ // All eager instantiation is complete, empty and clear the stored property now.
+ [self.eagerProtocolsToInstantiate removeAllObjects];


Assigning nil is sufficient to release all the contents of the array.

Changed, thanks.

wilhuff · 2019-10-11T17:18:01Z

Firebase/InstanceID/FIRInstanceID.m

+ // InstanceID only works with the default app.
+ if (!container.app.isDefaultApp) {
+ // Only configure for the default FIRApp.
+ FIRInstanceIDLoggerDebug(kFIRInstanceIDMessageCodeFIRApp002,


Shouldn't this be a warning or even an error?

Feel free to ignore--I see that this is just copypasta from the prior state.

This is also a "this code should never be run" so maybe it should be an error, but I'll leave it as is since it was copied from before.

wilhuff · 2019-10-11T17:25:12Z

Firebase/Messaging/FIRMessaging.m

+/// exposed as a class property for IID to fetch the property without instantiating an instance of
+/// Messaging. Since Messaging can only be used with the default FIRApp, we can have one point of
+/// entry without context of which FIRApp instance is being used.
+/// ** THIS METHOD IS DEPENDED ON INTERNALLY BY IID. PLEASE DO NOT CHANGE THE SIGNATURE. **


This comment doesn't strongly enough indicate that the call is made reflectively so it won't necessarily show up in find usages.

wilhuff

All my comments are optional, so LGTM

googlebot added the cla: yes label Oct 9, 2019

Typo

183cd74

jasonhu-g approved these changes Oct 9, 2019

View reviewed changes

paulb777 approved these changes Oct 9, 2019

View reviewed changes

ryanwilson added 3 commits October 10, 2019 22:13

Revert unintentional formatting of entire FIRMessaging file.

b909054

Fix tests.

9e26c95

ryanwilson requested review from jasonhu-g and paulb777 October 11, 2019 02:27

jasonhu-g reviewed Oct 11, 2019

View reviewed changes

jasonhu-g approved these changes Oct 11, 2019

View reviewed changes

ryanwilson added 2 commits October 11, 2019 09:48

Format InstanceID.m

459f0f8

Remove unused import.

a739b23

paulb777 requested changes Oct 11, 2019

View reviewed changes

paulb777 approved these changes Oct 11, 2019

View reviewed changes

Fix ComponentContainer test.

f7931a2

ryanwilson requested a review from wilhuff October 11, 2019 16:49

wilhuff reviewed Oct 11, 2019

View reviewed changes

wilhuff assigned ryanwilson Oct 11, 2019

wilhuff approved these changes Oct 11, 2019

View reviewed changes

Minor PR feedback.

a7820c4

ryanwilson merged commit 56b37d2 into master Oct 11, 2019

ryanwilson deleted the rw-container-eager branch October 11, 2019 21:20

jasonhu-g mentioned this pull request Oct 14, 2019

Move FIRInstanceIDInstanceProvider into the private header file for components to look up an instance of FIRInstanceID #4037

Closed

ryanwilson mentioned this pull request Oct 15, 2019

Crash at launch on iOS 13 only in production environment #3915

Closed

paulb777 added this to the 6.11.0 milestone Oct 15, 2019

firebase locked and limited conversation to collaborators Nov 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix container instantiation timing, IID startup. #4030

Fix container instantiation timing, IID startup. #4030

ryanwilson commented Oct 9, 2019 •

edited

jasonhu-g Oct 11, 2019

paulb777 Oct 11, 2019

paulb777 Oct 11, 2019

wilhuff Oct 11, 2019

paulb777 Oct 11, 2019

ryanwilson commented Oct 11, 2019

wilhuff Oct 11, 2019

wilhuff Oct 11, 2019

ryanwilson Oct 11, 2019

wilhuff Oct 11, 2019

ryanwilson Oct 11, 2019

wilhuff Oct 11, 2019

wilhuff left a comment

Fix container instantiation timing, IID startup. #4030

Fix container instantiation timing, IID startup. #4030

Conversation

ryanwilson commented Oct 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanwilson commented Oct 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhuff left a comment

Choose a reason for hiding this comment

ryanwilson commented Oct 9, 2019 •

edited