Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/jsc/bindings/BunString.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -919,8 +919,11 @@ extern "C" JSC::EncodedJSValue JSC__JSValue__upsertBunStringArray(
} else {
// Create new array with both values
JSC::JSArray* array = JSC::constructEmptyArray(global, nullptr, 2);
RETURN_IF_EXCEPTION(scope, {});
array->putDirectIndex(global, 0, existingValue);
RETURN_IF_EXCEPTION(scope, {});
array->putDirectIndex(global, 1, newValue);
RETURN_IF_EXCEPTION(scope, {});
target->putDirect(vm, id, array, 0);
}
} else {
Expand Down
5 changes: 5 additions & 0 deletions src/jsc/bindings/ProcessBindingHTTPParser.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "ProcessBindingHTTPParser.h"
#include "ZigGlobalObject.h"
#include "JavaScriptCore/TopExceptionScope.h"
#include "llhttp/llhttp.h"

namespace Bun {
Expand All @@ -9,8 +10,10 @@ using namespace JSC;
static JSValue ProcessBindingHTTPParser_methods(VM& vm, JSObject* binding)
{
JSGlobalObject* globalObject = binding->globalObject();
auto scope = DECLARE_TOP_EXCEPTION_SCOPE(vm);

JSArray* methods = constructEmptyArray(globalObject, nullptr, 35);
RETURN_IF_EXCEPTION(scope, {});

int index = 0;
#define FOR_EACH_METHOD(num, name, string) \
Expand All @@ -24,8 +27,10 @@ static JSValue ProcessBindingHTTPParser_methods(VM& vm, JSObject* binding)
static JSValue ProcessBindingHTTPParser_allMethods(VM& vm, JSObject* binding)
{
JSGlobalObject* globalObject = binding->globalObject();
auto scope = DECLARE_TOP_EXCEPTION_SCOPE(vm);

JSArray* methods = constructEmptyArray(globalObject, nullptr, 47);
RETURN_IF_EXCEPTION(scope, {});

int index = 0;
#define FOR_EACH_METHOD(num, name, string) \
Expand Down
30 changes: 20 additions & 10 deletions src/jsc/bindings/ProcessBindingUV.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -153,20 +153,30 @@ JSC_DEFINE_HOST_FUNCTION(jsErrname, (JSGlobalObject * globalObject, JSC::CallFra
JSC_DEFINE_HOST_FUNCTION(jsGetErrorMap, (JSGlobalObject * globalObject, JSC::CallFrame* callFrame))
{
auto& vm = JSC::getVM(globalObject);
auto map = JSC::JSMap::create(vm, globalObject->mapStructure());
auto scope = DECLARE_THROW_SCOPE(vm);
auto* map = JSC::JSMap::create(vm, globalObject->mapStructure());

// Inlining each of these via macros costs like 300 KB.
const auto putProperty = [](JSC::VM& vm, JSC::JSMap* map, JSC::JSGlobalObject* globalObject, ASCIILiteral name, int value, ASCIILiteral desc) -> void {
auto arr = JSC::constructEmptyArray(globalObject, static_cast<JSC::ArrayAllocationProfile*>(nullptr), 2);
// RETURN_IF_EXCEPTION
struct Entry {
ASCIILiteral name;
int value;
ASCIILiteral desc;
};
static constexpr Entry entries[] = {
#define ENTRY(name, desc) { #name##_s, UV_##name, desc##_s },
BUN_UV_ERRNO_MAP(ENTRY)
#undef ENTRY
};

for (const auto& [name, value, desc] : entries) {
auto* arr = JSC::constructEmptyArray(globalObject, static_cast<JSC::ArrayAllocationProfile*>(nullptr), 2);
RETURN_IF_EXCEPTION(scope, {});
arr->putDirectIndex(globalObject, 0, JSC::jsString(vm, String(name)));
RETURN_IF_EXCEPTION(scope, {});
arr->putDirectIndex(globalObject, 1, JSC::jsString(vm, String(desc)));
RETURN_IF_EXCEPTION(scope, {});
map->set(globalObject, JSC::jsNumber(value), arr);
};

#define PUT_PROPERTY(name, desc) putProperty(vm, map, globalObject, #name##_s, UV_##name, desc##_s);
BUN_UV_ERRNO_MAP(PUT_PROPERTY)
#undef PUT_PROPERTY
RETURN_IF_EXCEPTION(scope, {});
}

return JSValue::encode(map);
}
Expand Down
4 changes: 2 additions & 2 deletions src/runtime/bake/DevServer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2685,7 +2685,7 @@ impl DevServer {
// TODO: lazy structure caching since we are making these objects a lot
let global = self.vm().global();
let params_js_value = if self.router.match_slow(pathname, &mut params).is_some() {
params.to_js(global)
params.to_js(global)?
Comment thread
coderabbitai[bot] marked this conversation as resolved.
} else {
JSValue::NULL
};
Expand Down Expand Up @@ -6758,7 +6758,7 @@ fn new_route_params_for_bundle_promise(
route_index.get()
)));
}
let params_js_value = params.to_js(global);
let params_js_value = params.to_js(global)?;

// SAFETY: `dev_ptr` is live; `framework_bundle` points into
// `(*dev_ptr).route_bundles[route_bundle_index].data` and its reborrow is
Expand Down
23 changes: 11 additions & 12 deletions src/runtime/bake/FrameworkRouter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1210,27 +1210,24 @@ impl MatchedParams {

/// Convert the matched params to a JavaScript object
/// Returns null if there are no params
pub fn to_js(&self, global: &JSGlobalObject) -> JSValue {
pub fn to_js(&self, global: &JSGlobalObject) -> JsResult<JSValue> {
let params_array = self.params.const_slice();

if params_array.is_empty() {
return JSValue::NULL;
return Ok(JSValue::NULL);
}

// Create a JavaScript object with params
let obj = JSValue::create_empty_object(global, params_array.len());
for param in params_array {
let key_str = bun_core::String::clone_utf8(param.key.slice());
let value_str = bun_core::String::clone_utf8(param.value.slice());
let key_str =
bun_core::OwnedString::new(bun_core::String::clone_utf8(param.key.slice()));
let value_str =
bun_core::OwnedString::new(bun_core::String::clone_utf8(param.value.slice()));

obj.put_bun_string_one_or_array(
global,
&key_str,
value_str.to_js(global).expect("unreachable"),
)
.expect("unreachable");
obj.put_bun_string_one_or_array(global, &key_str, value_str.to_js(global)?)?;
}
obj
Ok(obj)
}
}

Expand Down Expand Up @@ -1872,7 +1869,9 @@ impl JSFrameworkRouter {
JSValue::create_empty_object(global, params_out.params.len() as usize);
for param in params_out.params.slice() {
// key/value borrow from `path`/pattern, both live here (RawSlice invariant)
let value_str = bun_core::String::clone_utf8(param.value.slice());
let value_str = bun_core::OwnedString::new(bun_core::String::clone_utf8(
param.value.slice(),
));
params_obj.put(global, param.key.slice(), value_str.to_js(global)?);
}
params_obj
Expand Down
51 changes: 50 additions & 1 deletion test/bake/framework-router.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { frameworkRouterInternals } from "bun:internal-for-testing";
import { describe, expect, test } from "bun:test";
import { tempDir } from "harness";
import { bunEnv, bunExe, tempDir } from "harness";
import path from "path";

const { parseRoutePattern, FrameworkRouter } = frameworkRouterInternals;
Expand Down Expand Up @@ -133,3 +133,52 @@ test("discovers from filesystem paths", () => {
],
});
});

test("match() releases the param strings it creates", async () => {
using dir = tempDir("fsr-params-leak", {
"[a]/[b]/[c]/[d].tsx": "1",
});

// Each matched param value is copied into a WTF string that the JS string
// then refs; the copy's own ref has to be released or every match leaks all
// four values (4 x 256 KiB here, about 200 MiB over the measured loop).
await using proc = Bun.spawn({
cmd: [
bunExe(),
"--smol",
"-e",
/* js */ `
const { frameworkRouterInternals } = require("bun:internal-for-testing");
const rss = process.platform === "darwin" && typeof Bun.unsafe.memoryFootprint === "function" ? Bun.unsafe.memoryFootprint : process.memoryUsage.rss;
const router = new frameworkRouterInternals.FrameworkRouter({ root: ${JSON.stringify(String(dir))}, style: "nextjs-pages" });
const segment = Buffer.alloc(256 * 1024, "x").toString();
const url = "/" + segment + "/" + segment + "/" + segment + "/" + segment;
const lengths = {};
for (const [name, value] of Object.entries(router.match(url).params)) lengths[name] = value.length;
console.log(JSON.stringify(lengths));
for (let i = 0; i < 20; i++) router.match(url);
Bun.gc(true);
const before = rss();
for (let i = 0; i < 200; i++) router.match(url);
Bun.gc(true);
const growthMB = (rss() - before) / 1024 / 1024;
if (growthMB > 64) throw new Error("leaked " + growthMB.toFixed(2) + "MB");
`,
],
env: {
...bunEnv,
// Under ASAN freed allocations sit in the quarantine (default
// quarantine_size_mb=256) instead of leaving RSS, which would hide the
// difference between releasing and leaking. Ignored by non-ASAN builds.
ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "quarantine_size_mb=0"].filter(Boolean).join(":"),
},
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout, stderr, exitCode }).toEqual({
stdout: '{"a":262144,"b":262144,"c":262144,"d":262144}\n',
stderr: "",
exitCode: 0,
});
});
Loading