Skip to content

Commit 6a533ce

Browse files
edusperoniedusperoni
committed
fix: resolve __postFrameCallback crash on re-scheduling
Also adds tests
1 parent 5738833 commit 6a533ce

File tree

4 files changed

+197
-9
lines changed

4 files changed

+197
-9
lines changed

‎test-app/app/src/main/assets/app/mainpage.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,4 +68,5 @@ require("./tests/kotlin/access/testInternalLanguageFeaturesSupport");
6868
require("./tests/testPackagePrivate");
6969
require("./tests/kotlin/properties/testPropertiesSupport.js");
7070
require('./tests/testNativeTimers');
71+
require("./tests/testPostFrameCallback");
7172
require("./tests/console/logTests.js");

‎test-app/app/src/main/assets/app/tests/testPostFrameCallback.js

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
describe("test PostFrameCallback", function () {
2+
const defaultWaitTime = 300;
3+
it("__postFrameCallback exists", () => {
4+
expect(global.__postFrameCallback).toBeDefined();
5+
});
6+
7+
it("__removeFrameCallback exists", () => {
8+
expect(global.__removeFrameCallback).toBeDefined();
9+
});
10+
11+
it("should throw when providing wrong arguments", () => {
12+
expect(() => global.__postFrameCallback(null)).toThrow();
13+
expect(() => global.__removeFrameCallback(null)).toThrow();
14+
expect(() => global.__postFrameCallback("")).toThrow();
15+
expect(() => global.__removeFrameCallback("")).toThrow();
16+
expect(() => global.__postFrameCallback()).toThrow();
17+
expect(() => global.__removeFrameCallback()).toThrow();
18+
});
19+
20+
it("should call the callback once", (done) => {
21+
let callCount = 0;
22+
const callback = () => {
23+
callCount++;
24+
};
25+
global.__postFrameCallback(callback);
26+
setTimeout(() => {
27+
expect(callCount).toBe(1);
28+
done();
29+
}, defaultWaitTime);
30+
});
31+
32+
it("should call the callback once even if scheduled multiple times", (done) => {
33+
let callCount = 0;
34+
const callback = () => {
35+
callCount++;
36+
};
37+
global.__postFrameCallback(callback);
38+
global.__postFrameCallback(callback);
39+
setTimeout(() => {
40+
expect(callCount).toBe(1);
41+
done();
42+
}, defaultWaitTime);
43+
});
44+
45+
it("should not trigger the callback if it was canceled", (done) => {
46+
let callCount = 0;
47+
const callback = () => {
48+
callCount++;
49+
};
50+
global.__postFrameCallback(callback);
51+
global.__removeFrameCallback(callback);
52+
setTimeout(() => {
53+
expect(callCount).toBe(0);
54+
done();
55+
}, defaultWaitTime);
56+
});
57+
58+
it("should trigger the callback if it was canceled then re-scheduled", (done) => {
59+
let callCount = 0;
60+
const callback = () => {
61+
callCount++;
62+
};
63+
global.__postFrameCallback(callback);
64+
global.__removeFrameCallback(callback);
65+
global.__postFrameCallback(callback);
66+
setTimeout(() => {
67+
expect(callCount).toBe(1);
68+
done();
69+
}, defaultWaitTime);
70+
});
71+
72+
it("should trigger the callback if it was re-scheduled by itself", (done) => {
73+
let callCount = 0;
74+
const callback = () => {
75+
callCount++;
76+
if (callCount === 1) {
77+
global.__postFrameCallback(callback);
78+
}
79+
};
80+
global.__postFrameCallback(callback);
81+
setTimeout(() => {
82+
expect(callCount).toBe(2);
83+
done();
84+
}, defaultWaitTime);
85+
});
86+
87+
it("should release the callback after being done", (done) => {
88+
let callCount = 0;
89+
let callback = () => {
90+
callCount++;
91+
};
92+
global.__postFrameCallback(callback);
93+
const weakCallback = new WeakRef(callback);
94+
callback = null;
95+
gc();
96+
setTimeout(() => {
97+
gc();
98+
expect(callCount).toBe(1);
99+
expect(!!weakCallback.deref()).toBe(false);
100+
done();
101+
}, defaultWaitTime);
102+
});
103+
104+
it("should release the callback removal", (done) => {
105+
let callCount = 0;
106+
let callback = () => {
107+
callCount++;
108+
};
109+
global.__postFrameCallback(callback);
110+
global.__removeFrameCallback(callback);
111+
const weakCallback = new WeakRef(callback);
112+
callback = null;
113+
gc();
114+
setTimeout(() => {
115+
gc();
116+
expect(callCount).toBe(0);
117+
expect(!!weakCallback.deref()).toBe(false);
118+
done();
119+
}, defaultWaitTime);
120+
});
121+
122+
it("should retain callback until called", (done) => {
123+
let callCount = 0;
124+
let callback = () => {
125+
callCount++;
126+
gc();
127+
expect(!!weakCallback.deref()).toBe(true);
128+
};
129+
global.__postFrameCallback(callback);
130+
global.__removeFrameCallback(callback);
131+
global.__postFrameCallback(callback);
132+
const weakCallback = new WeakRef(callback);
133+
callback = null;
134+
gc();
135+
setTimeout(() => {
136+
gc();
137+
expect(callCount).toBe(1);
138+
expect(!!weakCallback.deref()).toBe(false);
139+
done();
140+
}, defaultWaitTime);
141+
});
142+
});

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

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1609,7 +1609,6 @@ void CallbackHandlers::PostCallback(const FunctionCallbackInfo<v8::Value> &args,
16091609
void CallbackHandlers::PostFrameCallback(const FunctionCallbackInfo<v8::Value> &args) {
16101610
if (android_get_device_api_level() >= 24) {
16111611
InitChoreographer();
1612-
assert(args[0]->IsFunction());
16131612
Isolate *isolate = args.GetIsolate();
16141613

16151614
v8::Locker locker(isolate);
@@ -1618,6 +1617,11 @@ void CallbackHandlers::PostFrameCallback(const FunctionCallbackInfo<v8::Value> &
16181617
auto context = isolate->GetCurrentContext();
16191618
Context::Scope context_scope(context);
16201619

1620+
if (args.Length() < 1 || !args[0]->IsFunction()) {
1621+
isolate->ThrowException(v8::Exception::TypeError(v8::String::NewFromUtf8Literal(isolate, "Frame callback argument is not a function")));
1622+
return;
1623+
}
1624+
16211625
auto func = args[0].As<Function>();
16221626

16231627
auto idKey = ArgConverter::ConvertToV8String(isolate, "_postFrameCallbackId");
@@ -1629,8 +1633,13 @@ void CallbackHandlers::PostFrameCallback(const FunctionCallbackInfo<v8::Value> &
16291633
auto id = pId->IntegerValue(context).FromMaybe(0);
16301634
auto cb = frameCallbackCache_.find(id);
16311635
if (cb != frameCallbackCache_.end()) {
1632-
cb->second.removed = false;
1633-
PostCallback(args, &cb->second, context);
1636+
// check if it's already scheduled first, we don't want to schedule it twice
1637+
bool shouldReschedule = !cb->second.isScheduled();
1638+
// always mark as scheduled, as that will also mark it as not removed anymore
1639+
cb->second.markScheduled();
1640+
if (shouldReschedule) {
1641+
PostCallback(args, &cb->second, context);
1642+
}
16341643
return;
16351644
}
16361645
}
@@ -1645,6 +1654,7 @@ void CallbackHandlers::PostFrameCallback(const FunctionCallbackInfo<v8::Value> &
16451654
std::tie(val, inserted) = frameCallbackCache_.try_emplace(key, isolate, callback, key);
16461655
assert(inserted && "Frame callback ID should not be duplicated");
16471656

1657+
val->second.markScheduled();
16481658
PostCallback(args, &val->second, context);
16491659
}
16501660
}
@@ -1653,15 +1663,17 @@ void CallbackHandlers::RemoveFrameCallback(const FunctionCallbackInfo<v8::Value>
16531663

16541664
if (android_get_device_api_level() >= 24) {
16551665
InitChoreographer();
1656-
assert(args[0]->IsFunction());
16571666
Isolate *isolate = args.GetIsolate();
1658-
16591667
v8::Locker locker(isolate);
16601668
Isolate::Scope isolate_scope(isolate);
16611669
HandleScope handle_scope(isolate);
16621670
auto context = isolate->GetCurrentContext();
16631671
Context::Scope context_scope(context);
16641672

1673+
if (args.Length() < 1 || !args[0]->IsFunction()) {
1674+
isolate->ThrowException(v8::Exception::TypeError(v8::String::NewFromUtf8Literal(isolate, "Frame callback argument is not a function")));
1675+
return;
1676+
}
16651677
auto func = args[0].As<Function>();
16661678

16671679
auto idKey = ArgConverter::ConvertToV8String(isolate, "_postFrameCallbackId");
@@ -1673,7 +1685,7 @@ void CallbackHandlers::RemoveFrameCallback(const FunctionCallbackInfo<v8::Value>
16731685
auto id = pId->IntegerValue(context).FromMaybe(0);
16741686
auto cb = frameCallbackCache_.find(id);
16751687
if (cb != frameCallbackCache_.end()) {
1676-
cb->second.removed = true;
1688+
cb->second.markRemoved();
16771689
}
16781690
}
16791691

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

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -328,9 +328,21 @@ namespace tns {
328328

329329
v8::Isolate *isolate_;
330330
v8::Global<v8::Function> callback_;
331-
bool removed = false;
332331
uint64_t id;
333332

333+
bool isScheduled() {
334+
return scheduled;
335+
}
336+
337+
void markScheduled() {
338+
scheduled = true;
339+
removed = false;
340+
}
341+
void markRemoved() {
342+
// we can never unschedule a callback, so we just mark it as removed
343+
removed = true;
344+
}
345+
334346
AChoreographer_frameCallback frameCallback_ = [](long ts, void *data) {
335347
execute((double)ts, data);
336348
};
@@ -342,7 +354,7 @@ namespace tns {
342354
static void execute(double ts, void *data){
343355
if (data != nullptr) {
344356
auto entry = static_cast<FrameCallbackCacheEntry *>(data);
345-
if (entry->removed) {
357+
if (entry->shouldRemoveBeforeCall()) {
346358
frameCallbackCache_.erase(entry->id); // invalidates *entry
347359
return;
348360
}
@@ -354,21 +366,42 @@ namespace tns {
354366
v8::Local<v8::Function> cb = entry->callback_.Get(isolate);
355367
v8::Local<v8::Context> context = cb->GetCreationContextChecked();
356368
v8::Context::Scope context_scope(context);
369+
// we're running the callback now, so it's not scheduled anymore
370+
entry->markUnscheduled();
357371

358372
v8::Local<v8::Value> args[1] = {v8::Number::New(isolate, ts)};
359373

360374
v8::TryCatch tc(isolate);
361375

362376
cb->Call(context, context->Global(), 1, args); // ignore JS return value
363377

364-
frameCallbackCache_.erase(entry->id); // invalidates *entry
378+
// check if we should remove it (it should be both unscheduled and removed)
379+
if (entry->shouldRemoveAfterCall()) {
380+
frameCallbackCache_.erase(entry->id); // invalidates *entry
381+
}
382+
365383

366384
if(tc.HasCaught()){
367385
throw NativeScriptException(tc);
368386
}
369387

370388
}
371389
}
390+
private:
391+
bool removed = false;
392+
bool scheduled = false;
393+
void markUnscheduled() {
394+
scheduled = false;
395+
removed = true;
396+
}
397+
398+
bool shouldRemoveBeforeCall() {
399+
return removed;
400+
}
401+
402+
bool shouldRemoveAfterCall() {
403+
return !scheduled && removed;
404+
}
372405

373406
};
374407

0 commit comments

Comments
 (0)