Skip to content

Commit 9380d0f

Browse files
NathanWalkerNathanWalker
committed
fix(runtime): harden HTTP fetch, extend names, and worker drain retries
JNI mid-body read exceptions no longer spin the JS thread, async fetch threads detach from the JVM, and canonicalization config is published as an immutable snapshot so configureLoader cannot race a background fetch.
1 parent 7cf8d44 commit 9380d0f

8 files changed

Lines changed: 126 additions & 38 deletions

File tree

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

Lines changed: 64 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include <atomic>
99
#include <chrono>
1010
#include <cstring>
11+
#include <memory>
1112
#include <mutex>
1213
#include <string>
1314
#include <thread>
@@ -20,6 +21,7 @@
2021
#include "NativeScriptException.h"
2122
#include "Runtime.h"
2223
#include "robin_hood.h"
24+
#include "v8-json.h"
2325

2426
namespace tns {
2527

@@ -232,25 +234,33 @@ struct CanonicalizationConfig {
232234
std::vector<std::string> devPathPrefixes;
233235
std::vector<std::string> preserveQueryPrefixes;
234236
};
235-
static CanonicalizationConfig g_canonConfig;
236-
static bool g_canonConfigured = false;
237+
static std::mutex g_canonConfigMutex;
238+
static std::shared_ptr<const CanonicalizationConfig> g_canonConfig;
239+
240+
static std::shared_ptr<const CanonicalizationConfig> CurrentCanonicalizationConfig() {
241+
std::lock_guard<std::mutex> lock(g_canonConfigMutex);
242+
return g_canonConfig;
243+
}
237244

238245
static void SetCanonicalizationConfig(CanonicalizationConfig config) {
239-
g_canonConfig = std::move(config);
240-
g_canonConfigured = true;
246+
auto snapshot = std::make_shared<const CanonicalizationConfig>(std::move(config));
247+
{
248+
std::lock_guard<std::mutex> lock(g_canonConfigMutex);
249+
g_canonConfig = snapshot;
250+
}
241251
if (IsScriptLoadingLogEnabled()) {
242252
DEBUG_WRITE_FORCE(
243253
"[ns:module configureLoader] canonicalization set (strip=%lu devPrefixes=%lu "
244254
"preserve=%lu)",
245-
(unsigned long)g_canonConfig.stripParams.size(),
246-
(unsigned long)g_canonConfig.devPathPrefixes.size(),
247-
(unsigned long)g_canonConfig.preserveQueryPrefixes.size());
255+
(unsigned long)snapshot->stripParams.size(),
256+
(unsigned long)snapshot->devPathPrefixes.size(),
257+
(unsigned long)snapshot->preserveQueryPrefixes.size());
248258
}
249259
}
250260

251261
static void ResetCanonicalizationConfig() {
252-
g_canonConfig = CanonicalizationConfig{};
253-
g_canonConfigured = false;
262+
std::lock_guard<std::mutex> lock(g_canonConfigMutex);
263+
g_canonConfig.reset();
254264
}
255265

256266
std::string CanonicalizeHttpUrlKey(const std::string& url) {
@@ -278,16 +288,17 @@ std::string CanonicalizeHttpUrlKey(const std::string& url) {
278288
std::string originAndPath = (qPos == std::string::npos) ? noHash : noHash.substr(0, qPos);
279289
std::string query = (qPos == std::string::npos) ? std::string() : noHash.substr(qPos + 1);
280290

291+
auto canon = CurrentCanonicalizationConfig();
281292
{
282293
std::string pathOnly = originAndPath.substr(pathStart);
283-
if (g_canonConfigured) {
284-
for (const auto& p : g_canonConfig.preserveQueryPrefixes) {
294+
if (canon) {
295+
for (const auto& p : canon->preserveQueryPrefixes) {
285296
if (!p.empty() && pathOnly.find(p) != std::string::npos) {
286297
return noHash;
287298
}
288299
}
289300
bool isDevEndpoint = false;
290-
for (const auto& p : g_canonConfig.devPathPrefixes) {
301+
for (const auto& p : canon->devPathPrefixes) {
291302
if (!p.empty() && StartsWith(pathOnly, p.c_str())) {
292303
isDevEndpoint = true;
293304
break;
@@ -322,9 +333,9 @@ std::string CanonicalizeHttpUrlKey(const std::string& url) {
322333
size_t eq = pair.find('=');
323334
std::string name = (eq == std::string::npos) ? pair : pair.substr(0, eq);
324335
bool drop;
325-
if (g_canonConfigured) {
326-
drop = std::find(g_canonConfig.stripParams.begin(), g_canonConfig.stripParams.end(),
327-
name) != g_canonConfig.stripParams.end();
336+
if (canon) {
337+
drop = std::find(canon->stripParams.begin(), canon->stripParams.end(),
338+
name) != canon->stripParams.end();
328339
} else {
329340
drop = (name == "import" || name == "t" || name == "v");
330341
}
@@ -698,14 +709,29 @@ static bool PerformHttpFetchOnceSync(const std::string& url, std::string& out,
698709
jobject baos = env.NewObject(clsBAOS, baosCtor);
699710

700711
jbyteArray buffer = env.NewByteArray(8192);
712+
bool readFailed = false;
701713
while (true) {
702714
jint n = env.CallIntMethod(inStream, readMethod, buffer);
715+
std::string excClass, excMsg;
716+
if (DrainPendingJniException(env, excClass, excMsg)) {
717+
RecordLastHttpFetchError("read-body", excClass, excMsg);
718+
if (IsScriptLoadingLogEnabled()) {
719+
DEBUG_WRITE_FORCE(
720+
"[http-esm][fetch][exception] stage=read-body url=%s class=%s msg=%s",
721+
url.c_str(), excClass.c_str(), excMsg.c_str());
722+
}
723+
readFailed = true;
724+
break;
725+
}
703726
if (n < 0) break;
704727
if (n == 0) continue;
705728
env.CallVoidMethod(baos, baosWrite, buffer, 0, n);
706729
}
707730

708731
env.CallVoidMethod(inStream, closeIS);
732+
if (readFailed) {
733+
return false;
734+
}
709735
jbyteArray bytes = static_cast<jbyteArray>(env.CallObjectMethod(baos, baosToByteArray));
710736
env.CallVoidMethod(baos, baosClose);
711737

@@ -773,6 +799,26 @@ void FetchModuleBodyAsync(const std::string& url,
773799
}
774800

775801
std::thread([url, completion = std::move(completion)]() mutable {
802+
JavaVM* jvm = Runtime::GetJVM();
803+
bool attachedHere = false;
804+
if (jvm != nullptr) {
805+
JNIEnv* raw = nullptr;
806+
if (jvm->GetEnv(reinterpret_cast<void**>(&raw), JNI_VERSION_1_6) != JNI_OK) {
807+
if (jvm->AttachCurrentThread(&raw, nullptr) == JNI_OK) {
808+
attachedHere = true;
809+
}
810+
}
811+
}
812+
struct DetachIfAttached {
813+
JavaVM* jvm;
814+
bool attached;
815+
~DetachIfAttached() {
816+
if (attached && jvm != nullptr) {
817+
jvm->DetachCurrentThread();
818+
}
819+
}
820+
} detachGuard{jvm, attachedHere};
821+
776822
std::string out;
777823
std::string contentType;
778824
int status = 0;
@@ -870,19 +916,9 @@ void ConfigureLoaderCallback(const v8::FunctionCallbackInfo<v8::Value>& info) {
870916
v8::String::Utf8Value utf8(isolate, importMapVal);
871917
if (*utf8) jsonStr = *utf8;
872918
} else if (importMapVal->IsObject()) {
873-
v8::Local<v8::Object> jsonObj =
874-
ctx->Global()
875-
->Get(ctx, ToV8String(isolate, "JSON"))
876-
.ToLocalChecked()
877-
.As<v8::Object>();
878-
v8::Local<v8::Function> stringify =
879-
jsonObj->Get(ctx, ToV8String(isolate, "stringify"))
880-
.ToLocalChecked()
881-
.As<v8::Function>();
882-
v8::Local<v8::Value> args[] = {importMapVal};
883-
v8::Local<v8::Value> result;
884-
if (stringify->Call(ctx, jsonObj, 1, args).ToLocal(&result) && result->IsString()) {
885-
v8::String::Utf8Value utf8(isolate, result);
919+
v8::Local<v8::String> stringified;
920+
if (v8::JSON::Stringify(ctx, importMapVal).ToLocal(&stringified)) {
921+
v8::String::Utf8Value utf8(isolate, stringified);
886922
if (*utf8) jsonStr = *utf8;
887923
}
888924
}

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

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1872,6 +1872,11 @@ bool MetadataNode::GetExtendLocation(v8::Isolate* isolate, string& extendLocatio
18721872
}
18731873
}
18741874

1875+
size_t queryOrFragment = normalized.find_first_of("?#");
1876+
if (queryOrFragment != string::npos) {
1877+
normalized.resize(queryOrFragment);
1878+
}
1879+
18751880
const string& appRoot = Constants::APP_ROOT_FOLDER_PATH;
18761881
if (!appRoot.empty()) {
18771882
stripPrefix(normalized, appRoot);
@@ -1889,10 +1894,17 @@ bool MetadataNode::GetExtendLocation(v8::Isolate* isolate, string& extendLocatio
18891894

18901895
fullPathToFile = normalized;
18911896

1892-
std::replace(fullPathToFile.begin(), fullPathToFile.end(), '/', '_');
1893-
std::replace(fullPathToFile.begin(), fullPathToFile.end(), '.', '_');
1894-
std::replace(fullPathToFile.begin(), fullPathToFile.end(), '-', '_');
1895-
std::replace(fullPathToFile.begin(), fullPathToFile.end(), ' ', '_');
1897+
for (char& ch : fullPathToFile) {
1898+
const unsigned char c = static_cast<unsigned char>(ch);
1899+
const bool isIdentifierChar =
1900+
(c >= 'A' && c <= 'Z') ||
1901+
(c >= 'a' && c <= 'z') ||
1902+
(c >= '0' && c <= '9') ||
1903+
ch == '_';
1904+
if (!isIdentifierChar) {
1905+
ch = '_';
1906+
}
1907+
}
18961908

18971909
std::vector<std::string> pathParts;
18981910
Util::SplitString(fullPathToFile, "_", pathParts);

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ static std::string NormalizeHttpModuleUrl(const std::string& path) {
5555
static std::string PromiseRejectionMessage(Isolate* isolate, Local<Promise> promise,
5656
const std::string& path) {
5757
std::string errorMessage = "Module evaluation promise rejected: " + path;
58+
TryCatch tc(isolate);
5859
Local<Value> reason = promise->Result();
5960
if (reason.IsEmpty()) {
6061
return errorMessage;
@@ -83,6 +84,9 @@ static std::string PromiseRejectionMessage(Isolate* isolate, Local<Promise> prom
8384
}
8485
}
8586
}
87+
if (tc.HasCaught()) {
88+
tc.Reset();
89+
}
8690
return errorMessage;
8791
}
8892

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,7 @@ static void PumpPendingHttpModuleGraph(v8::Isolate* isolate) {
329329
ALooper_pollOnce(10, nullptr, nullptr, nullptr);
330330
isolate->PerformMicrotaskCheckpoint();
331331
if (std::chrono::duration<double>(std::chrono::steady_clock::now() - start).count() > 60.0) {
332+
DEBUG_WRITE("PumpPendingHttpModuleGraph: deadline expired with pending async module work");
332333
break;
333334
}
334335
}

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ WorkerWrapper::WorkerWrapper(Isolate* parentIsolate, int workerId, std::string w
4545
isTerminating_(false),
4646
isDisposed_(false),
4747
drainRetryPending_(false),
48+
drainRetryAttempts_(0),
4849
javaLooperRef_(nullptr) {}
4950

5051
void WorkerWrapper::Start() {
@@ -164,7 +165,9 @@ void WorkerWrapper::DrainPendingTasks() {
164165
.ToLocal(&onMessageValue);
165166
if (!gotHandler || !onMessageValue->IsFunction()) {
166167
bool expected = false;
167-
if (drainRetryPending_.compare_exchange_strong(expected, true)) {
168+
if (drainRetryAttempts_ < kMaxDrainRetryAttempts &&
169+
drainRetryPending_.compare_exchange_strong(expected, true)) {
170+
++drainRetryAttempts_;
168171
const int workerId = workerId_;
169172
std::thread([workerId]() {
170173
usleep(50 * 1000);
@@ -174,8 +177,15 @@ void WorkerWrapper::DrainPendingTasks() {
174177
wrapper->SignalMessageDrain();
175178
}
176179
}).detach();
180+
return;
177181
}
178-
return;
182+
if (drainRetryAttempts_ < kMaxDrainRetryAttempts) {
183+
return;
184+
}
185+
// Retry budget exhausted: fall through so the per-message loop
186+
// logs the missing handler and drops the messages.
187+
} else {
188+
drainRetryAttempts_ = 0;
179189
}
180190
}
181191

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,8 @@ class WorkerWrapper : public std::enable_shared_from_this<WorkerWrapper> {
171171
std::atomic_bool isTerminating_;
172172
std::atomic_bool isDisposed_;
173173
std::atomic_bool drainRetryPending_;
174+
int drainRetryAttempts_ = 0;
175+
static constexpr int kMaxDrainRetryAttempts = 40;
174176

175177
ConcurrentQueue queue_;
176178

‎test-app/runtime/src/main/java/com/tns/DexFactory.java‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ && injectDexIntoClassLoader((BaseDexClassLoader) classLoader, jarFilePath)) {
194194
}
195195

196196
public Class<?> findClass(String className) throws ClassNotFoundException {
197-
String canonicalName = className.replace('/', '.').replace('$', '_');
197+
String canonicalName = className.replace('/', '.');
198198
if (logger.isEnabled()) {
199199
logger.write(canonicalName);
200200
}
@@ -204,7 +204,22 @@ public Class<?> findClass(String className) throws ClassNotFoundException {
204204
return existingClass;
205205
}
206206

207-
return classLoader.loadClass(canonicalName);
207+
String underscored = canonicalName.replace('$', '_');
208+
if (!underscored.equals(canonicalName)) {
209+
existingClass = this.injectedDexClasses.get(underscored);
210+
if (existingClass != null) {
211+
return existingClass;
212+
}
213+
}
214+
215+
try {
216+
return classLoader.loadClass(canonicalName);
217+
} catch (ClassNotFoundException e) {
218+
if (!underscored.equals(canonicalName)) {
219+
return classLoader.loadClass(underscored);
220+
}
221+
throw e;
222+
}
208223
}
209224

210225
public static String strJoin(String[] array, String separator) {

‎test-app/tools/try_to_find_test_result_file.js‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -131,14 +131,22 @@ async function checkForErrorActivity() {
131131
}
132132
}
133133

134+
function isCompleteJunitXml(text) {
135+
if (!text || typeof text !== "string") {
136+
return false;
137+
}
138+
const trimmed = text.trim();
139+
return /<testsuites[\s>]/.test(trimmed) && trimmed.includes("</testsuites>");
140+
}
141+
134142
async function tryPullResultsFile() {
135143
const { error } = await execAndStream(`${adbPrefix} pull ${resultsPath}`);
136144

137145
if (!error) {
138146
const fs = require("fs");
139147
try {
140148
const text = fs.readFileSync("android_unit_test_results.xml", "utf8");
141-
if (text.trimStart().startsWith("<?xml")) {
149+
if (isCompleteJunitXml(text)) {
142150
console.log("Tests results file found!");
143151
process.exit(0);
144152
}
@@ -153,7 +161,7 @@ async function tryPullResultsFile() {
153161
const { error: runAsError, stdout } = await execAndStream(
154162
`${adbPrefix} exec-out run-as ${appId} cat android_unit_test_results.xml`
155163
);
156-
if (!runAsError && stdout && stdout.trimStart().startsWith("<?xml")) {
164+
if (!runAsError && isCompleteJunitXml(stdout)) {
157165
const fs = require("fs");
158166
fs.writeFileSync(localPath, stdout);
159167
console.log("Tests results file found via run-as!");

0 commit comments

Comments
 (0)