Remove casting of firebase::App platform specific data.

smiles · a-maurice · commit 841d9b7033a2 · 2019-09-10T18:16:17.000-07:00
Replaced friend class access to App with GetPlatformApp() that
allows access to the platform specific app implementation on
iOS and Android.

PiperOrigin-RevId: 265533440
diff --git a/app/src/app_android.cc b/app/src/app_android.cc
@@ -32,6 +32,12 @@ namespace firebase {
 
 DEFINE_FIREBASE_VERSION_STRING(Firebase);
 
+namespace internal {
+
+JOBJECT_REFERENCE(AppInternal);
+
+}  // namespace internal
+
 // Namespace and class for FirebaseApp.
 // java/com/google/android/gmscore/*/client/firebase-common/src/com/google/\
 // firebase/FirebaseApp.java
@@ -413,15 +419,13 @@ AppOptions* AppOptions::LoadDefault(AppOptions* app_options,
   return app_options;
 }
 
-App::App() : activity_(nullptr), data_(nullptr) {}
+App::App() : activity_(nullptr), internal_(nullptr) {}
 
 App::~App() {
   app_common::RemoveApp(this);
   JNIEnv* env = GetJNIEnv();
-  if (data_) {
-    env->DeleteGlobalRef(reinterpret_cast<jobject>(data_));
-    data_ = nullptr;
-  }
+  delete internal_;
+  internal_ = nullptr;
   if (activity_) {
     env->DeleteGlobalRef(reinterpret_cast<jobject>(activity_));
     activity_ = nullptr;
@@ -466,11 +470,9 @@ App* App::Create(const AppOptions& options, const char* name, JNIEnv* jni_env,
       app = new App();
       app->name_ = name;
       app->activity_ = jni_env->NewGlobalRef(activity);
-      jint result = jni_env->GetJavaVM(&app->java_vm_);
-      FIREBASE_ASSERT(result == JNI_OK);
       GetAppOptionsFromPlatformApp(jni_env, platform_app, &app->options_);
-      app->data_ = static_cast<void*>(jni_env->NewGlobalRef(platform_app));
-      jni_env->DeleteLocalRef(platform_app);
+      app->internal_ = new internal::AppInternal(
+          internal::AppInternal::FromLocalReference(jni_env, platform_app));
       app = app_common::AddApp(app, &app->init_results_);
     } else {
       ReleaseClasses(jni_env);
@@ -526,8 +528,7 @@ void App::SetDataCollectionDefaultEnabled(bool enabled) {
     return;
   }
   JNIEnv* env = GetJNIEnv();
-  jobject obj = reinterpret_cast<jobject>(data_);
-  env->CallVoidMethod(obj,
+  env->CallVoidMethod(**internal_,
                       app::GetMethodId(app::kSetDataCollectionDefaultEnabled),
                       enabled ? JNI_TRUE : JNI_FALSE);
   util::CheckAndClearJniExceptions(env);
@@ -540,13 +541,16 @@ bool App::IsDataCollectionDefaultEnabled() const {
     return true;
   }
   JNIEnv* env = GetJNIEnv();
-  jobject obj = reinterpret_cast<jobject>(data_);
   jboolean result = env->CallBooleanMethod(
-      obj, app::GetMethodId(app::kIsDataCollectionDefaultEnabled));
+      **internal_, app::GetMethodId(app::kIsDataCollectionDefaultEnabled));
   util::CheckAndClearJniExceptions(env);
   return result != JNI_FALSE;
 }
 
 const char* App::GetUserAgent() { return app_common::GetUserAgent(); }
 
+JavaVM* App::java_vm() const { return internal_->java_vm(); }
+
+jobject App::GetPlatformApp() const { return internal_->GetLocalRef(); }
+
 }  // namespace firebase
diff --git a/app/src/app_desktop.cc b/app/src/app_desktop.cc
@@ -29,7 +29,9 @@ namespace firebase {
 DEFINE_FIREBASE_VERSION_STRING(Firebase);
 
 namespace internal {
-struct PrivateAppData {
+
+class AppInternal {
+ public:
   // A registry that modules can use to expose functions to each other, without
   // requiring a linkage dependency.
   // todo - make all the implementations use something like this, for internal
@@ -115,14 +117,13 @@ AppOptions* AppOptions::LoadDefault(AppOptions* options) {
 }
 
 App::App() {
-  data_ = new internal::PrivateAppData();
+  internal_ = new internal::AppInternal();
 }
 
 App::~App() {
   app_common::RemoveApp(this);
-  // Once we use data_, we should delete it here.
-  delete static_cast<internal::PrivateAppData*>(data_);
-  data_ = nullptr;
+  delete internal_;
+  internal_ = nullptr;
 }
 
 // On desktop, if you create an app with no arguments, it will try to
@@ -164,7 +165,7 @@ App* App::GetInstance(const char* name) {  // NOLINT
 
 #ifdef INTERNAL_EXPERIMENTAL
 internal::FunctionRegistry* App::function_registry() {
-  return &(static_cast<internal::PrivateAppData*>(data_)->function_registry);
+  return &internal_->function_registry;
 }
 #endif
 
diff --git a/app/src/app_ios.h b/app/src/app_ios.h
@@ -26,8 +26,6 @@ namespace internal {
 OBJ_C_PTR_WRAPPER_NAMED(AppInternal, FIRApp);
 }  // namespace internal
 
-typedef internal::AppInternal FIRAppPointer;
-
 }  // namespace firebase
 
 #endif  // FIREBASE_APP_CLIENT_CPP_SRC_APP_IOS_H_
diff --git a/app/src/app_ios.mm b/app/src/app_ios.mm
@@ -220,14 +220,12 @@ static void PlatformOptionsToAppOptions(FIROptions* platform_options,
   return app_options;
 }
 
-App::App() : data_(nullptr) {}
+App::App() : internal_(nullptr) {}
 
 App::~App() {
   app_common::RemoveApp(this);
-  if (data_) {
-    delete static_cast<internal::AppInternal*>(data_);
-    data_ = nullptr;
-  }
+  delete internal_;
+  internal_ = nullptr;
 }
 
 App* App::Create() {
@@ -249,7 +247,7 @@ static void PlatformOptionsToAppOptions(FIROptions* platform_options,
     app = new App();
     app->options_ = options;
     app->name_ = name;
-    app->data_ = new internal::AppInternal(platform_app);
+    app->internal_ = new internal::AppInternal(platform_app);
     app_common::AddApp(app, &app->init_results_);
   }
   return app;
@@ -272,13 +270,15 @@ static void PlatformOptionsToAppOptions(FIROptions* platform_options,
 void App::SetDefaultConfigPath(const char* path) { }
 
 void App::SetDataCollectionDefaultEnabled(bool enabled) {
-  (**static_cast<internal::AppInternal*>(data_)).dataCollectionDefaultEnabled =
-      (enabled ? YES : NO);
+  GetPlatformApp().dataCollectionDefaultEnabled = (enabled ? YES : NO);
 }
 
 bool App::IsDataCollectionDefaultEnabled() const {
-  return (**static_cast<internal::AppInternal*>(data_)).isDataCollectionDefaultEnabled ?
-      true : false;
+  return GetPlatformApp().isDataCollectionDefaultEnabled ? true : false;
+}
+
+FIRApp* App::GetPlatformApp() const {
+  return internal_->get();
 }
 
 }  // namespace firebase
diff --git a/app/src/include/firebase/app.h b/app/src/include/firebase/app.h
@@ -22,9 +22,16 @@
 #if FIREBASE_PLATFORM_ANDROID
 #include <jni.h>
 #endif  // FIREBASE_PLATFORM_ANDROID
+
 #include <map>
 #include <string>
 
+#if FIREBASE_PLATFORM_IOS
+#ifdef __OBJC__
+@class FIRApp;
+#endif  // __OBJC__
+#endif  // FIREBASE_PLATFORM_IOS
+
 /// @brief Namespace that encompasses all Firebase APIs.
 #if !defined(FIREBASE_NAMESPACE)
 #define FIREBASE_NAMESPACE firebase
@@ -36,6 +43,7 @@ namespace FIREBASE_NAMESPACE {
 #ifdef INTERNAL_EXPERIMENTAL
 namespace internal {
 class FunctionRegistry;
+class AppInternal;
 }  // namespace internal
 #endif  // INTERNAL_EXPERIMENTAL
 
@@ -594,7 +602,7 @@ class App {
   /// @note This method is specific to the Android implementation.
   ///
   /// @return JNI Java virtual machine object.
-  JavaVM* java_vm() const { return java_vm_; }
+  JavaVM* java_vm() const;
   /// Get JNI environment, needed for performing JNI calls, set on creation.
   /// This is not trivial as the correct environment needs to retrieved per
   /// thread.
@@ -725,19 +733,24 @@ class App {
   static void SetDefaultConfigPath(const char* path);
 #endif  // INTERNAL_EXPERIMENTAL
 
- private:
-  /// @cond FIREBASE_APP_INTERNAL
-  friend class auth::Auth;
-  friend class crashlytics::internal::CrashlyticsInternal;
-  friend class database::internal::DatabaseInternal;
 #ifdef INTERNAL_EXPERIMENTAL
-  friend class firestore::FirestoreInternal;
+#if FIREBASE_PLATFORM_ANDROID
+  /// Get the platform specific app implementation referenced by this object.
+  ///
+  /// @return Global reference to the FirebaseApp.  The returned reference
+  /// most be deleted after use.
+  jobject GetPlatformApp() const;
+#elif FIREBASE_PLATFORM_IOS
+#ifdef __OBJC__
+  /// Get the platform specific app implementation referenced by this object.
+  ///
+  /// @return Reference to the FIRApp object owned by this app.
+  FIRApp* GetPlatformApp() const;
+#endif  // __OBJC__
+#endif  // FIREBASE_PLATFORM_ANDROID, FIREBASE_PLATFORM_IOS
 #endif  // INTERNAL_EXPERIMENTAL
-  friend class functions::internal::FunctionsInternal;
-  friend class instance_id::InstanceId;
-  friend class internal::InstanceId;
-  friend class storage::internal::StorageInternal;
 
+ private:
   /// Construct the object.
   App();
 
@@ -746,10 +759,6 @@ class App {
 // Unity doesn't need the JNI from here, it has its method to access JNI.
 // </SWIG>
 #if FIREBASE_PLATFORM_ANDROID || defined(DOXYGEN)
-  /// JNI reference to the Java virtual machine associated with the App's
-  /// process.
-  /// @note This is specific to Android.
-  JavaVM* java_vm_;
   /// Android activity.
   /// @note This is specific to Android.
   jobject activity_;
@@ -763,7 +772,7 @@ class App {
   /// Module initialization results.
   std::map<std::string, InitResult> init_results_;
   /// Pointer to other internal data used by this instance.
-  void* data_;
+  internal::AppInternal* internal_;
 
   /// @endcond
 };
diff --git a/auth/src/android/auth_android.cc b/auth/src/android/auth_android.cc
@@ -186,13 +186,10 @@ static void ReleaseClasses(JNIEnv* env) {
   ReleaseCommonClasses(env);
 }
 
-void* CreatePlatformAuth(App* app, void* app_impl) {
-  FIREBASE_ASSERT(app_impl != nullptr);
-
+void* CreatePlatformAuth(App* app) {
   // Grab varous java objects from the app.
   JNIEnv* env = app->GetJNIEnv();
   jobject activity = app->activity();
-  jobject j_app = reinterpret_cast<jobject>(app_impl);
 
   // Cache the JNI method ids so we only have to look them up by name once.
   if (!g_initialized_count) {
@@ -218,9 +215,11 @@ void* CreatePlatformAuth(App* app, void* app_impl) {
   g_initialized_count++;
 
   // Create the FirebaseAuth class in Java.
+  jobject platform_app = app->GetPlatformApp();
   jobject j_auth_impl = env->CallStaticObjectMethod(
-      auth::GetClass(), auth::GetMethodId(auth::kGetInstance), j_app);
-  assert(env->ExceptionCheck() == false);
+      auth::GetClass(), auth::GetMethodId(auth::kGetInstance), platform_app);
+  FIREBASE_ASSERT(!util::CheckAndClearJniExceptions(env));
+  env->DeleteLocalRef(platform_app);
 
   // Ensure the reference hangs around.
   void* auth_impl = nullptr;
diff --git a/auth/src/auth.cc b/auth/src/auth.cc
@@ -78,7 +78,7 @@ Auth* Auth::GetAuth(App* app, InitResult* init_result_out) {
   FIREBASE_UTIL_RETURN_NULL_IF_GOOGLE_PLAY_UNAVAILABLE(*app, init_result_out);
 
   // Create the platform dependent version of Auth.
-  void* auth_impl = CreatePlatformAuth(app, app->data_);
+  void* auth_impl = CreatePlatformAuth(app);
   if (!auth_impl) return nullptr;
 
   // Create a new Auth and initialize.
diff --git a/auth/src/common.h b/auth/src/common.h
@@ -31,7 +31,7 @@ enum CredentialApiFunction {
 };
 
 // Platform-specific method to create the wrapped Auth class.
-void* CreatePlatformAuth(App* app, void* app_impl);
+void* CreatePlatformAuth(App* app);
 
 // Platform-specific method to initialize AuthData.
 void InitPlatformAuth(AuthData* auth_data);
diff --git a/auth/src/desktop/auth_desktop.cc b/auth/src/desktop/auth_desktop.cc
@@ -82,7 +82,7 @@ Future<ResultT> DoSignInWithCredential(Promise<ResultT> promise,
 
 }  // namespace
 
-void* CreatePlatformAuth(App* const app, void* const /*app_impl*/) {
+void* CreatePlatformAuth(App* const app) {
   FIREBASE_ASSERT_RETURN(nullptr, app);
 
   AuthImpl* const auth = new AuthImpl();
diff --git a/auth/src/ios/auth_ios.mm b/auth/src/ios/auth_ios.mm
@@ -124,11 +124,9 @@ explicit ListenerHandleHolder(T handle) : handle(handle) {}
 };
 
 // Platform-specific method to create the wrapped Auth class.
-void *CreatePlatformAuth(App *app, void *app_impl) {
-  FIRApp *fir_app = static_cast<FIRAppPointer *>(app_impl)->get();
-
+void *CreatePlatformAuth(App *app) {
   // Grab the auth for our app.
-  FIRAuth *auth = [FIRAuth authWithApp:fir_app];
+  FIRAuth *auth = [FIRAuth authWithApp:app->GetPlatformApp()];
   FIREBASE_ASSERT(auth != nullptr);
 
   // Create a FIRAuth* that uses Objective-C's automatic reference counting.
diff --git a/database/src/android/database_android.cc b/database/src/android/database_android.cc
@@ -169,10 +169,12 @@ DatabaseInternal::DatabaseInternal(App* app) : log_level_(kLogLevelInfo) {
   app_ = app;
 
   JNIEnv* env = app_->GetJNIEnv();
+  jobject platform_app = app->GetPlatformApp();
   jobject database_obj = env->CallStaticObjectMethod(
       firebase_database::GetClass(),
       firebase_database::GetMethodId(firebase_database::kGetInstance),
-      reinterpret_cast<jobject>(app_->data_));
+      platform_app);
+  env->DeleteLocalRef(platform_app);
   if (database_obj == nullptr) {
     LogWarning("Could not create default Database");
     util::CheckAndClearJniExceptions(env);
@@ -193,10 +195,12 @@ DatabaseInternal::DatabaseInternal(App* app, const char* url)
 
   JNIEnv* env = app_->GetJNIEnv();
   jobject url_string = env->NewStringUTF(url);
+  jobject platform_app = app->GetPlatformApp();
   jobject database_obj = env->CallStaticObjectMethod(
       firebase_database::GetClass(),
       firebase_database::GetMethodId(firebase_database::kGetInstanceFromUrl),
-      reinterpret_cast<jobject>(app_->data_), url_string);
+      platform_app, url_string);
+  env->DeleteLocalRef(platform_app);
   if (database_obj == nullptr) {
     LogWarning("Could not create Database with URL '%s' .", url);
     util::CheckAndClearJniExceptions(env);
diff --git a/database/src/ios/database_ios.mm b/database/src/ios/database_ios.mm
@@ -32,7 +32,7 @@
     : app_(app), log_level_(kLogLevelInfo) {
   @try {
     impl_.reset(new FIRDatabasePointer(
-        [FIRDatabase databaseForApp:static_cast<FIRAppPointer*>(app->data_)->get()]));
+        [FIRDatabase databaseForApp:app->GetPlatformApp()]));
     query_lock_.reset(new NSRecursiveLockPointer([[NSRecursiveLock alloc] init]));
   }
   @catch (NSException* exception) {
@@ -46,7 +46,7 @@
     : app_(app), constructor_url_(url), log_level_(kLogLevelInfo) {
   @try {
     impl_.reset(new FIRDatabasePointer(
-        [FIRDatabase databaseForApp:static_cast<FIRAppPointer*>(app->data_)->get() URL:@(url)]));
+        [FIRDatabase databaseForApp:app->GetPlatformApp() URL:@(url)]));
     query_lock_.reset(new NSRecursiveLockPointer([[NSRecursiveLock alloc] init]));
   }
   @catch (NSException* exception) {
diff --git a/functions/src/android/functions_android.cc b/functions/src/android/functions_android.cc
@@ -83,11 +83,13 @@ FunctionsInternal::FunctionsInternal(App* app, const char* region)
   app_ = app;
   JNIEnv* env = app_->GetJNIEnv();
   jstring region_str = env->NewStringUTF(region);
+  jobject platform_app = app_->GetPlatformApp();
   jobject functions_obj = env->CallStaticObjectMethod(
       firebase_functions::GetClass(),
       firebase_functions::GetMethodId(firebase_functions::kGetInstance),
-      reinterpret_cast<jobject>(app_->data_), region_str);
+      platform_app, region_str);
   util::CheckAndClearJniExceptions(env);
+  env->DeleteLocalRef(platform_app);
   env->DeleteLocalRef(region_str);
   assert(functions_obj != nullptr);
   obj_ = env->NewGlobalRef(functions_obj);
diff --git a/functions/src/ios/functions_ios.mm b/functions/src/ios/functions_ios.mm
@@ -29,7 +29,7 @@
 FunctionsInternal::FunctionsInternal(App* app, const char* region)
     : app_(app), region_(region) {
   impl_.reset(new FIRFunctionsPointer([FIRFunctions
-      functionsForApp:static_cast<FIRAppPointer*>(app->data_)->get()
+      functionsForApp:app->GetPlatformApp()
                region:@(region)]));
 }
 
diff --git a/instance_id/src/android/instance_id.cc b/instance_id/src/android/instance_id.cc
diff --git a/storage/src/android/storage_android.cc b/storage/src/android/storage_android.cc
diff --git a/storage/src/ios/storage_ios.mm b/storage/src/ios/storage_ios.mm
