Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix container instantiation timing, IID startup. #4030

Merged
merged 9 commits into from Oct 11, 2019
Merged

Conversation

@ryanwilson
Copy link
Member

@ryanwilson ryanwilson commented Oct 9, 2019

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.
ryanwilson added 3 commits Oct 11, 2019
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).
@ryanwilson ryanwilson requested review from jasonhu-g and paulb777 Oct 11, 2019
@@ -22,6 +22,7 @@
#import <FirebaseInstanceID/FirebaseInstanceID.h>
#import <FirebaseAnalyticsInterop/FIRAnalyticsInterop.h>
#import <FirebaseMessaging/FIRMessaging.h>
#import <GoogleUtilities/GULUserDefaults.h>
Copy link
Contributor

@jasonhu-g jasonhu-g Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// 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. **
Copy link
Member

@paulb777 paulb777 Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@paulb777 paulb777 Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, I missed the call.

Copy link
Contributor

@wilhuff wilhuff Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// 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. **
Copy link
Member

@paulb777 paulb777 Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, I missed the call.

@ryanwilson
Copy link
Member Author

@ryanwilson ryanwilson commented Oct 11, 2019

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.

@ryanwilson ryanwilson requested a review from wilhuff Oct 11, 2019
BOOL (*isAutoInitEnabled)(id, SEL) = (BOOL(*)(id, SEL))isAutoInitEnabledIMP;
// Get the autoInitEnabled class method.
IMP isAutoInitEnabledIMP = [messagingClass methodForSelector:autoInitSelector];
BOOL(*isAutoInitEnabled)
Copy link
Contributor

@wilhuff wilhuff Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

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

@wilhuff wilhuff Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@ryanwilson ryanwilson Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed, thanks.

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

@wilhuff wilhuff Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@ryanwilson ryanwilson Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// 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. **
Copy link
Contributor

@wilhuff wilhuff Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@wilhuff wilhuff left a comment

All my comments are optional, so LGTM

@ryanwilson ryanwilson merged commit 56b37d2 into master Oct 11, 2019
2 checks passed
@ryanwilson ryanwilson deleted the rw-container-eager branch Oct 11, 2019
@paulb777 paulb777 added this to the 6.11.0 milestone Oct 15, 2019
@firebase firebase locked and limited conversation to collaborators Nov 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants