Skip to content

Commit 88309ec

Browse files
chkuang-ga-maurice
chkuang-g
authored and
a-maurice
committed
Fix a couple of race issue in Instance Id for Android
* Change CancelOperations() so that it does not hold the lock to operations_mutex_ all the time, which can result in deadlock with background thread. * Change all implementation to pass AsyncOperation pointer to background thread instead of the pointer to SharedPtr<AsyncOperation> in a std::vector. PiperOrigin-RevId: 254826416
1 parent 5dc1e86 commit 88309ec

File tree

3 files changed

+104
-44
lines changed

3 files changed

+104
-44
lines changed

‎instance_id/src/android/instance_id.cc

Lines changed: 61 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ Future<std::string> InstanceId::GetId() const {
121121
if (!instance_id_internal_) return Future<std::string>();
122122

123123
JNIEnv* env = app().GetJNIEnv();
124-
SharedPtr<AsyncOperation>* operation =
124+
SharedPtr<AsyncOperation> operation =
125125
instance_id_internal_->AddOperation(new AsyncOperation(
126126
env, instance_id_internal_,
127127
instance_id_internal_
@@ -130,10 +130,18 @@ Future<std::string> InstanceId::GetId() const {
130130
util::RunOnBackgroundThread(
131131
env,
132132
[](void* function_data) {
133-
SharedPtr<AsyncOperation> operation =
134-
*(static_cast<SharedPtr<AsyncOperation>*>(function_data));
133+
// Assume that when this callback is called, AsyncOperation should still
134+
// be in instance_id_internal->operations_ or this callback should not
135+
// be called at all due to the lock in CppThreadDispatcherContext.
136+
AsyncOperation* op_ptr = static_cast<AsyncOperation*>(function_data);
135137
InstanceIdInternal* instance_id_internal =
136-
operation->instance_id_internal();
138+
op_ptr->instance_id_internal();
139+
// Hold a reference to AsyncOperation so that it will not be deleted
140+
// during this callback.
141+
SharedPtr<AsyncOperation> operation =
142+
instance_id_internal->GetOperationSharedPtr(op_ptr);
143+
if (!operation) return;
144+
137145
JNIEnv* env = instance_id_internal->instance_id()->app().GetJNIEnv();
138146
jobject java_instance_id =
139147
env->NewLocalRef(instance_id_internal->java_instance_id());
@@ -151,16 +159,16 @@ Future<std::string> InstanceId::GetId() const {
151159
error.c_str());
152160
}
153161
},
154-
operation, InstanceIdInternal::CanceledWithResult<std::string>,
155-
&(**operation));
162+
&(*operation), InstanceIdInternal::CanceledWithResult<std::string>,
163+
&(*operation));
156164
return GetIdLastResult();
157165
}
158166

159167
Future<void> InstanceId::DeleteId() {
160168
if (!instance_id_internal_) return Future<void>();
161169

162170
JNIEnv* env = app().GetJNIEnv();
163-
SharedPtr<AsyncOperation>* operation = instance_id_internal_->AddOperation(
171+
SharedPtr<AsyncOperation> operation = instance_id_internal_->AddOperation(
164172
new AsyncOperation(env, instance_id_internal_,
165173
instance_id_internal_
166174
->FutureAlloc<std::string>(
@@ -169,10 +177,17 @@ Future<void> InstanceId::DeleteId() {
169177
util::RunOnBackgroundThread(
170178
env,
171179
[](void* function_data) {
172-
SharedPtr<AsyncOperation> operation =
173-
*(static_cast<SharedPtr<AsyncOperation>*>(function_data));
180+
// Assume that when this callback is called, AsyncOperation should still
181+
// be in instance_id_internal->operations_ or this callback should not
182+
// be called at all due to the lock in CppThreadDispatcherContext.
183+
AsyncOperation* op_ptr = static_cast<AsyncOperation*>(function_data);
174184
InstanceIdInternal* instance_id_internal =
175-
operation->instance_id_internal();
185+
op_ptr->instance_id_internal();
186+
// Hold a reference to AsyncOperation so that it will not be deleted
187+
// during this callback.
188+
SharedPtr<AsyncOperation> operation =
189+
instance_id_internal->GetOperationSharedPtr(op_ptr);
190+
if (!operation) return;
176191
JNIEnv* env = instance_id_internal->instance_id()->app().GetJNIEnv();
177192
jobject java_instance_id =
178193
env->NewLocalRef(instance_id_internal->java_instance_id());
@@ -187,7 +202,7 @@ Future<void> InstanceId::DeleteId() {
187202
operation, ExceptionStringToError(error.c_str()), error.c_str());
188203
}
189204
},
190-
operation, InstanceIdInternal::Canceled, &(**operation));
205+
&(*operation), InstanceIdInternal::Canceled, &(*operation));
191206
return DeleteIdLastResult();
192207
}
193208

@@ -218,7 +233,7 @@ Future<std::string> InstanceId::GetToken(const char* entity,
218233
if (!instance_id_internal_) return Future<std::string>();
219234

220235
JNIEnv* env = app().GetJNIEnv();
221-
SharedPtr<AsyncOperation>* operation = instance_id_internal_->AddOperation(
236+
SharedPtr<AsyncOperation> operation = instance_id_internal_->AddOperation(
222237
new AsyncTokenOperation(env, instance_id_internal_,
223238
instance_id_internal_
224239
->FutureAlloc<std::string>(
@@ -228,19 +243,26 @@ Future<std::string> InstanceId::GetToken(const char* entity,
228243
util::RunOnBackgroundThread(
229244
env,
230245
[](void* function_data) {
231-
SharedPtr<AsyncOperation> base_operation =
232-
*(static_cast<SharedPtr<AsyncOperation>*>(function_data));
233-
AsyncTokenOperation* operation =
234-
static_cast<AsyncTokenOperation*>(base_operation->derived());
246+
// Assume that when this callback is called, AsyncOperation should still
247+
// be in instance_id_internal->operations_ or this callback should not
248+
// be called at all due to the lock in CppThreadDispatcherContext.
249+
AsyncTokenOperation* op_ptr =
250+
static_cast<AsyncTokenOperation*>(function_data);
235251
InstanceIdInternal* instance_id_internal =
236-
operation->instance_id_internal();
252+
op_ptr->instance_id_internal();
253+
// Hold a reference to AsyncOperation so that it will not be deleted
254+
// during this callback.
255+
SharedPtr<AsyncOperation> operation =
256+
instance_id_internal->GetOperationSharedPtr(op_ptr);
257+
if (!operation) return;
258+
237259
JNIEnv* env = instance_id_internal->instance_id()->app().GetJNIEnv();
238260
jobject java_instance_id =
239261
env->NewLocalRef(instance_id_internal->java_instance_id());
240262
jmethodID java_instance_id_method =
241263
instance_id::GetMethodId(instance_id::kGetToken);
242-
jobject entity_jstring = env->NewStringUTF(operation->entity().c_str());
243-
jobject scope_jstring = env->NewStringUTF(operation->scope().c_str());
264+
jobject entity_jstring = env->NewStringUTF(op_ptr->entity().c_str());
265+
jobject scope_jstring = env->NewStringUTF(op_ptr->scope().c_str());
244266
operation->ReleaseExecuteCancelLock();
245267
jobject token_jstring =
246268
env->CallObjectMethod(java_instance_id, java_instance_id_method,
@@ -252,20 +274,20 @@ Future<std::string> InstanceId::GetToken(const char* entity,
252274
env->DeleteLocalRef(scope_jstring);
253275
if (operation->AcquireExecuteCancelLock()) {
254276
instance_id_internal->CompleteOperationWithResult(
255-
base_operation, token, ExceptionStringToError(error.c_str()),
277+
operation, token, ExceptionStringToError(error.c_str()),
256278
error.c_str());
257279
}
258280
},
259-
operation, InstanceIdInternal::CanceledWithResult<std::string>,
260-
&(**operation));
281+
&(*operation), InstanceIdInternal::CanceledWithResult<std::string>,
282+
&(*operation));
261283
return GetTokenLastResult();
262284
}
263285

264286
Future<void> InstanceId::DeleteToken(const char* entity, const char* scope) {
265287
if (!instance_id_internal_) return Future<void>();
266288

267289
JNIEnv* env = app().GetJNIEnv();
268-
SharedPtr<AsyncOperation>* operation =
290+
SharedPtr<AsyncOperation> operation =
269291
instance_id_internal_->AddOperation(new AsyncTokenOperation(
270292
env, instance_id_internal_,
271293
instance_id_internal_
@@ -276,15 +298,22 @@ Future<void> InstanceId::DeleteToken(const char* entity, const char* scope) {
276298
util::RunOnBackgroundThread(
277299
env,
278300
[](void* function_data) {
279-
SharedPtr<AsyncOperation> base_operation =
280-
*(static_cast<SharedPtr<AsyncOperation>*>(function_data));
281-
AsyncTokenOperation* operation =
282-
static_cast<AsyncTokenOperation*>(base_operation->derived());
301+
// Assume that when this callback is called, AsyncOperation should still
302+
// be in instance_id_internal->operations_ or this callback should not
303+
// be called at all due to the lock in CppThreadDispatcherContext.
304+
AsyncTokenOperation* op_ptr =
305+
static_cast<AsyncTokenOperation*>(function_data);
283306
InstanceIdInternal* instance_id_internal =
284-
operation->instance_id_internal();
307+
op_ptr->instance_id_internal();
308+
// Hold a reference to AsyncOperation so that it will not be deleted
309+
// during this callback.
310+
SharedPtr<AsyncOperation> operation =
311+
instance_id_internal->GetOperationSharedPtr(op_ptr);
312+
if (!operation) return;
313+
285314
JNIEnv* env = instance_id_internal->instance_id()->app().GetJNIEnv();
286-
jobject entity_jstring = env->NewStringUTF(operation->entity().c_str());
287-
jobject scope_jstring = env->NewStringUTF(operation->scope().c_str());
315+
jobject entity_jstring = env->NewStringUTF(op_ptr->entity().c_str());
316+
jobject scope_jstring = env->NewStringUTF(op_ptr->scope().c_str());
288317
jobject java_instance_id =
289318
env->NewLocalRef(instance_id_internal->java_instance_id());
290319
jmethodID java_instance_id_method =
@@ -298,11 +327,10 @@ Future<void> InstanceId::DeleteToken(const char* entity, const char* scope) {
298327
env->DeleteLocalRef(scope_jstring);
299328
if (operation->AcquireExecuteCancelLock()) {
300329
instance_id_internal->CompleteOperation(
301-
base_operation, ExceptionStringToError(error.c_str()),
302-
error.c_str());
330+
operation, ExceptionStringToError(error.c_str()), error.c_str());
303331
}
304332
},
305-
operation, InstanceIdInternal::Canceled, &(**operation));
333+
&(*operation), InstanceIdInternal::Canceled, &(*operation));
306334
return DeleteTokenLastResult();
307335
}
308336

‎instance_id/src/android/instance_id_internal.cc

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,11 @@ void InstanceIdInternal::Initialize(InstanceId* instance_id,
4444
}
4545

4646
// Store a reference to a scheduled operation.
47-
SharedPtr<AsyncOperation>* InstanceIdInternal::AddOperation(
47+
SharedPtr<AsyncOperation> InstanceIdInternal::AddOperation(
4848
AsyncOperation* operation) {
4949
MutexLock lock(operations_mutex_);
5050
operations_.push_back(SharedPtr<AsyncOperation>(operation));
51-
return &operations_[operations_.size() - 1];
51+
return operations_[operations_.size() - 1];
5252
}
5353

5454
// Remove a reference to a schedule operation.
@@ -63,11 +63,32 @@ void InstanceIdInternal::RemoveOperation(
6363
}
6464
}
6565

66+
// Find the SharedPtr to the operation using raw pointer.
67+
SharedPtr<AsyncOperation> InstanceIdInternal::GetOperationSharedPtr(
68+
AsyncOperation* operation) {
69+
MutexLock lock(operations_mutex_);
70+
for (auto it = operations_.begin(); it != operations_.end(); ++it) {
71+
if (&(**it) == operation) {
72+
return *it;
73+
}
74+
}
75+
return SharedPtr<AsyncOperation>();
76+
}
77+
6678
// Cancel all scheduled operations.
6779
void InstanceIdInternal::CancelOperations() {
68-
MutexLock lock(operations_mutex_);
69-
while (operations_.size()) {
70-
operations_[0]->Cancel();
80+
while (true) {
81+
SharedPtr<AsyncOperation> operation_ptr;
82+
{
83+
MutexLock lock(operations_mutex_);
84+
if (operations_.empty()) {
85+
break;
86+
}
87+
operation_ptr = operations_[0];
88+
}
89+
if (operation_ptr) {
90+
operation_ptr->Cancel();
91+
}
7192
}
7293
}
7394

@@ -80,9 +101,13 @@ void InstanceIdInternal::CompleteOperation(
80101
}
81102

82103
void InstanceIdInternal::Canceled(void* function_data) {
83-
SharedPtr<AsyncOperation>& operation =
84-
*(static_cast<SharedPtr<AsyncOperation>*>(function_data));
85-
operation->instance_id_internal()->CancelOperation(operation);
104+
AsyncOperation* ptr = static_cast<AsyncOperation*>(function_data);
105+
// Hold a reference to AsyncOperation so that it will not be deleted during
106+
// this callback.
107+
SharedPtr<AsyncOperation> operation =
108+
ptr->instance_id_internal()->GetOperationSharedPtr(ptr);
109+
if (!operation) return;
110+
operation->instance_id_internal()->CancelOperation(operation);
86111
}
87112

88113
} // namespace internal

‎instance_id/src/android/instance_id_internal.h

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,10 @@ class InstanceIdInternal : public InstanceIdInternalBase {
8484
InstanceId* instance_id() const { return instance_id_; }
8585

8686
// Store a reference to a scheduled operation.
87-
SharedPtr<AsyncOperation>* AddOperation(AsyncOperation* operation);
87+
SharedPtr<AsyncOperation> AddOperation(AsyncOperation* operation);
88+
89+
// Find the SharedPtr to the operation using raw pointer.
90+
SharedPtr<AsyncOperation> GetOperationSharedPtr(AsyncOperation* operation);
8891

8992
// Remove a reference to a schedule operation.
9093
void RemoveOperation(const SharedPtr<AsyncOperation>& operation);
@@ -127,8 +130,12 @@ class InstanceIdInternal : public InstanceIdInternalBase {
127130
// Complete a future with an error when an operation is canceled.
128131
template <typename T>
129132
static void CanceledWithResult(void* function_data) {
130-
SharedPtr<AsyncOperation>& operation =
131-
*(static_cast<SharedPtr<AsyncOperation>*>(function_data));
133+
AsyncOperation* ptr = static_cast<AsyncOperation*>(function_data);
134+
// Hold a reference to AsyncOperation so that it will not be deleted during
135+
// this callback.
136+
SharedPtr<AsyncOperation> operation =
137+
ptr->instance_id_internal()->GetOperationSharedPtr(ptr);
138+
if (!operation) return;
132139
operation->instance_id_internal()->CancelOperationWithResult<T>(operation);
133140
}
134141

0 commit comments

Comments
 (0)