Skip to content

Commit b495952

Browse files
DellaBittaDellaBitta
authored
Feature/admob 2021 stability in future usage (#762)
Fix stability issues regarding race conditions in the AdMob SDK: - Ensure that future objects are fully created before launching async operations which use their handles. - Remove the blocking destructor of BannerView on Android. - There was a race condition where the the future might be destroyed before the destroy method returns from Android causing an assertion in the Future implementation. - Remove the blocking destructors of Interstitial and Rewarded Ads on iOS. - These implementations don't invoke AdMob objects and so don't need to be run on the main thread.
1 parent dc996e0 commit b495952

10 files changed

+175
-138
lines changed

‎admob/integration_test/src/integration_test.cc

+2
Original file line numberDiff line numberDiff line change
@@ -795,6 +795,8 @@ TEST_F(FirebaseAdMobTest, TestBannerView) {
795795
// Set the listener.
796796
TestBoundingBoxListener bounding_box_listener;
797797
banner->SetBoundingBoxListener(&bounding_box_listener);
798+
PauseForVisualInspectionAndCallbacks();
799+
798800
int expected_num_bounding_box_changes = 0;
799801
EXPECT_EQ(expected_num_bounding_box_changes,
800802
bounding_box_listener.bounding_box_changes_.size());

‎admob/src/android/banner_view_internal_android.cc

+32-26
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@
3131
#include "admob/src/include/firebase/admob/banner_view.h"
3232
#include "admob/src/include/firebase/admob/types.h"
3333
#include "app/src/assert.h"
34-
#include "app/src/semaphore.h"
3534
#include "app/src/util_android.h"
3635

3736
namespace firebase {
@@ -142,23 +141,17 @@ BannerViewInternalAndroid::BannerViewInternalAndroid(BannerView* base)
142141
env->DeleteLocalRef(helper_ref);
143142
}
144143

145-
void DestroyOnDeleteCallback(const Future<void>& result, void* sem_data) {
146-
if (sem_data != nullptr) {
147-
Semaphore* semaphore = static_cast<Semaphore*>(sem_data);
148-
semaphore->Post();
149-
}
150-
}
151-
152144
BannerViewInternalAndroid::~BannerViewInternalAndroid() {
153145
firebase::MutexLock lock(mutex_);
154-
155-
Semaphore semaphore(0);
156-
InvokeNullary(kBannerViewFnDestroyOnDelete, banner_view_helper::kDestroy)
157-
.OnCompletion(DestroyOnDeleteCallback, &semaphore);
158-
semaphore.Wait();
159-
160146
JNIEnv* env = ::firebase::admob::GetJNI();
161147

148+
// The application should have invoked Destroy already, but
149+
// invoke it now just in case they haven't in the hope that
150+
// we can prevent leaking memory.
151+
env->CallVoidMethod(
152+
helper_, banner_view_helper::GetMethodId(banner_view_helper::kDestroy),
153+
/*callbackDataPtr=*/nullptr, /*destructor_invocation=*/true);
154+
162155
env->DeleteGlobalRef(ad_view_);
163156
ad_view_ = nullptr;
164157

@@ -257,16 +250,19 @@ Future<void> BannerViewInternalAndroid::Initialize(AdParent parent,
257250
if (initialized_) {
258251
const SafeFutureHandle<void> future_handle =
259252
future_data_.future_impl.SafeAlloc<void>(kBannerViewFnInitialize);
253+
Future<void> future = MakeFuture(&future_data_.future_impl, future_handle);
260254
CompleteFuture(kAdMobErrorAlreadyInitialized,
261255
kAdAlreadyInitializedErrorMessage, future_handle,
262256
&future_data_);
263-
return MakeFuture(&future_data_.future_impl, future_handle);
257+
return future;
264258
}
265259

266260
initialized_ = true;
267261

268262
FutureCallbackData<void>* callback_data =
269263
CreateVoidFutureCallbackData(kBannerViewFnSetPosition, &future_data_);
264+
Future<void> future =
265+
MakeFuture(&future_data_.future_impl, callback_data->future_handle);
270266

271267
JNIEnv* env = ::firebase::admob::GetJNI();
272268
FIREBASE_ASSERT(env);
@@ -281,8 +277,7 @@ Future<void> BannerViewInternalAndroid::Initialize(AdParent parent,
281277
call_data->callback_data = callback_data;
282278
util::RunOnMainThread(env, activity, InitializeBannerViewOnMainThread,
283279
call_data);
284-
285-
return MakeFuture(&future_data_.future_impl, callback_data->future_handle);
280+
return future;
286281
}
287282

288283
// This function is run on the main thread and is called in the
@@ -324,7 +319,8 @@ Future<AdResult> BannerViewInternalAndroid::LoadAd(const AdRequest& request) {
324319

325320
FutureCallbackData<AdResult>* callback_data =
326321
CreateAdResultFutureCallbackData(kBannerViewFnLoadAd, &future_data_);
327-
SafeFutureHandle<AdResult> future_handle = callback_data->future_handle;
322+
Future<AdResult> future =
323+
MakeFuture(&future_data_.future_impl, callback_data->future_handle);
328324

329325
LoadAdOnMainThreadData* call_data = new LoadAdOnMainThreadData();
330326
call_data->ad_request = request;
@@ -334,7 +330,7 @@ Future<AdResult> BannerViewInternalAndroid::LoadAd(const AdRequest& request) {
334330
jobject activity = ::firebase::admob::GetActivity();
335331
util::RunOnMainThread(env, activity, LoadAdOnMainThread, call_data);
336332

337-
return MakeFuture(&future_data_.future_impl, future_handle);
333+
return future;
338334
}
339335

340336
BoundingBox BannerViewInternalAndroid::bounding_box() const {
@@ -390,21 +386,29 @@ Future<void> BannerViewInternalAndroid::Resume() {
390386

391387
Future<void> BannerViewInternalAndroid::Destroy() {
392388
firebase::MutexLock lock(mutex_);
393-
return InvokeNullary(kBannerViewFnDestroy, banner_view_helper::kDestroy);
389+
FutureCallbackData<void>* callback_data =
390+
CreateVoidFutureCallbackData(kBannerViewFnDestroy, &future_data_);
391+
Future<void> future =
392+
MakeFuture(&future_data_.future_impl, callback_data->future_handle);
393+
::firebase::admob::GetJNI()->CallVoidMethod(
394+
helper_, banner_view_helper::GetMethodId(banner_view_helper::kDestroy),
395+
reinterpret_cast<jlong>(callback_data), /*destructor_invocation=*/false);
396+
return future;
394397
}
395398

396399
Future<void> BannerViewInternalAndroid::SetPosition(int x, int y) {
397400
firebase::MutexLock lock(mutex_);
398401

399402
FutureCallbackData<void>* callback_data =
400403
CreateVoidFutureCallbackData(kBannerViewFnSetPosition, &future_data_);
401-
SafeFutureHandle<void> future_handle = callback_data->future_handle;
404+
Future<void> future =
405+
MakeFuture(&future_data_.future_impl, callback_data->future_handle);
402406

403407
::firebase::admob::GetJNI()->CallVoidMethod(
404408
helper_, banner_view_helper::GetMethodId(banner_view_helper::kMoveToXY),
405409
reinterpret_cast<jlong>(callback_data), x, y);
406410

407-
return MakeFuture(&future_data_.future_impl, future_handle);
411+
return future;
408412
}
409413

410414
Future<void> BannerViewInternalAndroid::SetPosition(
@@ -413,14 +417,15 @@ Future<void> BannerViewInternalAndroid::SetPosition(
413417

414418
FutureCallbackData<void>* callback_data =
415419
CreateVoidFutureCallbackData(kBannerViewFnSetPosition, &future_data_);
416-
SafeFutureHandle<void> future_handle = callback_data->future_handle;
420+
Future<void> future =
421+
MakeFuture(&future_data_.future_impl, callback_data->future_handle);
417422

418423
::firebase::admob::GetJNI()->CallVoidMethod(
419424
helper_,
420425
banner_view_helper::GetMethodId(banner_view_helper::kMoveToPosition),
421426
reinterpret_cast<jlong>(callback_data), static_cast<int>(position));
422427

423-
return MakeFuture(&future_data_.future_impl, future_handle);
428+
return future;
424429
}
425430

426431
// This function is run on the main thread and is called in the
@@ -449,7 +454,8 @@ Future<void> BannerViewInternalAndroid::InvokeNullary(
449454

450455
FutureCallbackData<void>* callback_data =
451456
CreateVoidFutureCallbackData(fn, &future_data_);
452-
SafeFutureHandle<void> future_handle = callback_data->future_handle;
457+
Future<void> future =
458+
MakeFuture(&future_data_.future_impl, callback_data->future_handle);
453459

454460
NulleryInvocationOnMainThreadData* call_data =
455461
new NulleryInvocationOnMainThreadData();
@@ -459,7 +465,7 @@ Future<void> BannerViewInternalAndroid::InvokeNullary(
459465

460466
util::RunOnMainThread(env, activity, InvokeNulleryOnMainThread, call_data);
461467

462-
return MakeFuture(&future_data_.future_impl, future_handle);
468+
return future;
463469
}
464470

465471
} // namespace internal

‎admob/src/android/banner_view_internal_android.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ namespace admob {
3535
X(Show, "show", "(J)V"), \
3636
X(Pause, "pause", "(J)V"), \
3737
X(Resume, "resume", "(J)V"), \
38-
X(Destroy, "destroy", "(J)V"), \
38+
X(Destroy, "destroy", "(JZ)V"), \
3939
X(MoveToPosition, "moveTo", "(JI)V"), \
4040
X(MoveToXY, "moveTo", "(JII)V"), \
4141
X(GetBoundingBox, "getBoundingBox", "()[I"), \

‎admob/src/android/interstitial_ad_internal_android.cc

+18-10
Original file line numberDiff line numberDiff line change
@@ -76,10 +76,11 @@ Future<void> InterstitialAdInternalAndroid::Initialize(AdParent parent) {
7676
if (initialized_) {
7777
const SafeFutureHandle<void> future_handle =
7878
future_data_.future_impl.SafeAlloc<void>(kInterstitialAdFnInitialize);
79+
Future<void> future = MakeFuture(&future_data_.future_impl, future_handle);
7980
CompleteFuture(kAdMobErrorAlreadyInitialized,
8081
kAdAlreadyInitializedErrorMessage, future_handle,
8182
&future_data_);
82-
return MakeFuture(&future_data_.future_impl, future_handle);
83+
return future;
8384
}
8485

8586
initialized_ = true;
@@ -88,23 +89,27 @@ Future<void> InterstitialAdInternalAndroid::Initialize(AdParent parent) {
8889

8990
FutureCallbackData<void>* callback_data =
9091
CreateVoidFutureCallbackData(kInterstitialAdFnInitialize, &future_data_);
92+
Future<void> future =
93+
MakeFuture(&future_data_.future_impl, callback_data->future_handle);
9194
env->CallVoidMethod(
9295
helper_,
9396
interstitial_ad_helper::GetMethodId(interstitial_ad_helper::kInitialize),
9497
reinterpret_cast<jlong>(callback_data), parent);
95-
return MakeFuture(&future_data_.future_impl, callback_data->future_handle);
98+
return future;
9699
}
97100

98101
Future<AdResult> InterstitialAdInternalAndroid::LoadAd(
99102
const char* ad_unit_id, const AdRequest& request) {
100103
firebase::MutexLock lock(mutex_);
101104

102105
if (!initialized_) {
103-
SafeFutureHandle<AdResult> handle =
106+
SafeFutureHandle<AdResult> future_handle =
104107
CreateFuture<AdResult>(kInterstitialAdFnLoadAd, &future_data_);
108+
Future<AdResult> future =
109+
MakeFuture(&future_data_.future_impl, future_handle);
105110
CompleteFuture(kAdMobErrorUninitialized, kAdUninitializedErrorMessage,
106-
handle, &future_data_, AdResult());
107-
return MakeFuture(&future_data_.future_impl, handle);
111+
future_handle, &future_data_, AdResult());
112+
return future;
108113
}
109114

110115
admob::AdMobError error = kAdMobErrorNone;
@@ -121,6 +126,8 @@ Future<AdResult> InterstitialAdInternalAndroid::LoadAd(
121126
FIREBASE_ASSERT(env);
122127
FutureCallbackData<AdResult>* callback_data =
123128
CreateAdResultFutureCallbackData(kInterstitialAdFnLoadAd, &future_data_);
129+
Future<AdResult> future =
130+
MakeFuture(&future_data_.future_impl, callback_data->future_handle);
124131

125132
jstring j_ad_unit_str = env->NewStringUTF(ad_unit_id);
126133
::firebase::admob::GetJNI()->CallVoidMethod(
@@ -129,30 +136,31 @@ Future<AdResult> InterstitialAdInternalAndroid::LoadAd(
129136
reinterpret_cast<jlong>(callback_data), j_ad_unit_str, j_request);
130137
env->DeleteLocalRef(j_ad_unit_str);
131138
env->DeleteLocalRef(j_request);
132-
133-
return MakeFuture(&future_data_.future_impl, callback_data->future_handle);
139+
return future;
134140
}
135141

136142
Future<void> InterstitialAdInternalAndroid::Show() {
137143
firebase::MutexLock lock(mutex_);
138144
if (!initialized_) {
139145
const SafeFutureHandle<void> future_handle =
140146
future_data_.future_impl.SafeAlloc<void>(kInterstitialAdFnShow);
147+
Future<void> future = MakeFuture(&future_data_.future_impl, future_handle);
141148
CompleteFuture(kAdMobErrorUninitialized, kAdUninitializedErrorMessage,
142149
future_handle, &future_data_);
143-
return MakeFuture(&future_data_.future_impl, future_handle);
150+
return future;
144151
}
145152

146153
FutureCallbackData<void>* callback_data =
147154
CreateVoidFutureCallbackData(kInterstitialAdFnShow, &future_data_);
148-
SafeFutureHandle<void> future_handle = callback_data->future_handle;
155+
Future<void> future =
156+
MakeFuture(&future_data_.future_impl, callback_data->future_handle);
149157

150158
::firebase::admob::GetJNI()->CallVoidMethod(
151159
helper_,
152160
interstitial_ad_helper::GetMethodId(interstitial_ad_helper::kShow),
153161
reinterpret_cast<jlong>(callback_data));
154162

155-
return MakeFuture(&future_data_.future_impl, future_handle);
163+
return future;
156164
}
157165

158166
} // namespace internal

‎admob/src/android/rewarded_ad_internal_android.cc

+18-9
Original file line numberDiff line numberDiff line change
@@ -79,35 +79,40 @@ Future<void> RewardedAdInternalAndroid::Initialize(AdParent parent) {
7979
if (initialized_) {
8080
const SafeFutureHandle<void> future_handle =
8181
future_data_.future_impl.SafeAlloc<void>(kRewardedAdFnInitialize);
82+
Future<void> future = MakeFuture(&future_data_.future_impl, future_handle);
8283
CompleteFuture(kAdMobErrorAlreadyInitialized,
8384
kAdAlreadyInitializedErrorMessage, future_handle,
8485
&future_data_);
85-
return MakeFuture(&future_data_.future_impl, future_handle);
86+
return future;
8687
}
8788

8889
initialized_ = true;
8990
FutureCallbackData<void>* callback_data =
9091
CreateVoidFutureCallbackData(kRewardedAdFnInitialize, &future_data_);
92+
Future<void> future =
93+
MakeFuture(&future_data_.future_impl, callback_data->future_handle);
9194

9295
JNIEnv* env = ::firebase::admob::GetJNI();
9396
FIREBASE_ASSERT(env);
9497
env->CallVoidMethod(
9598
helper_, rewarded_ad_helper::GetMethodId(rewarded_ad_helper::kInitialize),
9699
reinterpret_cast<jlong>(callback_data), parent);
97100
util::CheckAndClearJniExceptions(env);
98-
return MakeFuture(&future_data_.future_impl, callback_data->future_handle);
101+
return future;
99102
}
100103

101104
Future<AdResult> RewardedAdInternalAndroid::LoadAd(const char* ad_unit_id,
102105
const AdRequest& request) {
103106
firebase::MutexLock lock(mutex_);
104107
if (!initialized_) {
105-
SafeFutureHandle<AdResult> handle =
108+
SafeFutureHandle<AdResult> future_handle =
106109
future_data_.future_impl.SafeAlloc<AdResult>(kRewardedAdFnLoadAd,
107110
AdResult());
111+
Future<AdResult> future =
112+
MakeFuture(&future_data_.future_impl, future_handle);
108113
CompleteFuture(kAdMobErrorUninitialized, kAdUninitializedErrorMessage,
109-
handle, &future_data_, AdResult());
110-
return MakeFuture(&future_data_.future_impl, handle);
114+
future_handle, &future_data_, AdResult());
115+
return future;
111116
}
112117

113118
admob::AdMobError error = kAdMobErrorNone;
@@ -122,6 +127,8 @@ Future<AdResult> RewardedAdInternalAndroid::LoadAd(const char* ad_unit_id,
122127
}
123128
FutureCallbackData<AdResult>* callback_data =
124129
CreateAdResultFutureCallbackData(kRewardedAdFnLoadAd, &future_data_);
130+
Future<AdResult> future =
131+
MakeFuture(&future_data_.future_impl, callback_data->future_handle);
125132

126133
JNIEnv* env = GetJNI();
127134
FIREBASE_ASSERT(env);
@@ -133,7 +140,7 @@ Future<AdResult> RewardedAdInternalAndroid::LoadAd(const char* ad_unit_id,
133140
env->DeleteLocalRef(j_ad_unit_str);
134141
env->DeleteLocalRef(j_request);
135142

136-
return MakeFuture(&future_data_.future_impl, callback_data->future_handle);
143+
return future;
137144
}
138145

139146
Future<void> RewardedAdInternalAndroid::Show(
@@ -142,16 +149,18 @@ Future<void> RewardedAdInternalAndroid::Show(
142149
if (!initialized_) {
143150
const SafeFutureHandle<void> future_handle =
144151
future_data_.future_impl.SafeAlloc<void>(kRewardedAdFnShow);
152+
Future<void> future = MakeFuture(&future_data_.future_impl, future_handle);
145153
CompleteFuture(kAdMobErrorUninitialized, kAdUninitializedErrorMessage,
146154
future_handle, &future_data_);
147-
return MakeFuture(&future_data_.future_impl, future_handle);
155+
return future;
148156
}
149157

150158
user_earned_reward_listener_ = listener;
151159

152160
FutureCallbackData<void>* callback_data =
153161
CreateVoidFutureCallbackData(kRewardedAdFnShow, &future_data_);
154-
SafeFutureHandle<void> future_handle = callback_data->future_handle;
162+
Future<void> future =
163+
MakeFuture(&future_data_.future_impl, callback_data->future_handle);
155164

156165
JNIEnv* env = GetJNI();
157166
FIREBASE_ASSERT(env);
@@ -167,7 +176,7 @@ Future<void> RewardedAdInternalAndroid::Show(
167176
env->DeleteLocalRef(j_verification_custom_data);
168177
env->DeleteLocalRef(j_verification_user_id);
169178

170-
return MakeFuture(&future_data_.future_impl, future_handle);
179+
return future;
171180
}
172181

173182
} // namespace internal

‎admob/src/common/admob_common.cc

+3-1
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,9 @@ const char* GetRequestAgentString() {
227227
// Mark a future as complete.
228228
void CompleteFuture(int error, const char* error_msg,
229229
SafeFutureHandle<void> handle, FutureData* future_data) {
230-
future_data->future_impl.Complete(handle, error, error_msg);
230+
if (future_data->future_impl.ValidFuture(handle)) {
231+
future_data->future_impl.Complete(handle, error, error_msg);
232+
}
231233
}
232234

233235
// For calls that aren't asynchronous, we can create and complete at the

0 commit comments

Comments
 (0)