From 7d91a73764489911b17eb8d4670f0fec212bf3c6 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 26 Sep 2023 11:40:17 -0400 Subject: [PATCH] url: improve invalid url performance PR-URL: https://github.com/nodejs/node/pull/49692 Reviewed-By: Matteo Collina Reviewed-By: Robert Nagy Reviewed-By: Benjamin Gruenbaum --- benchmark/url/whatwg-url-validity.js | 23 +++++++++++ lib/internal/errors.js | 7 +++- lib/internal/url.js | 8 +--- src/env_properties.h | 1 + src/node_url.cc | 38 +++++++++++++++++-- src/node_url.h | 3 ++ test/parallel/test-url-null-char.js | 2 +- .../test-whatwg-url-custom-parsing.js | 2 +- 8 files changed, 70 insertions(+), 14 deletions(-) create mode 100644 benchmark/url/whatwg-url-validity.js diff --git a/benchmark/url/whatwg-url-validity.js b/benchmark/url/whatwg-url-validity.js new file mode 100644 index 00000000000000..6ba22336408fa1 --- /dev/null +++ b/benchmark/url/whatwg-url-validity.js @@ -0,0 +1,23 @@ +'use strict'; +const common = require('../common.js'); +const url = require('url'); +const URL = url.URL; + +const bench = common.createBenchmark(main, { + type: ['valid', 'invalid'], + e: [1e5], +}); + +// This benchmark is used to compare the `Invalid URL` path of the URL parser +function main({ type, e }) { + const url = type === 'valid' ? 'https://www.nodejs.org' : 'www.nodejs.org'; + bench.start(); + for (let i = 0; i < e; i++) { + try { + new URL(url); + } catch { + // do nothing + } + } + bench.end(e); +} diff --git a/lib/internal/errors.js b/lib/internal/errors.js index a8c2a9ea15db04..4e332e1ce18d16 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1370,8 +1370,13 @@ E('ERR_INVALID_SYNC_FORK_INPUT', E('ERR_INVALID_THIS', 'Value of "this" must be of type %s', TypeError); E('ERR_INVALID_TUPLE', '%s must be an iterable %s tuple', TypeError); E('ERR_INVALID_URI', 'URI malformed', URIError); -E('ERR_INVALID_URL', function(input) { +E('ERR_INVALID_URL', function(input, base = null) { this.input = input; + + if (base != null) { + this.base = base; + } + // Don't include URL in message. // (See https://github.com/nodejs/node/pull/38614) return 'Invalid URL'; diff --git a/lib/internal/url.js b/lib/internal/url.js index 37f67e6792959c..8d5926e8fcb9df 100644 --- a/lib/internal/url.js +++ b/lib/internal/url.js @@ -772,13 +772,7 @@ class URL { base = `${base}`; } - const href = bindingUrl.parse(input, base); - - if (!href) { - throw new ERR_INVALID_URL(input); - } - - this.#updateContext(href); + this.#updateContext(bindingUrl.parse(input, base)); } [inspect.custom](depth, opts) { diff --git a/src/env_properties.h b/src/env_properties.h index 970e25d926dbb2..24de82429ec892 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -54,6 +54,7 @@ V(args_string, "args") \ V(asn1curve_string, "asn1Curve") \ V(async_ids_stack_string, "async_ids_stack") \ + V(base_string, "base") \ V(bits_string, "bits") \ V(block_list_string, "blockList") \ V(buffer_string, "buffer") \ diff --git a/src/node_url.cc b/src/node_url.cc index 666492ca47cee3..89fcfec20f5685 100644 --- a/src/node_url.cc +++ b/src/node_url.cc @@ -227,6 +227,35 @@ void BindingData::Format(const FunctionCallbackInfo& args) { .ToLocalChecked()); } +void BindingData::ThrowInvalidURL(node::Environment* env, + std::string_view input, + std::optional base) { + Local err = ERR_INVALID_URL(env->isolate(), "Invalid URL"); + DCHECK(err->IsObject()); + + auto err_object = err.As(); + + USE(err_object->Set(env->context(), + env->input_string(), + v8::String::NewFromUtf8(env->isolate(), + input.data(), + v8::NewStringType::kNormal, + input.size()) + .ToLocalChecked())); + + if (base.has_value()) { + USE(err_object->Set(env->context(), + env->base_string(), + v8::String::NewFromUtf8(env->isolate(), + base.value().c_str(), + v8::NewStringType::kNormal, + base.value().size()) + .ToLocalChecked())); + } + + env->isolate()->ThrowException(err); +} + void BindingData::Parse(const FunctionCallbackInfo& args) { CHECK_GE(args.Length(), 1); CHECK(args[0]->IsString()); // input @@ -235,15 +264,16 @@ void BindingData::Parse(const FunctionCallbackInfo& args) { Realm* realm = Realm::GetCurrent(args); BindingData* binding_data = realm->GetBindingData(); Isolate* isolate = realm->isolate(); + std::optional base_{}; Utf8Value input(isolate, args[0]); ada::result base; ada::url_aggregator* base_pointer = nullptr; if (args[1]->IsString()) { - base = - ada::parse(Utf8Value(isolate, args[1]).ToString()); + base_ = Utf8Value(isolate, args[1]).ToString(); + base = ada::parse(*base_); if (!base) { - return args.GetReturnValue().Set(false); + return ThrowInvalidURL(realm->env(), input.ToStringView(), base_); } base_pointer = &base.value(); } @@ -251,7 +281,7 @@ void BindingData::Parse(const FunctionCallbackInfo& args) { ada::parse(input.ToStringView(), base_pointer); if (!out) { - return args.GetReturnValue().Set(false); + return ThrowInvalidURL(realm->env(), input.ToStringView(), base_); } binding_data->UpdateComponents(out->get_components(), out->type); diff --git a/src/node_url.h b/src/node_url.h index c485caa2eb0343..f3aa136a5b538d 100644 --- a/src/node_url.h +++ b/src/node_url.h @@ -76,6 +76,9 @@ class BindingData : public SnapshotableObject { const ada::scheme::type type); static v8::CFunction fast_can_parse_methods_[]; + static void ThrowInvalidURL(Environment* env, + std::string_view input, + std::optional base); }; std::string FromFilePath(const std::string_view file_path); diff --git a/test/parallel/test-url-null-char.js b/test/parallel/test-url-null-char.js index 468080844d534b..d81cbcfb6648d8 100644 --- a/test/parallel/test-url-null-char.js +++ b/test/parallel/test-url-null-char.js @@ -4,5 +4,5 @@ const assert = require('assert'); assert.throws( () => { new URL('a\0b'); }, - { input: 'a\0b' } + { code: 'ERR_INVALID_URL', input: 'a\0b' } ); diff --git a/test/parallel/test-whatwg-url-custom-parsing.js b/test/parallel/test-whatwg-url-custom-parsing.js index 905028fee3812c..cdeda59eec0c98 100644 --- a/test/parallel/test-whatwg-url-custom-parsing.js +++ b/test/parallel/test-whatwg-url-custom-parsing.js @@ -55,7 +55,7 @@ for (const test of failureTests) { () => new URL(test.input, test.base), (error) => { assert.throws(() => { throw error; }, expectedError); - assert.strictEqual(`${error}`, 'TypeError [ERR_INVALID_URL]: Invalid URL'); + assert.strictEqual(`${error}`, 'TypeError: Invalid URL'); assert.strictEqual(error.message, 'Invalid URL'); return true; });