Skip to content

Feature/admob 2021 stability in future usage #762

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.

Sign up for GitHub

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

Merged
merged 14 commits into from
Nov 29, 2021
Merged

Feature/admob 2021 stability in future usage #762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
4eabdf0
construct future objects before calling async methods
DellaBitta Nov 27, 2021
b4251c9
remove async nature of bannerview destructor
DellaBitta Nov 28, 2021
6673e59
Errant character in BannerViewHelper.java
DellaBitta Nov 28, 2021
e2cea9d
misuse of Android Studio formatter
DellaBitta Nov 28, 2021
c03e414
support destroy from destructor and diret invocation
DellaBitta Nov 28, 2021
591a658
Java changes
DellaBitta Nov 28, 2021
40ef248
future callback fix in Java code
DellaBitta Nov 28, 2021
9c19e08
line lengh in bannerview android header
DellaBitta Nov 29, 2021
d73c995
reduce the complexity of interstitial destructor on iOS
DellaBitta Nov 29, 2021
37be813
Merge branch 'feature/admob_2021' into feature/admob_2021_make_future
DellaBitta Nov 29, 2021
7c1c6cf
reduce complexity of rewarded ads destructor on iOS
DellaBitta Nov 29, 2021
ef47ba6
Pause to ensure the bounding box listener isn't called
DellaBitta Nov 29, 2021
e79a87f
BannerViewHelper.java formatting
DellaBitta Nov 29, 2021
5cd408e
errant character
DellaBitta Nov 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions admob/integration_test/src/integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -795,6 +795,8 @@ TEST_F(FirebaseAdMobTest, TestBannerView) {
// Set the listener.
TestBoundingBoxListener bounding_box_listener;
banner->SetBoundingBoxListener(&bounding_box_listener);
PauseForVisualInspectionAndCallbacks();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait to ensure that any callback attempts have time to complete before we start testing the listener.


int expected_num_bounding_box_changes = 0;
EXPECT_EQ(expected_num_bounding_box_changes,
bounding_box_listener.bounding_box_changes_.size());
Expand Down
58 changes: 32 additions & 26 deletions admob/src/android/banner_view_internal_android.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
#include "admob/src/include/firebase/admob/banner_view.h"
#include "admob/src/include/firebase/admob/types.h"
#include "app/src/assert.h"
#include "app/src/semaphore.h"
#include "app/src/util_android.h"

namespace firebase {
Expand Down Expand Up @@ -142,23 +141,17 @@ BannerViewInternalAndroid::BannerViewInternalAndroid(BannerView* base)
env->DeleteLocalRef(helper_ref);
}

void DestroyOnDeleteCallback(const Future<void>& result, void* sem_data) {
if (sem_data != nullptr) {
Semaphore* semaphore = static_cast<Semaphore*>(sem_data);
semaphore->Post();
}
}

BannerViewInternalAndroid::~BannerViewInternalAndroid() {
firebase::MutexLock lock(mutex_);

Semaphore semaphore(0);
InvokeNullary(kBannerViewFnDestroyOnDelete, banner_view_helper::kDestroy)
.OnCompletion(DestroyOnDeleteCallback, &semaphore);
semaphore.Wait();

JNIEnv* env = ::firebase::admob::GetJNI();

// The application should have invoked Destroy already, but
// invoke it now just in case they haven't in the hope that
// we can prevent leaking memory.
env->CallVoidMethod(
helper_, banner_view_helper::GetMethodId(banner_view_helper::kDestroy),
/*callbackDataPtr=*/nullptr, /*destructor_invocation=*/true);

env->DeleteGlobalRef(ad_view_);
ad_view_ = nullptr;

Expand Down Expand Up @@ -257,16 +250,19 @@ Future<void> BannerViewInternalAndroid::Initialize(AdParent parent,
if (initialized_) {
const SafeFutureHandle<void> future_handle =
future_data_.future_impl.SafeAlloc<void>(kBannerViewFnInitialize);
Future<void> future = MakeFuture(&future_data_.future_impl, future_handle);
CompleteFuture(kAdMobErrorAlreadyInitialized,
kAdAlreadyInitializedErrorMessage, future_handle,
&future_data_);
return MakeFuture(&future_data_.future_impl, future_handle);
return future;
}

initialized_ = true;

FutureCallbackData<void>* callback_data =
CreateVoidFutureCallbackData(kBannerViewFnSetPosition, &future_data_);
Future<void> future =
MakeFuture(&future_data_.future_impl, callback_data->future_handle);

JNIEnv* env = ::firebase::admob::GetJNI();
FIREBASE_ASSERT(env);
Expand All @@ -281,8 +277,7 @@ Future<void> BannerViewInternalAndroid::Initialize(AdParent parent,
call_data->callback_data = callback_data;
util::RunOnMainThread(env, activity, InitializeBannerViewOnMainThread,
call_data);

return MakeFuture(&future_data_.future_impl, callback_data->future_handle);
return future;
}

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

FutureCallbackData<AdResult>* callback_data =
CreateAdResultFutureCallbackData(kBannerViewFnLoadAd, &future_data_);
SafeFutureHandle<AdResult> future_handle = callback_data->future_handle;
Future<AdResult> future =
MakeFuture(&future_data_.future_impl, callback_data->future_handle);

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

return MakeFuture(&future_data_.future_impl, future_handle);
return future;
}

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

Future<void> BannerViewInternalAndroid::Destroy() {
firebase::MutexLock lock(mutex_);
return InvokeNullary(kBannerViewFnDestroy, banner_view_helper::kDestroy);
FutureCallbackData<void>* callback_data =
Copy link
Contributor Author

@DellaBitta DellaBitta Nov 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

No longer use futures to track the destruction as the ReferenceCountedFutureImpl may be destroyed before the operation attempts to complete the future. This had caused a crash on some Android runs which was responsible for a lot of flake.

TBH, the Application should have already invoked Destroy() prior to this object's destruction. Invoking it again here isn't required. I guess this code was put here to protect our users from accidentally leaking memory.

In theory this code could be removed, but I'll keep it anyway. But moving forward there doesn't seem to be a good reason to block the destructor and wait, though. Instead I'll start an asynchronous attempt to destroy the Java resources, if they haven't been destroyed already, and then return immediately from here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think there's no reason to actually wait for the Java destruction to complete.

CreateVoidFutureCallbackData(kBannerViewFnDestroy, &future_data_);
Future<void> future =
MakeFuture(&future_data_.future_impl, callback_data->future_handle);
::firebase::admob::GetJNI()->CallVoidMethod(
helper_, banner_view_helper::GetMethodId(banner_view_helper::kDestroy),
reinterpret_cast<jlong>(callback_data), /*destructor_invocation=*/false);
return future;
}

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

FutureCallbackData<void>* callback_data =
CreateVoidFutureCallbackData(kBannerViewFnSetPosition, &future_data_);
SafeFutureHandle<void> future_handle = callback_data->future_handle;
Future<void> future =
MakeFuture(&future_data_.future_impl, callback_data->future_handle);

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

return MakeFuture(&future_data_.future_impl, future_handle);
return future;
}

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

FutureCallbackData<void>* callback_data =
CreateVoidFutureCallbackData(kBannerViewFnSetPosition, &future_data_);
SafeFutureHandle<void> future_handle = callback_data->future_handle;
Future<void> future =
MakeFuture(&future_data_.future_impl, callback_data->future_handle);

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

return MakeFuture(&future_data_.future_impl, future_handle);
return future;
}

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

FutureCallbackData<void>* callback_data =
CreateVoidFutureCallbackData(fn, &future_data_);
SafeFutureHandle<void> future_handle = callback_data->future_handle;
Future<void> future =
MakeFuture(&future_data_.future_impl, callback_data->future_handle);

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

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

return MakeFuture(&future_data_.future_impl, future_handle);
return future;
}

} // namespace internal
Expand Down
2 changes: 1 addition & 1 deletion admob/src/android/banner_view_internal_android.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ namespace admob {
X(Show, "show", "(J)V"), \
X(Pause, "pause", "(J)V"), \
X(Resume, "resume", "(J)V"), \
X(Destroy, "destroy", "(J)V"), \
X(Destroy, "destroy", "(JZ)V"), \
X(MoveToPosition, "moveTo", "(JI)V"), \
X(MoveToXY, "moveTo", "(JII)V"), \
X(GetBoundingBox, "getBoundingBox", "()[I"), \
Expand Down
28 changes: 18 additions & 10 deletions admob/src/android/interstitial_ad_internal_android.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,11 @@ Future<void> InterstitialAdInternalAndroid::Initialize(AdParent parent) {
if (initialized_) {
const SafeFutureHandle<void> future_handle =
future_data_.future_impl.SafeAlloc<void>(kInterstitialAdFnInitialize);
Future<void> future = MakeFuture(&future_data_.future_impl, future_handle);
CompleteFuture(kAdMobErrorAlreadyInitialized,
kAdAlreadyInitializedErrorMessage, future_handle,
&future_data_);
return MakeFuture(&future_data_.future_impl, future_handle);
return future;
}

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

FutureCallbackData<void>* callback_data =
CreateVoidFutureCallbackData(kInterstitialAdFnInitialize, &future_data_);
Future<void> future =
MakeFuture(&future_data_.future_impl, callback_data->future_handle);
env->CallVoidMethod(
helper_,
interstitial_ad_helper::GetMethodId(interstitial_ad_helper::kInitialize),
reinterpret_cast<jlong>(callback_data), parent);
return MakeFuture(&future_data_.future_impl, callback_data->future_handle);
return future;
}

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

if (!initialized_) {
SafeFutureHandle<AdResult> handle =
SafeFutureHandle<AdResult> future_handle =
CreateFuture<AdResult>(kInterstitialAdFnLoadAd, &future_data_);
Future<AdResult> future =
MakeFuture(&future_data_.future_impl, future_handle);
CompleteFuture(kAdMobErrorUninitialized, kAdUninitializedErrorMessage,
handle, &future_data_, AdResult());
return MakeFuture(&future_data_.future_impl, handle);
future_handle, &future_data_, AdResult());
return future;
}

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

jstring j_ad_unit_str = env->NewStringUTF(ad_unit_id);
::firebase::admob::GetJNI()->CallVoidMethod(
Expand All @@ -129,30 +136,31 @@ Future<AdResult> InterstitialAdInternalAndroid::LoadAd(
reinterpret_cast<jlong>(callback_data), j_ad_unit_str, j_request);
env->DeleteLocalRef(j_ad_unit_str);
env->DeleteLocalRef(j_request);

return MakeFuture(&future_data_.future_impl, callback_data->future_handle);
return future;
}

Future<void> InterstitialAdInternalAndroid::Show() {
firebase::MutexLock lock(mutex_);
if (!initialized_) {
const SafeFutureHandle<void> future_handle =
future_data_.future_impl.SafeAlloc<void>(kInterstitialAdFnShow);
Future<void> future = MakeFuture(&future_data_.future_impl, future_handle);
CompleteFuture(kAdMobErrorUninitialized, kAdUninitializedErrorMessage,
future_handle, &future_data_);
return MakeFuture(&future_data_.future_impl, future_handle);
return future;
}

FutureCallbackData<void>* callback_data =
CreateVoidFutureCallbackData(kInterstitialAdFnShow, &future_data_);
SafeFutureHandle<void> future_handle = callback_data->future_handle;
Future<void> future =
MakeFuture(&future_data_.future_impl, callback_data->future_handle);

::firebase::admob::GetJNI()->CallVoidMethod(
helper_,
interstitial_ad_helper::GetMethodId(interstitial_ad_helper::kShow),
reinterpret_cast<jlong>(callback_data));

return MakeFuture(&future_data_.future_impl, future_handle);
return future;
}

} // namespace internal
Expand Down
27 changes: 18 additions & 9 deletions admob/src/android/rewarded_ad_internal_android.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,35 +79,40 @@ Future<void> RewardedAdInternalAndroid::Initialize(AdParent parent) {
if (initialized_) {
const SafeFutureHandle<void> future_handle =
future_data_.future_impl.SafeAlloc<void>(kRewardedAdFnInitialize);
Future<void> future = MakeFuture(&future_data_.future_impl, future_handle);
CompleteFuture(kAdMobErrorAlreadyInitialized,
kAdAlreadyInitializedErrorMessage, future_handle,
&future_data_);
return MakeFuture(&future_data_.future_impl, future_handle);
return future;
}

initialized_ = true;
FutureCallbackData<void>* callback_data =
CreateVoidFutureCallbackData(kRewardedAdFnInitialize, &future_data_);
Future<void> future =
MakeFuture(&future_data_.future_impl, callback_data->future_handle);

JNIEnv* env = ::firebase::admob::GetJNI();
FIREBASE_ASSERT(env);
env->CallVoidMethod(
helper_, rewarded_ad_helper::GetMethodId(rewarded_ad_helper::kInitialize),
reinterpret_cast<jlong>(callback_data), parent);
util::CheckAndClearJniExceptions(env);
return MakeFuture(&future_data_.future_impl, callback_data->future_handle);
return future;
}

Future<AdResult> RewardedAdInternalAndroid::LoadAd(const char* ad_unit_id,
const AdRequest& request) {
firebase::MutexLock lock(mutex_);
if (!initialized_) {
SafeFutureHandle<AdResult> handle =
SafeFutureHandle<AdResult> future_handle =
future_data_.future_impl.SafeAlloc<AdResult>(kRewardedAdFnLoadAd,
AdResult());
Future<AdResult> future =
MakeFuture(&future_data_.future_impl, future_handle);
CompleteFuture(kAdMobErrorUninitialized, kAdUninitializedErrorMessage,
handle, &future_data_, AdResult());
return MakeFuture(&future_data_.future_impl, handle);
future_handle, &future_data_, AdResult());
return future;
}

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

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

return MakeFuture(&future_data_.future_impl, callback_data->future_handle);
return future;
}

Future<void> RewardedAdInternalAndroid::Show(
Expand All @@ -142,16 +149,18 @@ Future<void> RewardedAdInternalAndroid::Show(
if (!initialized_) {
const SafeFutureHandle<void> future_handle =
future_data_.future_impl.SafeAlloc<void>(kRewardedAdFnShow);
Future<void> future = MakeFuture(&future_data_.future_impl, future_handle);
CompleteFuture(kAdMobErrorUninitialized, kAdUninitializedErrorMessage,
future_handle, &future_data_);
return MakeFuture(&future_data_.future_impl, future_handle);
return future;
}

user_earned_reward_listener_ = listener;

FutureCallbackData<void>* callback_data =
CreateVoidFutureCallbackData(kRewardedAdFnShow, &future_data_);
SafeFutureHandle<void> future_handle = callback_data->future_handle;
Future<void> future =
MakeFuture(&future_data_.future_impl, callback_data->future_handle);

JNIEnv* env = GetJNI();
FIREBASE_ASSERT(env);
Expand All @@ -167,7 +176,7 @@ Future<void> RewardedAdInternalAndroid::Show(
env->DeleteLocalRef(j_verification_custom_data);
env->DeleteLocalRef(j_verification_user_id);

return MakeFuture(&future_data_.future_impl, future_handle);
return future;
}

} // namespace internal
Expand Down
4 changes: 3 additions & 1 deletion admob/src/common/admob_common.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,9 @@ const char* GetRequestAgentString() {
// Mark a future as complete.
void CompleteFuture(int error, const char* error_msg,
SafeFutureHandle<void> handle, FutureData* future_data) {
future_data->future_impl.Complete(handle, error, error_msg);
if (future_data->future_impl.ValidFuture(handle)) {
future_data->future_impl.Complete(handle, error, error_msg);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just an extra safety check.

}
}

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