url: improve invalid url performance

PR-URL: #49692 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
nodejs · Sep 28, 2023 · 7d91a73 · 7d91a73
1 parent 6db0d3d
commit 7d91a73
Show file tree

Hide file tree

Showing 8 changed files with 70 additions and 14 deletions.
diff --git a/benchmark/url/whatwg-url-validity.js 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
@@ -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
@@ -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
@@ -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
@@ -227,6 +227,35 @@ void BindingData::Format(const FunctionCallbackInfo<Value>& args) {
                                 .ToLocalChecked());
 }
 
+void BindingData::ThrowInvalidURL(node::Environment* env,
+                                  std::string_view input,
+                                  std::optional<std::string> base) {
+  Local<Value> err = ERR_INVALID_URL(env->isolate(), "Invalid URL");
+  DCHECK(err->IsObject());
+
+  auto err_object = err.As<Object>();
+
+  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<Value>& args) {
   CHECK_GE(args.Length(), 1);
   CHECK(args[0]->IsString());  // input
@@ -235,23 +264,24 @@ void BindingData::Parse(const FunctionCallbackInfo<Value>& args) {
   Realm* realm = Realm::GetCurrent(args);
   BindingData* binding_data = realm->GetBindingData<BindingData>();
   Isolate* isolate = realm->isolate();
+  std::optional<std::string> base_{};
 
   Utf8Value input(isolate, args[0]);
   ada::result<ada::url_aggregator> base;
   ada::url_aggregator* base_pointer = nullptr;
   if (args[1]->IsString()) {
-    base =
-        ada::parse<ada::url_aggregator>(Utf8Value(isolate, args[1]).ToString());
+    base_ = Utf8Value(isolate, args[1]).ToString();
+    base = ada::parse<ada::url_aggregator>(*base_);
     if (!base) {
-      return args.GetReturnValue().Set(false);
+      return ThrowInvalidURL(realm->env(), input.ToStringView(), base_);
     }
     base_pointer = &base.value();
   }
   auto out =
       ada::parse<ada::url_aggregator>(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
@@ -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<std::string> 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
@@ -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
@@ -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;
     });