Two URL parser fixes

lib/internal/url.js

@@ -42,7 +42,7 @@ const {
   domainToUnicode: _domainToUnicode,
   encodeAuth,
   toUSVString: _toUSVString,
-  parse: _parse,
+  parse,
   setURLConstructor,
   URL_FLAGS_CANNOT_BE_BASE,
   URL_FLAGS_HAS_FRAGMENT,
@@ -242,21 +242,6 @@ function onParseError(flags, input) {
   throw error;
 }
 
-// Reused by URL constructor and URL#href setter.
-function parse(url, input, base) {
-  const base_context = base ? base[context] : undefined;
-  // In the URL#href setter
-  if (!url[context]) {
-    Object.defineProperty(url, context, {
-      enumerable: false,
-      configurable: false,
-      value: new URLContext()
-    });
-  }
-  _parse(input.trim(), -1, base_context, undefined,
-         onParseComplete.bind(url), onParseError);
-}
-
 function onParseProtocolComplete(flags, protocol, username, password,
                                  host, port, path, query, fragment) {
   const ctx = this[context];
@@ -325,10 +310,13 @@ class URL {
   constructor(input, base) {
     // toUSVString is not needed.
     input = `${input}`;
+    let base_context;
     if (base !== undefined) {
-      base = new URL(base);
+      base_context = new URL(base)[context];
     }
-    parse(this, input, base);
+    this[context] = new URLContext();
+    parse(input, -1, base_context, undefined, onParseComplete.bind(this),
+          onParseError);
   }
 
   get [special]() {
@@ -451,7 +439,8 @@ Object.defineProperties(URL.prototype, {
     set(input) {
       // toUSVString is not needed.
       input = `${input}`;
-      parse(this, input);
+      parse(input, -1, undefined, undefined, onParseComplete.bind(this),
+            onParseError);
     }
   },
   origin: {  // readonly
@@ -497,8 +486,8 @@ Object.defineProperties(URL.prototype, {
           (ctx.host === '' || ctx.host === null)) {
         return;
       }
-      _parse(scheme, kSchemeStart, null, ctx,
-             onParseProtocolComplete.bind(this));
+      parse(scheme, kSchemeStart, null, ctx,
+            onParseProtocolComplete.bind(this));
     }
   },
   username: {
@@ -561,7 +550,7 @@ Object.defineProperties(URL.prototype, {
         // Cannot set the host if cannot-be-base is set
         return;
       }
-      _parse(host, kHost, null, ctx, onParseHostComplete.bind(this));
+      parse(host, kHost, null, ctx, onParseHostComplete.bind(this));
     }
   },
   hostname: {
@@ -578,7 +567,7 @@ Object.defineProperties(URL.prototype, {
         // Cannot set the host if cannot-be-base is set
         return;
       }
-      _parse(host, kHostname, null, ctx, onParseHostnameComplete.bind(this));
+      parse(host, kHostname, null, ctx, onParseHostnameComplete.bind(this));
     }
   },
   port: {
@@ -598,7 +587,7 @@ Object.defineProperties(URL.prototype, {
         ctx.port = null;
         return;
       }
-      _parse(port, kPort, null, ctx, onParsePortComplete.bind(this));
+      parse(port, kPort, null, ctx, onParsePortComplete.bind(this));
     }
   },
   pathname: {
@@ -617,8 +606,8 @@ Object.defineProperties(URL.prototype, {
       path = `${path}`;
       if (this[cannotBeBase])
         return;
-      _parse(path, kPathStart, null, this[context],
-             onParsePathComplete.bind(this));
+      parse(path, kPathStart, null, this[context],
+            onParsePathComplete.bind(this));
     }
   },
   search: {
@@ -641,7 +630,7 @@ Object.defineProperties(URL.prototype, {
         ctx.query = '';
         ctx.flags |= URL_FLAGS_HAS_QUERY;
         if (search) {
-          _parse(search, kQuery, null, ctx, onParseSearchComplete.bind(this));
+          parse(search, kQuery, null, ctx, onParseSearchComplete.bind(this));
         }
       }
       initSearchParams(this[searchParams], search);
@@ -675,7 +664,7 @@ Object.defineProperties(URL.prototype, {
       if (hash[0] === '#') hash = hash.slice(1);
       ctx.fragment = '';
       ctx.flags |= URL_FLAGS_HAS_FRAGMENT;
-      _parse(hash, kFragment, null, ctx, onParseHashComplete.bind(this));
+      parse(hash, kFragment, null, ctx, onParseHashComplete.bind(this));
     }
   },
   toJSON: {
@@ -1443,15 +1432,6 @@ function toPathIfFileURL(fileURLOrPath) {
   return fileURLToPath(fileURLOrPath);
 }
 
-function NativeURL(ctx) {
-  Object.defineProperty(this, context, {
-    enumerable: false,
-    configurable: false,
-    value: ctx
-  });
-}
-NativeURL.prototype = URL.prototype;
-
 function constructUrl(flags, protocol, username, password,
                       host, port, path, query, fragment) {
   var ctx = new URLContext();
@@ -1464,10 +1444,13 @@ function constructUrl(flags, protocol, username, password,
   ctx.query = query;
   ctx.fragment = fragment;
   ctx.host = host;
-  const url = new NativeURL(ctx);
-  url[searchParams] = new URLSearchParams();
-  url[searchParams][context] = url;
-  initSearchParams(url[searchParams], query);
+
+  const url = Object.create(URL.prototype);
+  url[context] = ctx;
+  const params = new URLSearchParams();
+  url[searchParams] = params;
+  params[context] = url;
+  initSearchParams(params, query);
   return url;
 }
 setURLConstructor(constructUrl);

src/node_url.cc

@@ -1208,21 +1208,33 @@ inline url_data HarvestBase(Environment* env, Local<Object> base_obj) {
       base_obj->Get(env->context(), env->scheme_string()).ToLocalChecked();
   base.scheme = Utf8Value(env->isolate(), scheme).out();
 
-  auto GetStr = [&](std::string url_data::* member,
+  auto GetStr = [&](std::string url_data::*member,
                     int flag,
-                    Local<String> name) {
+                    Local<String> name,
+                    bool empty_as_present) {
     Local<Value> value = base_obj->Get(env->context(), name).ToLocalChecked();
     if (value->IsString()) {
       Utf8Value utf8value(env->isolate(), value.As<String>());
       (base.*member).assign(*utf8value, utf8value.length());
-      base.flags |= flag;
+      if (empty_as_present || value.As<String>()->Length() != 0) {
+        base.flags |= flag;
+      }
     }
   };
-  GetStr(&url_data::username, URL_FLAGS_HAS_USERNAME, env->username_string());
-  GetStr(&url_data::password, URL_FLAGS_HAS_PASSWORD, env->password_string());
-  GetStr(&url_data::host, URL_FLAGS_HAS_HOST, env->host_string());
-  GetStr(&url_data::query, URL_FLAGS_HAS_QUERY, env->query_string());
-  GetStr(&url_data::fragment, URL_FLAGS_HAS_FRAGMENT, env->fragment_string());
+  GetStr(&url_data::username,
+         URL_FLAGS_HAS_USERNAME,
+         env->username_string(),
+         false);
+  GetStr(&url_data::password,
+         URL_FLAGS_HAS_PASSWORD,
+         env->password_string(),
+         false);
+  GetStr(&url_data::host, URL_FLAGS_HAS_HOST, env->host_string(), true);
+  GetStr(&url_data::query, URL_FLAGS_HAS_QUERY, env->query_string(), true);
+  GetStr(&url_data::fragment,
+         URL_FLAGS_HAS_FRAGMENT,
+         env->fragment_string(),
+         true);
 
   Local<Value> port =
       base_obj->Get(env->context(), env->port_string()).ToLocalChecked();
@@ -1363,6 +1375,7 @@ void URL::Parse(const char* input,
       else
         break;
     }
+    input = p;
     len = end - p;
   }
 

test/parallel/test-whatwg-url-custom-no-enumerable-context.js → test/parallel/test-whatwg-url-custom-deepequal.js

@@ -1,6 +1,6 @@
 'use strict';
-// This tests that the context of URL objects are not
-// enumerable and thus considered by assert libraries.
+// This tests that the internal flags in URL objects are consistent, as manifest
+// through assert libraries.
 // See https://github.com/nodejs/node/issues/24211
 
 // Tests below are not from WPT.
@@ -12,3 +12,7 @@ assert.deepStrictEqual(
   new URL('./foo', 'https://example.com/'),
   new URL('https://example.com/foo')
 );
+assert.deepStrictEqual(
+  new URL('./foo', 'https://user:pass@example.com/'),
+  new URL('https://user:pass@example.com/foo')
+);

test/parallel/test-whatwg-url-custom-href-side-effect.js

@@ -0,0 +1,15 @@
+'use strict';
+
+// Tests below are not from WPT.
+const common = require('../common');
+const assert = require('assert');
+
+const ref = new URL('http://example.com/path');
+const url = new URL('http://example.com/path');
+common.expectsError(() => {
+  url.href = '';
+}, {
+  type: TypeError
+});
+
+assert.deepStrictEqual(url, ref);