Skip to content

Commit 2ed5343

Browse files
committed
fix(runtime): JNI hygiene in the sync fetch's failure paths
The local-ref frame now encloses the catch handlers: a caught NativeScriptException holds the pending Java throwable as a local ref in that frame and renders its message lazily, so popping the frame during unwind handed the handlers a stale ref on the common network-failure path. StrictModeScope clears the pending exception on every construction bail (a null FindClass/GetMethodID result leaves one armed) and holds the saved policy as a global ref so the restore does not depend on frame lifetime. NormalizeHttpModuleUrl lives beside CanonicalizeHttpUrlKey now that every URL consumer must run it first.
1 parent 253eef6 commit 2ed5343

3 files changed

Lines changed: 65 additions & 21 deletions

File tree

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

Lines changed: 55 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,27 @@ bool IsRemoteUrlAllowed(const std::string& url) {
151151
// ─────────────────────────────────────────────────────────────
152152
// Canonical module keys
153153

154+
std::string NormalizeHttpModuleUrl(const std::string& path) {
155+
if (path.empty()) {
156+
return path;
157+
}
158+
159+
std::string normalized = path;
160+
if (StartsWith(normalized, "file://http://") || StartsWith(normalized, "file://https://")) {
161+
normalized = normalized.substr(strlen("file://"));
162+
}
163+
164+
// A path normalizer that collapses `//` into `/` (Java's, or a URL that
165+
// travelled through one) leaves the scheme separator one slash short.
166+
if (normalized.rfind("http:/", 0) == 0 && normalized.rfind("http://", 0) != 0) {
167+
normalized.insert(5, "/");
168+
} else if (normalized.rfind("https:/", 0) == 0 && normalized.rfind("https://", 0) != 0) {
169+
normalized.insert(6, "/");
170+
}
171+
172+
return normalized;
173+
}
174+
154175
std::string CanonicalizeHttpUrlKey(const std::string& url) {
155176
std::string normalizedUrl = url;
156177
if (StartsWith(normalizedUrl, "file://http://") || StartsWith(normalizedUrl, "file://https://")) {
@@ -359,28 +380,36 @@ struct StrictModeScope {
359380
jobject savedPolicy = nullptr;
360381

361382
explicit StrictModeScope(JEnv& env) {
383+
// Every bail must clear: FindClass/GetMethodID return null WITH an
384+
// exception pending, and leaving one armed makes the caller's next
385+
// JNI call illegal.
386+
JNIEnv* raw = env;
362387
clsStrict = env.FindClass("android/os/StrictMode");
363388
jclass clsPolicyBuilder = env.FindClass("android/os/StrictMode$ThreadPolicy$Builder");
364389
if (!clsStrict || !clsPolicyBuilder) {
390+
raw->ExceptionClear();
365391
return;
366392
}
367393
jmethodID getThreadPolicy = env.GetStaticMethodID(
368394
clsStrict, "getThreadPolicy", "()Landroid/os/StrictMode$ThreadPolicy;");
369395
jmethodID setter = env.GetStaticMethodID(
370396
clsStrict, "setThreadPolicy", "(Landroid/os/StrictMode$ThreadPolicy;)V");
371397
if (!getThreadPolicy || !setter) {
398+
raw->ExceptionClear();
372399
return;
373400
}
374401
// No captured policy means no way back, so leave the thread alone
375402
// rather than relaxing it permanently.
376403
jobject captured = env.CallStaticObjectMethod(clsStrict, getThreadPolicy);
377404
if (!captured) {
405+
raw->ExceptionClear();
378406
return;
379407
}
380408

381409
jmethodID builderCtor = env.GetMethodID(clsPolicyBuilder, "<init>", "()V");
382-
jobject builder = env.NewObject(clsPolicyBuilder, builderCtor);
410+
jobject builder = builderCtor ? env.NewObject(clsPolicyBuilder, builderCtor) : nullptr;
383411
if (!builder) {
412+
raw->ExceptionClear();
384413
return;
385414
}
386415
jmethodID permitAll = env.GetMethodID(clsPolicyBuilder, "permitAll",
@@ -391,12 +420,15 @@ struct StrictModeScope {
391420
jobject policy = build ? env.CallObjectMethod(builder2 ? builder2 : builder, build)
392421
: nullptr;
393422
if (!policy) {
423+
raw->ExceptionClear();
394424
return;
395425
}
396426
env.CallStaticVoidMethod(clsStrict, setter, policy);
397-
// Armed only once the permissive policy is actually in force.
427+
// Armed only once the permissive policy is actually in force. The
428+
// saved policy is a global ref so the restore does not depend on any
429+
// JNI local frame the caller pushed around this scope.
398430
setThreadPolicy = setter;
399-
savedPolicy = captured;
431+
savedPolicy = env.NewGlobalRef(captured);
400432
jni = env;
401433
}
402434

@@ -406,6 +438,7 @@ struct StrictModeScope {
406438
~StrictModeScope() {
407439
if (jni == nullptr) return;
408440
jni->CallStaticVoidMethod(clsStrict, setThreadPolicy, savedPolicy);
441+
jni->DeleteGlobalRef(savedPolicy);
409442
jni->ExceptionClear();
410443
}
411444
};
@@ -630,27 +663,28 @@ static bool PerformHttpFetchOnceSync(const std::string& url, const std::string&
630663
url.c_str(), excClass.c_str(), excMsg.c_str());
631664
};
632665

633-
try {
634-
JEnv env;
635-
JNIEnv* raw = env;
666+
JEnv env;
667+
JNIEnv* raw = env;
636668

637-
// Request setup alone burns a couple of dozen local refs (a jstring
638-
// per header, one per drained exception), and the sync path runs
639-
// inside a caller's frame — V8's resolve walk — that must not be left
640-
// holding them. Pushed first, so everything scoped inside it,
641-
// StrictModeScope included, is torn down while its refs are still live.
642-
const bool framePushed = raw->PushLocalFrame(64) == JNI_OK;
643-
if (!framePushed) {
644-
raw->ExceptionClear();
669+
// Request setup alone burns a couple of dozen local refs (a jstring per
670+
// header, one per drained exception), and the sync path runs inside a
671+
// caller's frame — V8's resolve walk — that must not be left holding
672+
// them. Declared OUTSIDE the try: a caught NativeScriptException may hold
673+
// the Java throwable as a local ref in this frame, which the handlers
674+
// below read — the pop must come after them, not during the unwind.
675+
const bool framePushed = raw->PushLocalFrame(64) == JNI_OK;
676+
if (!framePushed) {
677+
raw->ExceptionClear();
678+
}
679+
struct LocalFrame {
680+
JNIEnv* jni;
681+
bool pushed;
682+
~LocalFrame() {
683+
if (pushed) jni->PopLocalFrame(nullptr);
645684
}
646-
struct LocalFrame {
647-
JNIEnv* jni;
648-
bool pushed;
649-
~LocalFrame() {
650-
if (pushed) jni->PopLocalFrame(nullptr);
651-
}
652-
} localFrame{raw, framePushed};
685+
} localFrame{raw, framePushed};
653686

687+
try {
654688
StrictModeScope strictMode(env);
655689

656690
jclass clsURL = env.FindClass("java/net/URL");

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,13 @@ struct CanonicalizationConfig {
6868
// module under exactly one URL and never varies it for freshness.
6969
std::string CanonicalizeHttpUrlKey(const std::string& url);
7070

71+
// Undoes what a path normalizer did to an http(s) URL — unwraps a `file://`
72+
// prefix the resolver may have put in front, and re-doubles a scheme
73+
// separator collapsed to a single slash (`http:/host/...`). Every consumer
74+
// that classifies or keys a module URL must run this first, or the same
75+
// module string routes/keys differently at different sites.
76+
std::string NormalizeHttpModuleUrl(const std::string& path);
77+
7178
// What a module response turned out to be. Decided once, by the shared
7279
// classifier, for whichever transport produced the response.
7380
enum class ModuleResponseKind {

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,9 @@ std::shared_ptr<Persistent<Value>> MakeOwnedPersistent(Isolate* isolate,
183183

184184
} // namespace
185185

186+
// The throwable is held as a JNI LOCAL ref, and the message is rendered
187+
// lazily from it (what()/ToString) - so the exception must be consumed
188+
// before any JNI local frame enclosing this construction is popped.
186189
NativeScriptException::NativeScriptException(JEnv& env)
187190
: m_javascriptException(nullptr) {
188191
m_javaException = JniLocalRef(env.ExceptionOccurred());

0 commit comments

Comments
 (0)