Skip to content

Commit 50828a9

Browse files
authored
Fix user-agent register timing in C++ and Unity SDK (#1217)
Change user-agent register timing in C++ Change to register user-agents before platform App creation so that the new user-agents can be captured and logged at iOS/Android level. Also add Android utility `CallAfterEnsureMethodsCached` which is used internally to ensure the callback is triggered when all necessary Java classes and methods are cached.
1 parent fc9bbbf commit 50828a9

File tree

11 files changed

+122
-27
lines changed

11 files changed

+122
-27
lines changed

app/src/app_android.cc

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
* limitations under the License.
1515
*/
1616

17+
#include "app/src/app_android.h"
18+
1719
#include <jni.h>
1820
#include <string.h>
1921

@@ -480,6 +482,9 @@ App* App::Create(const AppOptions& options, const char* name, JNIEnv* jni_env,
480482
}
481483
LogDebug("Creating Firebase App %s for %s", name, kFirebaseVersionString);
482484
if (CacheMethods(jni_env, activity)) {
485+
// Register C++ user-agents before creating Android app.
486+
app_common::RegisterSdkUsage(jni_env);
487+
483488
// Try to get or create a new FirebaseApp object.
484489
jobject platform_app =
485490
CreateOrGetPlatformApp(jni_env, options, name, activity);
@@ -527,9 +532,14 @@ static void RegisterLibraryWithVersionRegistrar(JNIEnv* env,
527532
env->DeleteLocalRef(registrar);
528533
}
529534

530-
void App::RegisterLibrary(const char* library, const char* version) {
531-
RegisterLibraryWithVersionRegistrar(util::GetJNIEnvFromApp(), library,
532-
version);
535+
void App::RegisterLibrary(const char* library, const char* version,
536+
void* platform_resource) {
537+
FIREBASE_ASSERT(platform_resource);
538+
539+
// Always relies on platform_resource to get reference to JNIEnv* to reduce
540+
// complexity.
541+
RegisterLibraryWithVersionRegistrar(
542+
reinterpret_cast<JNIEnv*>(platform_resource), library, version);
533543
app_common::RegisterLibrary(library, version);
534544
}
535545

@@ -570,4 +580,12 @@ JavaVM* App::java_vm() const { return internal_->java_vm(); }
570580

571581
jobject App::GetPlatformApp() const { return internal_->GetLocalRef(); }
572582

583+
void CallAfterEnsureMethodsCached(JNIEnv* env, jobject activity,
584+
std::function<void()> callback) {
585+
if (CacheMethods(env, activity)) {
586+
callback();
587+
ReleaseClasses(env);
588+
}
589+
}
590+
573591
} // namespace firebase

app/src/app_android.h

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
/*
2+
* Copyright 2023 Google LLC
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
#ifndef FIREBASE_APP_SRC_APP_ANDROID_H_
18+
#define FIREBASE_APP_SRC_APP_ANDROID_H_
19+
20+
#include <jni.h>
21+
22+
#include <functional>
23+
24+
namespace firebase {
25+
// Make sure the Java classes and methods are cached before triggering the
26+
// the callback. Can be slow if this is called BEFORE any Firebase App is
27+
// created. This is currently used by Unity SDK.
28+
void CallAfterEnsureMethodsCached(JNIEnv* env, jobject activity,
29+
std::function<void()> callback);
30+
} // namespace firebase
31+
32+
#endif // FIREBASE_APP_SRC_APP_ANDROID_H_

app/src/app_common.cc

Lines changed: 38 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ struct AppData {
190190
// Tracks library registrations.
191191
class LibraryRegistry {
192192
private:
193-
LibraryRegistry() {}
193+
LibraryRegistry() : is_common_library_registered(false) {}
194194

195195
public:
196196
// Register a library, returns true if the library version changed.
@@ -252,16 +252,30 @@ class LibraryRegistry {
252252
}
253253
}
254254

255+
static bool IsCommonLibraryRegistered() {
256+
if (library_registry_) {
257+
return library_registry_->is_common_library_registered;
258+
}
259+
return false;
260+
}
261+
262+
static void SetCommonLibraryRegistered() {
263+
if (library_registry_) {
264+
library_registry_->is_common_library_registered = true;
265+
}
266+
}
267+
255268
private:
256269
std::map<std::string, std::string> library_to_version_;
257270
std::string user_agent_;
271+
bool is_common_library_registered;
258272

259273
static LibraryRegistry* library_registry_;
260274
};
261275

262276
// Guards g_apps and g_default_app.
263277
static Mutex* g_app_mutex = new Mutex();
264-
static std::map<std::string, UniquePtr<AppData>>* g_apps;
278+
static std::map<std::string, UniquePtr<AppData>>* g_apps = nullptr;
265279
static App* g_default_app = nullptr;
266280
LibraryRegistry* LibraryRegistry::library_registry_ = nullptr;
267281

@@ -294,21 +308,6 @@ App* AddApp(App* app, std::map<std::string, InitResult>* results) {
294308
app_options.storage_bucket(), app_options.project_id(),
295309
static_cast<int>(reinterpret_cast<intptr_t>(app)));
296310
}
297-
LibraryRegistry::Initialize();
298-
if (created_first_app) {
299-
// This calls the platform specific method to propagate the registration to
300-
// any SDKs in use by this library.
301-
App::RegisterLibrary(FIREBASE_CPP_USER_AGENT_PREFIX,
302-
FIREBASE_VERSION_NUMBER_STRING);
303-
App::RegisterLibrary(FIREBASE_CPP_USER_AGENT_PREFIX "-os",
304-
kOperatingSystem);
305-
App::RegisterLibrary(FIREBASE_CPP_USER_AGENT_PREFIX "-arch",
306-
kCpuArchitecture);
307-
App::RegisterLibrary(FIREBASE_CPP_USER_AGENT_PREFIX "-stl",
308-
kCppRuntimeOrStl);
309-
App::RegisterLibrary(FIREBASE_CPP_USER_AGENT_PREFIX "-buildsrc",
310-
kBuildSource);
311-
}
312311
callback::Initialize();
313312
AppCallback::NotifyAllAppCreated(app, results);
314313
return app;
@@ -433,6 +432,28 @@ void RegisterLibrariesFromUserAgent(const char* user_agent) {
433432
if (changed) registry->UpdateUserAgent();
434433
}
435434

435+
void RegisterSdkUsage(void* platform_resource) {
436+
MutexLock lock(*g_app_mutex);
437+
438+
// Only register libraries when no C++ apps was created before.
439+
if (!LibraryRegistry::IsCommonLibraryRegistered()) {
440+
LibraryRegistry::Initialize();
441+
// This calls the platform specific method to propagate the registration to
442+
// any SDKs in use by this library.
443+
App::RegisterLibrary(FIREBASE_CPP_USER_AGENT_PREFIX,
444+
FIREBASE_VERSION_NUMBER_STRING, platform_resource);
445+
App::RegisterLibrary(FIREBASE_CPP_USER_AGENT_PREFIX "-os", kOperatingSystem,
446+
platform_resource);
447+
App::RegisterLibrary(FIREBASE_CPP_USER_AGENT_PREFIX "-arch",
448+
kCpuArchitecture, platform_resource);
449+
App::RegisterLibrary(FIREBASE_CPP_USER_AGENT_PREFIX "-stl",
450+
kCppRuntimeOrStl, platform_resource);
451+
App::RegisterLibrary(FIREBASE_CPP_USER_AGENT_PREFIX "-buildsrc",
452+
kBuildSource, platform_resource);
453+
LibraryRegistry::SetCommonLibraryRegistered();
454+
}
455+
}
456+
436457
const char* GetUserAgent() {
437458
MutexLock lock(*g_app_mutex);
438459
return LibraryRegistry::Initialize()->GetUserAgent();

app/src/app_common.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,10 @@ void RegisterLibrary(const char* library, const char* version);
8080
// This is useful when this SDK wraps other SDKs.
8181
void RegisterLibrariesFromUserAgent(const char* user_agent);
8282

83+
// Register all user-agents related to C++ SDK usages.
84+
// platform_resource currently is only used for passing JNIEnv on Android
85+
void RegisterSdkUsage(void* platform_resource);
86+
8387
// Get the user agent string for all registered libraries.
8488
// This is not thread safe w.r.t RegisterLibrary().
8589
// NOTE: This is an internal method, use App::UserAgent() instead.

app/src/app_desktop.cc

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,9 @@ App* App::Create(const AppOptions& options, const char* name) { // NOLINT
137137

138138
AppOptions options_with_defaults = options;
139139
if (options_with_defaults.PopulateRequiredWithDefaults()) {
140+
// Register C++/Unity user-agents
141+
app_common::RegisterSdkUsage(nullptr);
142+
140143
app = new App();
141144
app->name_ = name;
142145
app->options_ = options_with_defaults;
@@ -168,7 +171,8 @@ internal::FunctionRegistry* App::function_registry() {
168171
}
169172
#endif // INTERNAL_EXPERIMENTAL
170173

171-
void App::RegisterLibrary(const char* library, const char* version) {
174+
void App::RegisterLibrary(const char* library, const char* version,
175+
void* /* platform_resource */) {
172176
app_common::RegisterLibrary(library, version);
173177
}
174178

app/src/app_ios.mm

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,10 @@ static void PlatformOptionsToAppOptions(FIROptions* platform_options, AppOptions
252252
LogError("App %s already created, options will not be applied.", name);
253253
return app;
254254
}
255+
256+
// Register C++/Unity user-agents before creating iOS app.
257+
app_common::RegisterSdkUsage(nullptr);
258+
255259
LogDebug("Creating Firebase App %s for %s", name, kFirebaseVersionString);
256260
app = new App();
257261
app->options_ = options;
@@ -277,7 +281,7 @@ static void PlatformOptionsToAppOptions(FIROptions* platform_options, AppOptions
277281

278282
App* App::GetInstance(const char* name) { return app_common::FindAppByName(name); }
279283

280-
void App::RegisterLibrary(const char* library, const char* version) {
284+
void App::RegisterLibrary(const char* library, const char* version, void* /* platform_resource */) {
281285
[FIRApp registerLibrary:@(library) withVersion:@(version)];
282286
app_common::RegisterLibrariesFromUserAgent(
283287
util::NSStringToString([FIRApp firebaseUserAgent]).c_str());

app/src/app_stub.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,8 @@ internal::FunctionRegistry* App::function_registry() {
7272
}
7373
#endif
7474

75-
void App::RegisterLibrary(const char* library, const char* version) {
75+
void App::RegisterLibrary(const char* library, const char* version,
76+
void* /* platform_resource */) {
7677
app_common::RegisterLibrary(library, version);
7778
}
7879

app/src/include/firebase/app.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -697,7 +697,10 @@ class App {
697697
/// @param library Name of the library to register as a user of the Firebase
698698
/// C++ SDK.
699699
/// @param version Version of the library being registered.
700-
static void RegisterLibrary(const char* library, const char* version);
700+
/// @param platform_resource Platform specific resource. Ex. for Android, this
701+
/// is JNIEnv.
702+
static void RegisterLibrary(const char* library, const char* version,
703+
void* platform_resource);
701704

702705
// Internal method to retrieve the combined string of registered libraries.
703706
static const char* GetUserAgent();

app/src/util_android.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1126,7 +1126,6 @@ jint AttachCurrentThread(JavaVM* java_vm, JNIEnv** env);
11261126
// firebase::App, either the default App (if it exists) or any valid
11271127
// App. If there is no instantiated App, returns nullptr.
11281128
JNIEnv* GetJNIEnvFromApp();
1129-
11301129
} // namespace util
11311130
// NOLINTNEXTLINE - allow namespace overridden
11321131
} // namespace firebase

app/tests/app_test.cc

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -498,7 +498,12 @@ TEST_F(AppTest, TestRegisterLibrary) {
498498
ContainsRegex("fire-cpp-arch/[^ ]+"));
499499
EXPECT_THAT(std::string(App::GetUserAgent()),
500500
ContainsRegex("fire-cpp-stl/[^ ]+"));
501-
App::RegisterLibrary("fire-testing", "1.2.3");
501+
#if FIREBASE_PLATFORM_ANDROID
502+
App::RegisterLibrary("fire-testing", "1.2.3",
503+
firebase_app_default->GetJNIEnv());
504+
#else
505+
App::RegisterLibrary("fire-testing", "1.2.3", nullptr);
506+
#endif
502507
EXPECT_THAT(std::string(App::GetUserAgent()),
503508
HasSubstr("fire-testing/1.2.3"));
504509
firebase_app_default.reset(nullptr);

firestore/src/main/firestore_main.cc

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,11 @@ FirestoreInternal::FirestoreInternal(
110110
"com.google.firebase.firestore.transaction", /*threads=*/5))) {
111111
ApplyDefaultSettings();
112112

113-
App::RegisterLibrary("fire-fst", kFirestoreVersionString);
113+
#if FIREBASE_PLATFORM_ANDROID
114+
App::RegisterLibrary("fire-fst", kFirestoreVersionString, app->GetJNIEnv());
115+
#else
116+
App::RegisterLibrary("fire-fst", kFirestoreVersionString, nullptr);
117+
#endif
114118
}
115119

116120
FirestoreInternal::~FirestoreInternal() {

0 commit comments

Comments
 (0)