Handle jniresultcallback not loading due to lack of gms.Task. (#953)

jonsimantov · web-flow · commit 6985af993aa7 · 2022-05-13T04:37:08.000Z
* Handle jniresultcallback not loading due to lack of gms.Task.

* Make initialization of google_play_services optional in app.

Check for initialization in util.cc.

* Add alternate inclusion method for app that doesn't require play-services.

* Add readme note

* Return an error if you try to use ModuleInitializer without play-services.
diff --git a/Android/firebase_dependencies.gradle b/Android/firebase_dependencies.gradle
@@ -17,28 +17,24 @@ import org.gradle.util.ConfigureUtil;
 // A map of library to the dependencies that need to be added for it.
 def firebaseDependenciesMap = [
   'app' : ['com.google.firebase:firebase-analytics:21.0.0'],
+  'play_services' : ['com.google.android.gms:play-services-base:18.0.1'],
   'admob' : ['com.google.firebase:firebase-ads:19.8.0',
-             'com.google.firebase:firebase-analytics:21.0.0',
-             'com.google.android.gms:play-services-base:18.0.1'],
-  'analytics' : ['com.google.firebase:firebase-analytics:21.0.0',
-                 'com.google.android.gms:play-services-base:18.0.1'],
+             'com.google.firebase:firebase-analytics:21.0.0'],
+  'analytics' : ['com.google.firebase:firebase-analytics:21.0.0'],
   'auth' : ['com.google.firebase:firebase-auth:21.0.3'],
   'database' : ['com.google.firebase:firebase-database:20.0.5'],
   'dynamic_links' : ['com.google.firebase:firebase-dynamic-links:21.0.1'],
   'firestore' : ['com.google.firebase:firebase-firestore:24.1.2'],
   'functions' : ['com.google.firebase:firebase-functions:20.1.0'],
-  'installations' : ['com.google.firebase:firebase-installations:17.0.1',
-                     'com.google.android.gms:play-services-base:18.0.1'],
+  'installations' : ['com.google.firebase:firebase-installations:17.0.1'],
   'invites' : ['com.google.firebase:firebase-invites:17.0.0'],
   // Messaging has an additional local dependency to include.
   'messaging' : ['com.google.firebase:firebase-messaging:23.0.4',
                  'firebase_cpp_sdk.messaging:messaging_java',
                  'androidx.core:core:1.6.0-alpha03',
-                 'com.google.flatbuffers:flatbuffers-java:1.12.0',
-                 'com.google.android.gms:play-services-base:18.0.1'],
+                 'com.google.flatbuffers:flatbuffers-java:1.12.0'],
   'performance' : ['com.google.firebase:firebase-perf:20.0.6'],
-  'remote_config' : ['com.google.firebase:firebase-config:21.1.0',
-                     'com.google.android.gms:play-services-base:18.0.1'],
+  'remote_config' : ['com.google.firebase:firebase-config:21.1.0'],
   'storage' : ['com.google.firebase:firebase-storage:20.0.1'],
   'testlab' : []
 ]
@@ -68,6 +64,10 @@ class Dependencies {
 
   def getApp() {
     libSet.add('app')
+    libSet.add('play_services')
+  }
+  def getAppWithoutPlayServices() {
+    libSet.add('app')
   }
   def getAdmob() {
     libSet.add('admob')
@@ -146,6 +146,7 @@ project.afterEvaluate {
     // App is required, so add it if it wasn't included.
     if (!firebaseCpp.dependencies.libSet.contains('app')) {
       firebaseCpp.dependencies.libSet.add('app')
+      firebaseCpp.dependencies.libSet.add('play_services')
     }
 
     for (String lib : firebaseCpp.dependencies.libSet) {
diff --git a/admob/integration_test/build.gradle b/admob/integration_test/build.gradle
@@ -75,7 +75,10 @@ android {
 }
 
 apply from: "$gradle.firebase_cpp_sdk_dir/Android/firebase_dependencies.gradle"
+// AdMob doesn't require play-services-base as a dependency, so include the
+// version of App without it, to ensure the C++ SDK also doesn't require it.
 firebaseCpp.dependencies {
+  appWithoutPlayServices
   admob
 }
 
diff --git a/app/src/app_android.cc b/app/src/app_android.cc
@@ -150,6 +150,7 @@ METHOD_LOOKUP_DEFINITION(
 namespace {
 
 static int g_methods_cached_count = 0;
+static bool g_initialized_google_play_services = false;
 
 void ReleaseClasses(JNIEnv* env);
 
@@ -165,11 +166,13 @@ bool CacheMethods(JNIEnv* env, jobject activity) {
     if (!(app::CacheMethodIds(env, activity) &&
           options_builder::CacheMethodIds(env, activity) &&
           options::CacheMethodIds(env, activity) &&
-          version_registrar::CacheMethodIds(env, activity) &&
-          google_play_services::Initialize(env, activity))) {
+          version_registrar::CacheMethodIds(env, activity))) {
       ReleaseClasses(env);
       return false;
     }
+    if (google_play_services::Initialize(env, activity)) {
+      g_initialized_google_play_services = true;
+    }
   }
   return true;
 }
@@ -182,7 +185,10 @@ void ReleaseClasses(JNIEnv* env) {
     options_builder::ReleaseClass(env);
     options::ReleaseClass(env);
     version_registrar::ReleaseClass(env);
-    google_play_services::Terminate(env);
+    if (g_initialized_google_play_services) {
+      google_play_services::Terminate(env);
+      g_initialized_google_play_services = false;
+    }
     util::Terminate(env);
   }
 }
diff --git a/app/src/util.cc b/app/src/util.cc
@@ -25,7 +25,9 @@
 #include "app/src/include/firebase/internal/platform.h"
 
 #if FIREBASE_PLATFORM_ANDROID
+#include "app/src/google_play_services/availability_android.h"
 #include "app/src/include/google_play_services/availability.h"
+#include "app/src/util_android.h"
 #endif  // FIREBASE_PLATFORM_ANDROID
 #include "app/src/include/firebase/internal/mutex.h"
 #include "app/src/log.h"
@@ -77,32 +79,49 @@ static void PerformInitialize(ModuleInitializerData* data) {
 
 #if FIREBASE_PLATFORM_ANDROID
     if (init_result == kInitResultFailedMissingDependency) {
-      // On Android, we need to update or activate Google Play services
-      // before we can initialize this Firebase module.
-      LogWarning("Google Play services unavailable, trying to fix.");
-
-      Future<void> make_available = google_play_services::MakeAvailable(
-          data->app->GetJNIEnv(), data->app->activity());
-
-      make_available.OnCompletion(
-          [](const Future<void>& result, void* ptr) {
-            ModuleInitializerData* data =
-                reinterpret_cast<ModuleInitializerData*>(ptr);
-            if (result.status() == kFutureStatusComplete) {
-              if (result.error() == 0) {
-                LogInfo("Google Play services now available, continuing.");
-                PerformInitialize(data);
-              } else {
-                LogError("Google Play services still unavailable.");
-                int num_remaining = data->init_fns.size() - data->init_fn_idx;
-                data->future_impl.Complete(
-                    data->future_handle_init, num_remaining,
-                    "Unable to initialize due to missing Google Play services "
-                    "dependency.");
+      // Note: If Initialize here succeeds, google_play_services::Terminate
+      // is called in the OnCompletion handler below. Note that these
+      // are reference-counted so it's safe to init/terminate an extra time..
+      if (google_play_services::Initialize(data->app->GetJNIEnv(),
+                                           data->app->activity())) {
+        // On Android, we need to update or activate Google Play services
+        // before we can initialize this Firebase module.
+        LogWarning("Google Play services unavailable, trying to fix.");
+
+        Future<void> make_available = google_play_services::MakeAvailable(
+            data->app->GetJNIEnv(), data->app->activity());
+
+        make_available.OnCompletion(
+            [](const Future<void>& result, void* ptr) {
+              ModuleInitializerData* data =
+                  reinterpret_cast<ModuleInitializerData*>(ptr);
+              if (result.status() == kFutureStatusComplete) {
+                if (result.error() == 0) {
+                  LogInfo("Google Play services now available, continuing.");
+                  PerformInitialize(data);
+                  google_play_services::Terminate(data->app->GetJNIEnv());
+                } else {
+                  LogError("Google Play services still unavailable.");
+                  int num_remaining = data->init_fns.size() - data->init_fn_idx;
+                  data->future_impl.Complete(data->future_handle_init,
+                                             num_remaining,
+                                             "Unable to initialize due to "
+                                             "missing Google Play services "
+                                             "dependency.");
+                  google_play_services::Terminate(util::GetJNIEnvFromApp());
+                }
               }
-            }
-          },
-          data);
+            },
+            data);
+      } else {
+        int num_remaining = data->init_fns.size() - data->init_fn_idx;
+        data->future_impl.Complete(
+            data->future_handle_init, num_remaining,
+            "Could not run Google Play services update due to app "
+            "misconfiguration. Please add "
+            "com.google.android.gms:play-services-base as an Android "
+            "dependency to enable this functionality.");
+      }
     }
 #else   // !FIREBASE_PLATFORM_ANDROID
     // Outside of Android, we shouldn't get kInitResultFailedMissingDependency.
diff --git a/app/src/util_android.cc b/app/src/util_android.cc
@@ -245,6 +245,8 @@ static pthread_mutex_t g_task_callbacks_mutex;
 // classes.
 static std::vector<jobject>* g_class_loaders;
 
+static bool g_jniresultcallback_loaded = false;
+
 JNIEXPORT void JNICALL JniResultCallback_nativeOnResult(
     JNIEnv* env, jobject clazz, jobject result, jboolean success,
     jboolean cancelled, jstring status_message, jlong callback_fn_param,
@@ -393,7 +395,10 @@ static void ReleaseClasses(JNIEnv* env) {
   uri::ReleaseClass(env);
   object::ReleaseClass(env);
   uribuilder::ReleaseClass(env);
-  jniresultcallback::ReleaseClass(env);
+  if (g_jniresultcallback_loaded) {
+    jniresultcallback::ReleaseClass(env);
+    g_jniresultcallback_loaded = false;
+  }
   JavaThreadContext::Terminate(env);
 #if defined(FIREBASE_ANDROID_FOR_DESKTOP)
   java_uri::ReleaseClass(env);
@@ -526,12 +531,13 @@ bool Initialize(JNIEnv* env, jobject activity_object) {
     return false;
   }
 
-  if (!(jniresultcallback::CacheClassFromFiles(env, activity_object,
-                                               &embedded_files) &&
-        jniresultcallback::CacheMethodIds(env, activity_object) &&
-        jniresultcallback::RegisterNatives(env, &kJniCallbackMethod, 1))) {
-    return false;
-  }
+  // With a subset of dependencies, jniresultcallback can't be loaded due to
+  // lack of gms.Task. This is fine - just make sure not to try terminating it.
+  g_jniresultcallback_loaded =
+      (jniresultcallback::CacheClassFromFiles(env, activity_object,
+                                              &embedded_files) &&
+       jniresultcallback::CacheMethodIds(env, activity_object) &&
+       jniresultcallback::RegisterNatives(env, &kJniCallbackMethod, 1));
 
   if (!JavaThreadContext::Initialize(env, activity_object, embedded_files)) {
     return false;
diff --git a/release_build_files/Android/firebase_dependencies.gradle b/release_build_files/Android/firebase_dependencies.gradle
@@ -17,28 +17,24 @@ import org.gradle.util.ConfigureUtil;
 // A map of library to the dependencies that need to be added for it.
 def firebaseDependenciesMap = [
   'app' : ['com.google.firebase:firebase-analytics:21.0.0'],
+  'play_services' : ['com.google.android.gms:play-services-base:18.0.1'],
   'admob' : ['com.google.firebase:firebase-ads:19.8.0',
-             'com.google.firebase:firebase-analytics:21.0.0',
-             'com.google.android.gms:play-services-base:18.0.1'],
-  'analytics' : ['com.google.firebase:firebase-analytics:21.0.0',
-                 'com.google.android.gms:play-services-base:18.0.1'],
+             'com.google.firebase:firebase-analytics:21.0.0'],
+  'analytics' : ['com.google.firebase:firebase-analytics:21.0.0'],
   'auth' : ['com.google.firebase:firebase-auth:21.0.3'],
   'database' : ['com.google.firebase:firebase-database:20.0.5'],
   'dynamic_links' : ['com.google.firebase:firebase-dynamic-links:21.0.1'],
   'firestore' : ['com.google.firebase:firebase-firestore:24.1.2'],
   'functions' : ['com.google.firebase:firebase-functions:20.1.0'],
-  'installations' : ['com.google.firebase:firebase-installations:17.0.1',
-                     'com.google.android.gms:play-services-base:18.0.1'],
+  'installations' : ['com.google.firebase:firebase-installations:17.0.1'],
   'invites' : ['com.google.firebase:firebase-invites:17.0.0'],
   // Messaging has an additional local dependency to include.
   'messaging' : ['com.google.firebase:firebase-messaging:23.0.4',
                  'com.google.firebase.messaging.cpp:firebase_messaging_cpp@aar',
                  'androidx.core:core:1.6.0-alpha03',
-                 'com.google.flatbuffers:flatbuffers-java:1.12.0',
-                 'com.google.android.gms:play-services-base:18.0.1'],
+                 'com.google.flatbuffers:flatbuffers-java:1.12.0'],
   'performance' : ['com.google.firebase:firebase-perf:20.0.6'],
-  'remote_config' : ['com.google.firebase:firebase-config:21.1.0',
-                     'com.google.android.gms:play-services-base:18.0.1'],
+  'remote_config' : ['com.google.firebase:firebase-config:21.1.0'],
   'storage' : ['com.google.firebase:firebase-storage:20.0.1'],
   'testlab' : []
 ]
@@ -49,6 +45,10 @@ class Dependencies {
 
   def getApp() {
     libSet.add('app')
+    libSet.add('play_services')
+  }
+  def getAppWithoutPlayServices() {
+    libSet.add('app')
   }
   def getAdmob() {
     libSet.add('admob')
@@ -121,6 +121,7 @@ project.afterEvaluate {
     // App is required, so add it if it wasn't included.
     if (!firebaseCpp.dependencies.libSet.contains('app')) {
       firebaseCpp.dependencies.libSet.add('app')
+      firebaseCpp.dependencies.libSet.add('play_services')
     }
 
     for (String lib : firebaseCpp.dependencies.libSet) {
diff --git a/release_build_files/readme.md b/release_build_files/readme.md
@@ -166,6 +166,32 @@ Android SDK (20.x.x). Please ensure that you use firebase-ads version 19.8.0 in
 conjunction with the latest firebase-analytics version to maintain
 compatibility.
 
+#### Gradle dependency file
+
+Firebase C++ includes an `Android/firebase_dependencies.gradle` file
+that helps you include the correct Android dependencies and native C++
+libraries for each Firebase product. To use it, include the following
+in your build.gradle file (you can omit any Firebase products you
+aren't using):
+
+```
+apply from: "$gradle.firebase_cpp_sdk_dir/Android/firebase_dependencies.gradle"
+firebaseCpp.dependencies {
+  app  // Recommended for all apps using Firebase.
+  admob
+  analytics
+  auth
+  database
+  dynamicLinks
+  firestore
+  functions
+  installations
+  messaging
+  remoteConfig
+  storage
+}
+```
+
 ### iOS Dependencies
 
 iOS users can include either xcframeworks or static libraries depending upon their
@@ -572,6 +598,12 @@ workflow use only during the development of your app, not for publicly shipping
 code.
 
 ## Release Notes
+### Upcoming release
+-   Changes
+    - General (Android): Fixed a bug that required Android apps to include
+      `com.google.android.gms:play-services-base` as an explicit dependency when
+      only using AdMob, Analytics, Remote Config, or Messaging.
+
 ### 9.0.0
 -   Changes
     -   General (iOS): Firebase C++ on iOS is now built using Xcode 13.3.1.
diff --git a/remote_config/integration_test/build.gradle b/remote_config/integration_test/build.gradle
@@ -75,7 +75,10 @@ android {
 }
 
 apply from: "$gradle.firebase_cpp_sdk_dir/Android/firebase_dependencies.gradle"
+// Remote Config doesn't require play-services-base as a dependency, so include
+// the version of App without it, to ensure the C++ SDK also doesn't require it.
 firebaseCpp.dependencies {
+  appWithoutPlayServices
   remoteConfig
 }
 

Original file line number	Diff line number	Diff line change
`@@ -75,7 +75,10 @@ android {`
`75`	`75`	`}`
`76`	`76`
`77`	`77`	`apply from: "$gradle.firebase_cpp_sdk_dir/Android/firebase_dependencies.gradle"`
	`78`	`+// AdMob doesn't require play-services-base as a dependency, so include the`
	`79`	`+// version of App without it, to ensure the C++ SDK also doesn't require it.`
`78`	`80`	`firebaseCpp.dependencies {`
	`81`	`+ appWithoutPlayServices`
`79`	`82`	`admob`
`80`	`83`	`}`
`81`	`84`