Skip to content

Commit d89d08e

Browse files
committed
fix(event-loop): claim-gate ABI below API 26 and failure-path rollbacks from review
- @CriticalNative is ignored below API 26, where ART calls the method through the standard JNI ABI - binding the critical-convention function there would misread its arguments (minSdk is 21). Registration now binds a standard-ABI twin on api < 26, so the gate behaves identically on every supported API level. RegisterNatives failure no longer asserts: it clears the pending exception and gates PostTimerToken to plain tokens, so the unbound native can never be reached. - a failed JNI token post no longer leaks state: PostTimerToken releases the claim cell (no dispatch gate will ever retire it), and addTask erases the just-inserted sorted slot and map entry before rethrowing - a tokenless slot would otherwise consume another token's dispatch (live) or starve the item behind it (tombstoned). - RunOnMainThreadCallback resolves the main event loop before caching the callback, so a pre-init call can't pin the closure in the cache with no post to consume it. - tests: done.fail does not exist in the pinned jasmine 2.0.1 (it would TypeError inside the rejection handler and time out silently) - replaced with record-then-done; the background-clear race spec now counts only iterations whose clear provably ran (AtomicBoolean signal, bounded attempts), so it can't pass without racing. The RunMainThreadEntry isolate-liveness window flagged by review is byte-for-byte the removed pipe implementation's behavior and needs teardown-spanning liveness; deferred to the teardown-coordination work queued with the kExplicit follow-up.
1 parent c2be640 commit d89d08e

5 files changed

Lines changed: 100 additions & 23 deletions

File tree

test-app/app/src/main/assets/app/tests/testEventLoop.js

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,9 @@ describe("event loop foreground tasks", function () {
1212
expect(value).toBe("ok");
1313
done();
1414
}).catch(e => {
15-
done.fail("Atomics.waitAsync promise rejected: " + e);
15+
// jasmine 2.0.1: done has no .fail - record the failure, then complete
16+
expect("resolved").toBe("rejected: " + e);
17+
done();
1618
});
1719

1820
const woken = Atomics.notify(i32, 0);
@@ -30,7 +32,9 @@ describe("event loop foreground tasks", function () {
3032
expect(value).toBe("timed-out");
3133
done();
3234
}).catch(e => {
33-
done.fail("Atomics.waitAsync promise rejected: " + e);
35+
// jasmine 2.0.1: done has no .fail - record the failure, then complete
36+
expect("resolved").toBe("rejected: " + e);
37+
done();
3438
});
3539
});
3640

@@ -57,7 +61,8 @@ describe("event loop foreground tasks", function () {
5761
expect(order).toEqual(["waitAsync", "chained"]);
5862
done();
5963
}).catch(e => {
60-
done.fail("promise chain failed: " + e);
64+
expect("resolved").toBe("rejected: " + e);
65+
done();
6166
});
6267

6368
Atomics.notify(i32, 0);
@@ -169,14 +174,25 @@ describe("event loop token cancellation", function () {
169174
});
170175

171176
it("background-thread clear racing dispatch neither jumps java posts nor ghost-fires", function (done) {
177+
// only iterations whose clear provably ran count toward the quota, so
178+
// the spec can't pass on 30 runs where the thread never raced at all
172179
let remaining = 30;
180+
let attempts = 0;
173181
(function iter() {
182+
if (++attempts > 300) {
183+
expect("background clears raced " + (30 - remaining) + "/30 times")
184+
.toBe("background clears raced 30/30 times");
185+
done();
186+
return;
187+
}
174188
const order = [];
189+
const cleared = new java.util.concurrent.atomic.AtomicBoolean(false);
175190
const handler = new android.os.Handler(android.os.Looper.myLooper());
176191
const t1 = __ns__setTimeout(() => order.push("t1"), 0);
177192
new java.lang.Thread(new java.lang.Runnable({
178193
run() {
179194
__ns__clearTimeout(t1);
195+
cleared.set(true);
180196
}
181197
})).start();
182198
handler.post(new java.lang.Runnable({
@@ -188,7 +204,7 @@ describe("event loop token cancellation", function () {
188204
// t1 either fired before the clear landed (at its own legal
189205
// slot, ahead of "java") or never; t2 must never jump "java"
190206
expect(observed === "java>t2" || observed === "t1>java>t2").toBe(true);
191-
if (--remaining === 0) {
207+
if (cleared.get() && --remaining === 0) {
192208
done();
193209
} else {
194210
iter();

test-app/runtime/src/main/cpp/CallbackHandlers.cpp

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -684,17 +684,20 @@ void CallbackHandlers::RunOnMainThreadCallback(const FunctionCallbackInfo<v8::Va
684684
uint64_t key = ++count_;
685685
Local<v8::Function> callback = args[0].As<v8::Function>();
686686

687+
// resolve the loop before inserting: an entry cached with no post to
688+
// consume it would pin the callback until isolate teardown
689+
auto mainLoop = Runtime::GetMainEventLoop();
690+
if (mainLoop == nullptr) {
691+
return;
692+
}
693+
687694
{
688695
std::lock_guard<std::mutex> lock(cacheMutex_);
689696
bool inserted;
690697
std::tie(std::ignore, inserted) = cache_.try_emplace(key, isolate, callback);
691698
assert(inserted && "Main thread callback ID should not be duplicated");
692699
}
693700

694-
auto mainLoop = Runtime::GetMainEventLoop();
695-
if (mainLoop == nullptr) {
696-
return;
697-
}
698701
// bare entry: the closure locks the CALLER's isolate (possibly a
699702
// worker's), so the loop must not take the main isolate's Locker first -
700703
// nesting the two can deadlock against multithreaded-JS entry paths

test-app/runtime/src/main/cpp/EventLoop.cpp

Lines changed: 41 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
#include "EventLoop.h"
22

3+
#include <android/api-level.h>
34
#include <sys/eventfd.h>
45
#include <sys/timerfd.h>
56
#include <unistd.h>
@@ -46,6 +47,7 @@ void RunGuarded(F&& body) {
4647

4748
namespace tns {
4849

50+
bool EventLoop::claimGateRegistered_ = false;
4951
jclass EventLoop::EVENT_LOOP_HANDLER_CLASS = nullptr;
5052
jmethodID EventLoop::EVENT_LOOP_HANDLER_CTOR = nullptr;
5153
jmethodID EventLoop::EVENT_LOOP_HANDLER_POST = nullptr;
@@ -106,14 +108,23 @@ void EventLoop::BindToCurrentThread() {
106108
EVENT_LOOP_HANDLER_CANCEL_IDENTIFIED = env.GetMethodID(
107109
EVENT_LOOP_HANDLER_CLASS, "cancelIdentified", "(Ljava/lang/Object;)Z");
108110
EVENT_LOOP_HANDLER_RELEASE = env.GetMethodID(EVENT_LOOP_HANDLER_CLASS, "release", "()V");
109-
// the @CriticalNative gate must be bound explicitly (name resolution
110-
// doesn't apply to the critical calling convention on older ART)
111-
static const JNINativeMethod claimMethod = {
111+
// The @CriticalNative gate must be bound explicitly (name resolution
112+
// doesn't apply to the critical calling convention on older ART).
113+
// Below API 26 ART ignores the annotation and calls through the
114+
// normal JNI ABI, so bind the standard-convention twin there.
115+
const bool criticalAbi = android_get_device_api_level() >= 26;
116+
const JNINativeMethod claimMethod = {
112117
const_cast<char*>("nativeClaimToken"), const_cast<char*>("(JJ)Z"),
113-
reinterpret_cast<void*>(EventLoop::ClaimTokenCritical)};
118+
criticalAbi ? reinterpret_cast<void*>(EventLoop::ClaimTokenCritical)
119+
: reinterpret_cast<void*>(EventLoop::ClaimTokenLegacy)};
114120
JNIEnv* rawEnv = env;
115-
jint registered = rawEnv->RegisterNatives(EVENT_LOOP_HANDLER_CLASS, &claimMethod, 1);
116-
assert(registered == 0);
121+
if (rawEnv->RegisterNatives(EVENT_LOOP_HANDLER_CLASS, &claimMethod, 1) == 0) {
122+
claimGateRegistered_ = true;
123+
} else {
124+
rawEnv->ExceptionClear();
125+
DEBUG_WRITE_FORCE(
126+
"EventLoop: claim gate registration failed; timer tokens stay plain");
127+
}
117128
}
118129
JniLocalRef handler(env.NewObject(EVENT_LOOP_HANDLER_CLASS, EVENT_LOOP_HANDLER_CTOR,
119130
reinterpret_cast<jlong>(this)));
@@ -283,16 +294,28 @@ uint64_t EventLoop::PostTimerToken(jlong uptimeMillis, int timerId) {
283294
}
284295
uint64_t word = 0;
285296
auto& cell = claimCells_[((uint32_t) timerId) & (kClaimCells - 1)];
286-
uint64_t expected = 0;
287-
uint64_t candidate = (((uint64_t) (uint32_t) timerId) << 2) | kCellActive;
288-
if (cell.compare_exchange_strong(expected, candidate, std::memory_order_acq_rel)) {
289-
word = candidate;
297+
if (claimGateRegistered_) {
298+
uint64_t expected = 0;
299+
uint64_t candidate = (((uint64_t) (uint32_t) timerId) << 2) | kCellActive;
300+
if (cell.compare_exchange_strong(expected, candidate, std::memory_order_acq_rel)) {
301+
word = candidate;
302+
}
303+
// a busy slot (previous token of the same interval still in flight,
304+
// or an id collision) downgrades this token to plain; clear then uses
305+
// tombstones
290306
}
291-
// a busy slot (previous token of the same interval still in flight, or an
292-
// id collision) downgrades this token to plain; clear then uses tombstones
293307
JEnv env;
294-
env.CallVoidMethod(handler_, EVENT_LOOP_HANDLER_POST_TOKEN, uptimeMillis,
295-
(jint) (word >> 32), (jint) (word & 0xffffffffull));
308+
try {
309+
env.CallVoidMethod(handler_, EVENT_LOOP_HANDLER_POST_TOKEN, uptimeMillis,
310+
(jint) (word >> 32), (jint) (word & 0xffffffffull));
311+
} catch (...) {
312+
// no token reached the queue, so no dispatch gate will ever retire
313+
// the cell - release it here or the slot is burned for the process
314+
if (word != 0) {
315+
cell.store(0, std::memory_order_release);
316+
}
317+
throw;
318+
}
296319
return word;
297320
}
298321

@@ -340,6 +363,10 @@ void EventLoop::ReleaseIdentifiedToken(jobject peer) {
340363
env.DeleteGlobalRef(peer);
341364
}
342365

366+
jboolean EventLoop::ClaimTokenLegacy(JNIEnv* env, jclass clazz, jlong loopPtr, jlong cellWord) {
367+
return ClaimTokenCritical(loopPtr, cellWord);
368+
}
369+
343370
jboolean EventLoop::ClaimTokenCritical(jlong loopPtr, jlong cellWord) {
344371
// @CriticalNative: no JNIEnv, thread stays runnable - a single CAS, no
345372
// locks, no allocation, no exceptions. The loop pointer is valid for the

test-app/runtime/src/main/cpp/EventLoop.h

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,17 @@ class EventLoop {
259259
*/
260260
static jboolean ClaimTokenCritical(jlong loopPtr, jlong cellWord);
261261

262+
/**
263+
* Standard-ABI twin registered on devices below API 26, where ART ignores
264+
* @CriticalNative and calls through the normal JNI convention - binding
265+
* the critical-convention function there would misread its arguments.
266+
*/
267+
static jboolean ClaimTokenLegacy(JNIEnv* env, jclass clazz, jlong loopPtr, jlong cellWord);
268+
269+
// false when the claim gate couldn't be registered: PostTimerToken then
270+
// never emits cell words, so nativeClaimToken is never invoked
271+
static bool claimGateRegistered_;
272+
262273
v8::Isolate* isolate_;
263274
std::mutex mutex_;
264275
Lane internal_;

test-app/runtime/src/main/cpp/Timers.cpp

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,27 @@ void Timers::addTask(std::shared_ptr<TimerTask> task) {
9494
return ref.dueTime > value;
9595
});
9696
sortedTimers_.insert(it, TimerReference{task->id_, task->dueTime_});
97-
postTimer(task, now);
97+
try {
98+
postTimer(task, now);
99+
} catch (...) {
100+
// No token reached the queue: the slot must not linger, tokenless -
101+
// as a live entry it would consume some other token's slot, and as a
102+
// tombstone it would starve the item behind it. Erase it outright.
103+
auto sit = std::lower_bound(sortedTimers_.begin(), sortedTimers_.end(), task->dueTime_,
104+
[](const TimerReference &ref, const double &value) {
105+
return ref.dueTime < value;
106+
});
107+
while (sit != sortedTimers_.end() && sit->dueTime == task->dueTime_) {
108+
if (sit->id == task->id_) {
109+
sortedTimers_.erase(sit);
110+
break;
111+
}
112+
++sit;
113+
}
114+
timerMap_.erase(task->id_);
115+
task->Unschedule();
116+
throw;
117+
}
98118
}
99119

100120
// Above this remaining delay a timer's token gets an identified peer so a

0 commit comments

Comments
 (0)