-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Start fixing bugs discovered by Node.js's Node-API tests #14501
base: main
Are you sure you want to change the base?
Changes from 171 commits
1f53597
4181393
eb8d465
e66ec2a
dfa2a6b
2583f33
afcf7b1
23dc0fe
85f617f
6169f10
6a440aa
ef4728c
49b2de9
1649c03
a0c2a73
ed4175b
216e5b3
2fee09f
b773e66
2d0e0c9
7be1bf3
8c571d8
6d1db2c
ea1ddb2
5bae294
e5ffd66
a240093
020c32b
d612cff
e5e643d
c44eb73
710f779
b2080c8
a7bc53b
b753e4b
b8aba83
bdcca41
e04f461
d29e72f
f71b440
600bc1c
3ba398f
43d7cfc
3587391
c28d419
71101e1
249227d
39b442b
528d9a6
6999978
19b0fed
73579e1
84c4f96
8b19e08
774bb89
8a0a88c
838ca00
059185f
197a26f
91a5231
a669ff1
5176ab5
fb6a48a
d2c4a9a
dc4177f
a1c4240
fe18b87
936ae5a
b6dfd89
2d96ec0
f15059f
dffc718
8bb8193
1293039
b5ed0f0
664c080
a8a2403
30fe8d5
7fab670
08116e4
03d945e
7110c07
1d5da9e
afd023a
7a20f51
066b1da
e3b5927
d8557ea
3303a5d
a60ae54
c659b3b
ce46947
a6d707a
1c06dbd
f5dc049
ab92fc5
3296a6e
d4b7102
07a217f
71f3089
0dce736
7978505
80b7426
a152557
0f29267
85dcebe
fceeb22
855b710
c30ef2c
07a3913
9f3b0f7
7c13e63
e34673c
86e421a
657f5b9
8b5fb34
1de2319
3a71be3
0bee1c9
1d8423e
9490c30
469be87
9ea9925
c17e05c
09a6a11
7993f4f
bd45a65
2335e35
6e7240b
563b3c0
d11a483
83f536f
86d4dbe
c20c0de
7b25ce1
a8fa566
ac8c6f0
5970006
ccd7275
440111f
b11d631
d3b509e
adc00e0
f54f4e6
9dbe40d
8358f4d
6dd369e
06d37bf
bb8b465
0e7ed99
544dd24
f439dac
e11a683
83a2c24
9fa480c
2646ea0
ed1f25e
f37df90
134f66c
2afb5e6
90852a3
f501143
f9718af
f73ef54
4103b73
2aee623
28830f0
d9c8f27
e5c5033
6603871
cf960b5
eef79ce
d090501
73e9866
3e085b5
ba5490d
b02fb24
d93122a
c38eca2
437d333
7f9935a
5949777
1e649b4
b0a30ca
01bbe30
dec572e
6ba0563
7a73f14
24a3f96
2406936
b191968
e0414d0
30eda1e
8169061
9426039
f78ac63
7a623fe
3fc6ad4
e817928
7886182
33d3732
4a10bf2
bb33176
1789364
1baa1b6
c1a25d0
769c6de
7ee91a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
#include "napi.h" | ||
|
||
#include "BunProcess.h" | ||
#include <JavaScriptCore/InternalFieldTuple.h> | ||
#include <JavaScriptCore/JSMicrotask.h> | ||
|
@@ -13,7 +15,6 @@ | |
#include "JavaScriptCore/PutPropertySlot.h" | ||
#include "ScriptExecutionContext.h" | ||
#include "headers-handwritten.h" | ||
#include "node_api.h" | ||
#include "ZigGlobalObject.h" | ||
#include "headers.h" | ||
#include "JSEnvironmentVariableMap.h" | ||
|
@@ -26,6 +27,7 @@ | |
#include <JavaScriptCore/LazyPropertyInlines.h> | ||
#include <JavaScriptCore/VMTrapsInlines.h> | ||
#include "wtf-bindings.h" | ||
#include "EventLoopTask.h" | ||
|
||
#include "ProcessBindingTTYWrap.h" | ||
#include "wtf/text/ASCIILiteral.h" | ||
|
@@ -82,6 +84,8 @@ typedef int mode_t; | |
#include <unistd.h> // setuid, getuid | ||
#endif | ||
|
||
#include <cstring> | ||
|
||
namespace Bun { | ||
|
||
using namespace JSC; | ||
|
@@ -274,6 +278,69 @@ extern "C" bool Bun__resolveEmbeddedNodeFile(void*, BunString*); | |
extern "C" HMODULE Bun__LoadLibraryBunString(BunString*); | ||
#endif | ||
|
||
/// Returns a pointer that needs to be freed with `delete[]`. | ||
static char* toFileURI(std::string_view path) | ||
{ | ||
auto needs_escape = [](char ch) { | ||
return !(('a' <= ch && ch <= 'z') || ('A' <= ch && ch <= 'Z') || ('0' <= ch && ch <= '9') | ||
|| ch == '_' || ch == '-' || ch == '.' || ch == '!' || ch == '~' || ch == '*' || ch == '\'' || ch == '(' || ch == ')' || ch == '/' || ch == ':'); | ||
}; | ||
|
||
auto to_hex = [](uint8_t nybble) -> char { | ||
if (nybble < 0xa) { | ||
return '0' + nybble; | ||
} | ||
|
||
return 'a' + (nybble - 0xa); | ||
}; | ||
|
||
size_t escape_count = 0; | ||
for (char ch : path) { | ||
#if OS(WINDOWS) | ||
if (needs_escape(ch) && ch != '\\') { | ||
#else | ||
if (needs_escape(ch)) { | ||
#endif | ||
++escape_count; | ||
} | ||
} | ||
|
||
#if OS(WINDOWS) | ||
#define FILE_URI_START "file:///" | ||
#else | ||
#define FILE_URI_START "file://" | ||
#endif | ||
|
||
const size_t string_size = sizeof(FILE_URI_START) + path.size() + 2 * escape_count; // null byte is included in the sizeof expression | ||
char* characters = new char[string_size]; | ||
strncpy(characters, FILE_URI_START, sizeof(FILE_URI_START)); | ||
size_t i = sizeof(FILE_URI_START) - 1; | ||
for (char ch : path) { | ||
#if OS(WINDOWS) | ||
if (ch == '\\') { | ||
characters[i++] = '/'; | ||
continue; | ||
} | ||
#endif | ||
if (needs_escape(ch)) { | ||
characters[i++] = '%'; | ||
characters[i++] = to_hex(static_cast<uint8_t>(ch) >> 4); | ||
characters[i++] = to_hex(ch & 0xf); | ||
} else { | ||
characters[i++] = ch; | ||
} | ||
} | ||
|
||
characters[i] = '\0'; | ||
ASSERT(i + 1 == string_size); | ||
return characters; | ||
} | ||
|
||
static char* toFileURI(std::span<const uint8_t> span) | ||
{ | ||
return toFileURI(std::string_view(reinterpret_cast<const char*>(span.data()), span.size())); | ||
} | ||
|
||
extern "C" size_t Bun__process_dlopen_count; | ||
|
||
JSC_DEFINE_HOST_FUNCTION(Process_functionDlopen, | ||
|
@@ -390,18 +457,17 @@ JSC_DEFINE_HOST_FUNCTION(Process_functionDlopen, | |
return JSValue::encode(jsUndefined()); | ||
} | ||
|
||
JSC::EncodedJSValue (*napi_register_module_v1)(JSC::JSGlobalObject* globalObject, | ||
JSC::EncodedJSValue exports); | ||
#if OS(WINDOWS) | ||
#define dlsym GetProcAddress | ||
#endif | ||
|
||
// TODO(@190n) look for node_register_module_vXYZ according to BuildOptions.reported_nodejs_version | ||
// (bun/src/env.zig:36) and the table at https://github.com/nodejs/node/blob/main/doc/abi_version_registry.json | ||
napi_register_module_v1 = reinterpret_cast<JSC::EncodedJSValue (*)(JSC::JSGlobalObject*, | ||
JSC::EncodedJSValue)>( | ||
auto napi_register_module_v1 = reinterpret_cast<napi_value (*)(napi_env, napi_value)>( | ||
dlsym(handle, "napi_register_module_v1")); | ||
|
||
auto node_api_module_get_api_version_v1 = reinterpret_cast<int32_t (*)()>(dlsym(handle, "node_api_module_get_api_version_v1")); | ||
|
||
#if OS(WINDOWS) | ||
#undef dlsym | ||
#endif | ||
|
@@ -412,14 +478,39 @@ JSC_DEFINE_HOST_FUNCTION(Process_functionDlopen, | |
#else | ||
dlclose(handle); | ||
#endif | ||
|
||
JSC::throwTypeError(globalObject, scope, "symbol 'napi_register_module_v1' not found in native module. Is this a Node API (napi) module?"_s); | ||
return {}; | ||
} | ||
|
||
// TODO(@heimskr): get the API version without node_api_module_get_api_version_v1 a different way | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the other way? curious There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think in some cases there's a global variable of type |
||
int module_version = 8; | ||
if (node_api_module_get_api_version_v1) { | ||
module_version = node_api_module_get_api_version_v1(); | ||
} | ||
|
||
NapiHandleScope handleScope(globalObject); | ||
|
||
EncodedJSValue exportsValue = JSC::JSValue::encode(exports); | ||
JSC::JSValue resultValue = JSValue::decode(napi_register_module_v1(globalObject, exportsValue)); | ||
|
||
char* filename_cstr = toFileURI(filename.utf8().span()); | ||
|
||
napi_module nmodule { | ||
.nm_version = module_version, | ||
.nm_flags = 0, | ||
.nm_filename = filename_cstr, | ||
.nm_register_func = nullptr, | ||
.nm_modname = "[no modname]", | ||
.nm_priv = nullptr, | ||
.reserved = {}, | ||
}; | ||
|
||
static_assert(sizeof(napi_value) == sizeof(EncodedJSValue), "EncodedJSValue must be reinterpretable as a pointer"); | ||
|
||
auto env = globalObject->makeNapiEnv(nmodule); | ||
env->filename = filename_cstr; | ||
|
||
JSC::JSValue resultValue = JSValue::decode(reinterpret_cast<EncodedJSValue>(napi_register_module_v1(env, reinterpret_cast<napi_value>(exportsValue)))); | ||
|
||
RETURN_IF_EXCEPTION(scope, {}); | ||
|
||
|
@@ -2701,6 +2792,18 @@ extern "C" void Bun__Process__queueNextTick1(GlobalObject* globalObject, Encoded | |
} | ||
JSC_DECLARE_HOST_FUNCTION(Bun__Process__queueNextTick1); | ||
|
||
extern "C" bool Bun__queueFinishNapiFinalizers(JSGlobalObject* jsGlobalObject) | ||
{ | ||
Zig::GlobalObject* globalObject = jsCast<Zig::GlobalObject*>(jsGlobalObject); | ||
if (globalObject->hasNapiFinalizers()) { | ||
globalObject->queueTask(new EventLoopTask([](ScriptExecutionContext&) { | ||
// TODO(@heimskr): do something more sensible | ||
})); | ||
Comment on lines
+2806
to
+2808
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment why this is needed too |
||
return true; | ||
} | ||
return false; | ||
} | ||
|
||
JSValue Process::constructNextTickFn(JSC::VM& vm, Zig::GlobalObject* globalObject) | ||
{ | ||
JSValue nextTickQueueObject; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this return the right string on Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not. It expects ASCII.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UTF8, actually, which it's receiving from the WTF::String. And the NAPI docs don't specify anything about a required encoding. The
const char **
type suggests to me that it's not expecting the output to be UTF16/UCS2.