Skip to content

Commit a9c408a

Browse files
anonriganonrig
committed
url: improve invalid url performance
1 parent 7e12d0e commit a9c408a

File tree

9 files changed

+75
-35
lines changed

9 files changed

+75
-35
lines changed

‎benchmark/url/whatwg-url-validity.js‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
'use strict';
2+
const common = require('../common.js');
3+
const url = require('url');
4+
const URL = url.URL;
5+
6+
const bench = common.createBenchmark(main, {
7+
type: ['valid', 'invalid'],
8+
e: [1e5],
9+
});
10+
11+
// This benchmark is used to compare the `Invalid URL` path of the URL parser
12+
function main({ type, e }) {
13+
const url = type === 'valid' ? 'https://www.nodejs.org' : 'www.nodejs.org';
14+
bench.start();
15+
for (let i = 0; i < e; i++) {
16+
try {
17+
new URL(url);
18+
} catch {
19+
// do nothing
20+
}
21+
}
22+
bench.end(e);
23+
}

‎lib/internal/errors.js‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1370,8 +1370,13 @@ E('ERR_INVALID_SYNC_FORK_INPUT',
13701370
E('ERR_INVALID_THIS', 'Value of "this" must be of type %s', TypeError);
13711371
E('ERR_INVALID_TUPLE', '%s must be an iterable %s tuple', TypeError);
13721372
E('ERR_INVALID_URI', 'URI malformed', URIError);
1373-
E('ERR_INVALID_URL', function(input) {
1373+
E('ERR_INVALID_URL', function(input, base) {
13741374
this.input = input;
1375+
1376+
if (base != null) {
1377+
this.base = base;
1378+
}
1379+
13751380
// Don't include URL in message.
13761381
// (See https://github.com/nodejs/node/pull/38614)
13771382
return 'Invalid URL';

‎lib/internal/url.js‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -780,13 +780,7 @@ class URL {
780780
base = `${base}`;
781781
}
782782

783-
const href = bindingUrl.parse(input, base);
784-
785-
if (!href) {
786-
throw new ERR_INVALID_URL(input);
787-
}
788-
789-
this.#updateContext(href);
783+
this.#updateContext(bindingUrl.parse(input, base));
790784
}
791785

792786
[inspect.custom](depth, opts) {

‎src/env_properties.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@
5757
V(args_string, "args") \
5858
V(asn1curve_string, "asn1Curve") \
5959
V(async_ids_stack_string, "async_ids_stack") \
60+
V(base_string, "base") \
6061
V(bits_string, "bits") \
6162
V(block_list_string, "blockList") \
6263
V(buffer_string, "buffer") \

‎src/node_url.cc‎

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,35 @@ void BindingData::Format(const FunctionCallbackInfo<Value>& args) {
227227
.ToLocalChecked());
228228
}
229229

230+
void BindingData::ThrowInvalidURL(node::Environment* env,
231+
std::string_view input,
232+
std::optional<std::string> base) {
233+
Local<Value> err = ERR_INVALID_URL(env->isolate(), "Invalid URL");
234+
DCHECK(err->IsObject());
235+
236+
auto err_object = err.As<Object>();
237+
238+
USE(err_object->Set(env->context(),
239+
env->input_string(),
240+
v8::String::NewFromUtf8(env->isolate(),
241+
input.data(),
242+
v8::NewStringType::kNormal,
243+
input.size())
244+
.ToLocalChecked()));
245+
246+
if (base.has_value()) {
247+
USE(err_object->Set(env->context(),
248+
env->base_string(),
249+
v8::String::NewFromUtf8(env->isolate(),
250+
base.value().c_str(),
251+
v8::NewStringType::kNormal,
252+
base.value().size())
253+
.ToLocalChecked()));
254+
}
255+
256+
env->isolate()->ThrowException(err);
257+
}
258+
230259
void BindingData::Parse(const FunctionCallbackInfo<Value>& args) {
231260
CHECK_GE(args.Length(), 1);
232261
CHECK(args[0]->IsString()); // input
@@ -235,23 +264,24 @@ void BindingData::Parse(const FunctionCallbackInfo<Value>& args) {
235264
Realm* realm = Realm::GetCurrent(args);
236265
BindingData* binding_data = realm->GetBindingData<BindingData>();
237266
Isolate* isolate = realm->isolate();
267+
std::optional<std::string> base_{};
238268

239269
Utf8Value input(isolate, args[0]);
240270
ada::result<ada::url_aggregator> base;
241271
ada::url_aggregator* base_pointer = nullptr;
242272
if (args[1]->IsString()) {
243-
base =
244-
ada::parse<ada::url_aggregator>(Utf8Value(isolate, args[1]).ToString());
273+
base_ = Utf8Value(isolate, args[1]).ToString();
274+
base = ada::parse<ada::url_aggregator>(*base_);
245275
if (!base) {
246-
return args.GetReturnValue().Set(false);
276+
return ThrowInvalidURL(realm->env(), input.ToStringView(), base_);
247277
}
248278
base_pointer = &base.value();
249279
}
250280
auto out =
251281
ada::parse<ada::url_aggregator>(input.ToStringView(), base_pointer);
252282

253283
if (!out) {
254-
return args.GetReturnValue().Set(false);
284+
return ThrowInvalidURL(realm->env(), input.ToStringView(), base_);
255285
}
256286

257287
binding_data->UpdateComponents(out->get_components(), out->type);

‎src/node_url.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,9 @@ class BindingData : public SnapshotableObject {
7676
const ada::scheme::type type);
7777

7878
static v8::CFunction fast_can_parse_methods_[];
79+
static void ThrowInvalidURL(Environment* env,
80+
std::string_view input,
81+
std::optional<std::string> base);
7982
};
8083

8184
std::string FromFilePath(const std::string_view file_path);

‎src/pipe_wrap.cc‎

Lines changed: 5 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,6 @@ MaybeLocal<Object> PipeWrap::Instantiate(Environment* env,
6262
constructor->NewInstance(env->context(), 1, &type_value));
6363
}
6464

65-
6665
void PipeWrap::Initialize(Local<Object> target,
6766
Local<Value> unused,
6867
Local<Context> context,
@@ -71,8 +70,7 @@ void PipeWrap::Initialize(Local<Object> target,
7170
Isolate* isolate = env->isolate();
7271

7372
Local<FunctionTemplate> t = NewFunctionTemplate(isolate, New);
74-
t->InstanceTemplate()
75-
->SetInternalFieldCount(StreamBase::kInternalFieldCount);
73+
t->InstanceTemplate()->SetInternalFieldCount(StreamBase::kInternalFieldCount);
7674

7775
t->Inherit(LibuvStreamWrap::GetConstructorTemplate(env));
7876

@@ -102,9 +100,7 @@ void PipeWrap::Initialize(Local<Object> target,
102100
NODE_DEFINE_CONSTANT(constants, IPC);
103101
NODE_DEFINE_CONSTANT(constants, UV_READABLE);
104102
NODE_DEFINE_CONSTANT(constants, UV_WRITABLE);
105-
target->Set(context,
106-
env->constants_string(),
107-
constants).Check();
103+
target->Set(context, env->constants_string(), constants).Check();
108104
}
109105

110106
void PipeWrap::RegisterExternalReferences(ExternalReferenceRegistry* registry) {
@@ -152,7 +148,6 @@ void PipeWrap::New(const FunctionCallbackInfo<Value>& args) {
152148
new PipeWrap(env, args.This(), provider, ipc);
153149
}
154150

155-
156151
PipeWrap::PipeWrap(Environment* env,
157152
Local<Object> object,
158153
ProviderType provider,
@@ -163,7 +158,6 @@ PipeWrap::PipeWrap(Environment* env,
163158
// Suggestion: uv_pipe_init() returns void.
164159
}
165160

166-
167161
void PipeWrap::Bind(const FunctionCallbackInfo<Value>& args) {
168162
PipeWrap* wrap;
169163
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
@@ -172,7 +166,6 @@ void PipeWrap::Bind(const FunctionCallbackInfo<Value>& args) {
172166
args.GetReturnValue().Set(err);
173167
}
174168

175-
176169
#ifdef _WIN32
177170
void PipeWrap::SetPendingInstances(const FunctionCallbackInfo<Value>& args) {
178171
PipeWrap* wrap;
@@ -183,7 +176,6 @@ void PipeWrap::SetPendingInstances(const FunctionCallbackInfo<Value>& args) {
183176
}
184177
#endif
185178

186-
187179
void PipeWrap::Fchmod(const v8::FunctionCallbackInfo<v8::Value>& args) {
188180
PipeWrap* wrap;
189181
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
@@ -193,20 +185,17 @@ void PipeWrap::Fchmod(const v8::FunctionCallbackInfo<v8::Value>& args) {
193185
args.GetReturnValue().Set(err);
194186
}
195187

196-
197188
void PipeWrap::Listen(const FunctionCallbackInfo<Value>& args) {
198189
PipeWrap* wrap;
199190
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
200191
Environment* env = wrap->env();
201192
int backlog;
202193
if (!args[0]->Int32Value(env->context()).To(&backlog)) return;
203-
int err = uv_listen(reinterpret_cast<uv_stream_t*>(&wrap->handle_),
204-
backlog,
205-
OnConnection);
194+
int err = uv_listen(
195+
reinterpret_cast<uv_stream_t*>(&wrap->handle_), backlog, OnConnection);
206196
args.GetReturnValue().Set(err);
207197
}
208198

209-
210199
void PipeWrap::Open(const FunctionCallbackInfo<Value>& args) {
211200
Environment* env = Environment::GetCurrent(args);
212201

@@ -222,7 +211,6 @@ void PipeWrap::Open(const FunctionCallbackInfo<Value>& args) {
222211
args.GetReturnValue().Set(err);
223212
}
224213

225-
226214
void PipeWrap::Connect(const FunctionCallbackInfo<Value>& args) {
227215
Environment* env = Environment::GetCurrent(args);
228216

@@ -237,10 +225,7 @@ void PipeWrap::Connect(const FunctionCallbackInfo<Value>& args) {
237225

238226
ConnectWrap* req_wrap =
239227
new ConnectWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_PIPECONNECTWRAP);
240-
req_wrap->Dispatch(uv_pipe_connect,
241-
&wrap->handle_,
242-
*name,
243-
AfterConnect);
228+
req_wrap->Dispatch(uv_pipe_connect, &wrap->handle_, *name, AfterConnect);
244229

245230
TRACE_EVENT_NESTABLE_ASYNC_BEGIN1(TRACING_CATEGORY_NODE2(net, native),
246231
"connect",
@@ -251,7 +236,6 @@ void PipeWrap::Connect(const FunctionCallbackInfo<Value>& args) {
251236
args.GetReturnValue().Set(0); // uv_pipe_connect() doesn't return errors.
252237
}
253238

254-
255239
} // namespace node
256240

257241
NODE_BINDING_CONTEXT_AWARE_INTERNAL(pipe_wrap, node::PipeWrap::Initialize)

‎test/parallel/test-url-null-char.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,5 +4,5 @@ const assert = require('assert');
44

55
assert.throws(
66
() => { new URL('a\0b'); },
7-
{ input: 'a\0b' }
7+
{ code: 'ERR_INVALID_URL', input: 'a\0b' }
88
);

‎test/parallel/test-whatwg-url-custom-parsing.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ for (const test of failureTests) {
5555
() => new URL(test.input, test.base),
5656
(error) => {
5757
assert.throws(() => { throw error; }, expectedError);
58-
assert.strictEqual(`${error}`, 'TypeError [ERR_INVALID_URL]: Invalid URL');
58+
assert.strictEqual(`${error}`, 'TypeError: Invalid URL');
5959
assert.strictEqual(error.message, 'Invalid URL');
6060
return true;
6161
});

0 commit comments

Comments
 (0)