From 9fed0e071d4c6a6f3c17e371a904972ee6e3f166 Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Tue, 13 Nov 2018 14:34:07 -0800 Subject: [PATCH 1/5] Revert "url: make the context non-enumerable" This reverts commit 5e1bf058f4906c0a49c34c6a1353d0b34784c80a, as it causes major performance regressions during object construction. Refs: https://github.com/nodejs/node/pull/24218 --- lib/internal/url.js | 15 ++------------- ...est-whatwg-url-custom-no-enumerable-context.js | 14 -------------- 2 files changed, 2 insertions(+), 27 deletions(-) delete mode 100644 test/parallel/test-whatwg-url-custom-no-enumerable-context.js diff --git a/lib/internal/url.js b/lib/internal/url.js index 693f082aed6137..518c745b41e760 100644 --- a/lib/internal/url.js +++ b/lib/internal/url.js @@ -245,14 +245,7 @@ function onParseError(flags, input) { // 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() - }); - } + url[context] = new URLContext(); _parse(input.trim(), -1, base_context, undefined, onParseComplete.bind(url), onParseError); } @@ -1444,11 +1437,7 @@ function toPathIfFileURL(fileURLOrPath) { } function NativeURL(ctx) { - Object.defineProperty(this, context, { - enumerable: false, - configurable: false, - value: ctx - }); + this[context] = ctx; } NativeURL.prototype = URL.prototype; diff --git a/test/parallel/test-whatwg-url-custom-no-enumerable-context.js b/test/parallel/test-whatwg-url-custom-no-enumerable-context.js deleted file mode 100644 index f24a47b6d5c8f0..00000000000000 --- a/test/parallel/test-whatwg-url-custom-no-enumerable-context.js +++ /dev/null @@ -1,14 +0,0 @@ -'use strict'; -// This tests that the context of URL objects are not -// enumerable and thus considered by assert libraries. -// See https://github.com/nodejs/node/issues/24211 - -// Tests below are not from WPT. - -require('../common'); -const assert = require('assert'); - -assert.deepStrictEqual( - new URL('./foo', 'https://example.com/'), - new URL('https://example.com/foo') -); From 6cf1769ff819213bac467ffd7cee800e43137aae Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Tue, 13 Nov 2018 15:31:44 -0800 Subject: [PATCH 2/5] src: set HAS_USERNAME/PASSWORD more strictly Fixes: #24211 --- src/node_url.cc | 28 +++++++++++++------ .../test-whatwg-url-custom-deepequal.js | 18 ++++++++++++ 2 files changed, 38 insertions(+), 8 deletions(-) create mode 100644 test/parallel/test-whatwg-url-custom-deepequal.js diff --git a/src/node_url.cc b/src/node_url.cc index a3f22f7c983e7d..b1358e5ac81de0 100644 --- a/src/node_url.cc +++ b/src/node_url.cc @@ -1208,21 +1208,33 @@ inline url_data HarvestBase(Environment* env, Local 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 name) { + Local name, + bool empty_as_present) { Local value = base_obj->Get(env->context(), name).ToLocalChecked(); if (value->IsString()) { Utf8Value utf8value(env->isolate(), value.As()); (base.*member).assign(*utf8value, utf8value.length()); - base.flags |= flag; + if (empty_as_present || value.As()->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 port = base_obj->Get(env->context(), env->port_string()).ToLocalChecked(); diff --git a/test/parallel/test-whatwg-url-custom-deepequal.js b/test/parallel/test-whatwg-url-custom-deepequal.js new file mode 100644 index 00000000000000..9150b1561b7c77 --- /dev/null +++ b/test/parallel/test-whatwg-url-custom-deepequal.js @@ -0,0 +1,18 @@ +'use strict'; +// 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. + +require('../common'); +const assert = require('assert'); + +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') +); From 8be84ac58930db9c9250ae9b89e6866d3aa0dff5 Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Tue, 13 Nov 2018 15:53:35 -0800 Subject: [PATCH 3/5] url: reuse existing context in href setter Correctness-wise, this removes side effects in the href setter if parsing fails. Style-wise, this allows removing the parse() wrapper function around C++ _parse(). Also fix an existing bug with whitespace trimming in C++ that was previously circumvented by additionally trimming the input in JavaScript. Fixes: #24345 Refs: https://github.com/nodejs/node/pull/24218#issuecomment-438476591 --- lib/internal/url.js | 34 ++++++++----------- src/node_url.cc | 1 + ...test-whatwg-url-custom-href-side-effect.js | 15 ++++++++ 3 files changed, 31 insertions(+), 19 deletions(-) create mode 100644 test/parallel/test-whatwg-url-custom-href-side-effect.js diff --git a/lib/internal/url.js b/lib/internal/url.js index 518c745b41e760..c6d403fdd99a5e 100644 --- a/lib/internal/url.js +++ b/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,14 +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; - url[context] = 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]; @@ -318,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]() { @@ -444,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 @@ -490,7 +486,7 @@ Object.defineProperties(URL.prototype, { (ctx.host === '' || ctx.host === null)) { return; } - _parse(scheme, kSchemeStart, null, ctx, + parse(scheme, kSchemeStart, null, ctx, onParseProtocolComplete.bind(this)); } }, @@ -554,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: { @@ -571,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: { @@ -591,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: { @@ -610,7 +606,7 @@ Object.defineProperties(URL.prototype, { path = `${path}`; if (this[cannotBeBase]) return; - _parse(path, kPathStart, null, this[context], + parse(path, kPathStart, null, this[context], onParsePathComplete.bind(this)); } }, @@ -634,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); @@ -668,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: { diff --git a/src/node_url.cc b/src/node_url.cc index b1358e5ac81de0..c7abf79475b457 100644 --- a/src/node_url.cc +++ b/src/node_url.cc @@ -1375,6 +1375,7 @@ void URL::Parse(const char* input, else break; } + input = p; len = end - p; } diff --git a/test/parallel/test-whatwg-url-custom-href-side-effect.js b/test/parallel/test-whatwg-url-custom-href-side-effect.js new file mode 100644 index 00000000000000..f161f6bbe9546e --- /dev/null +++ b/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); From 19634c7785835f4ddcb8851b050fa5aedea097d1 Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Mon, 19 Nov 2018 16:54:03 -0800 Subject: [PATCH 4/5] url: simplify native URL object construction Co-authored-by: Joyee Cheung --- lib/internal/url.js | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/lib/internal/url.js b/lib/internal/url.js index c6d403fdd99a5e..fb7501ddc04198 100644 --- a/lib/internal/url.js +++ b/lib/internal/url.js @@ -1432,11 +1432,6 @@ function toPathIfFileURL(fileURLOrPath) { return fileURLToPath(fileURLOrPath); } -function NativeURL(ctx) { - this[context] = ctx; -} -NativeURL.prototype = URL.prototype; - function constructUrl(flags, protocol, username, password, host, port, path, query, fragment) { var ctx = new URLContext(); @@ -1449,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); From 61114b1c1fa96c5b1424ccd6f6c02efef82c841e Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Mon, 19 Nov 2018 16:56:48 -0800 Subject: [PATCH 5/5] fixup! url: reuse existing context in href setter Fix linting --- lib/internal/url.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/internal/url.js b/lib/internal/url.js index fb7501ddc04198..f55f5ebe407e0d 100644 --- a/lib/internal/url.js +++ b/lib/internal/url.js @@ -316,7 +316,7 @@ class URL { } this[context] = new URLContext(); parse(input, -1, base_context, undefined, onParseComplete.bind(this), - onParseError); + onParseError); } get [special]() { @@ -440,7 +440,7 @@ Object.defineProperties(URL.prototype, { // toUSVString is not needed. input = `${input}`; parse(input, -1, undefined, undefined, onParseComplete.bind(this), - onParseError); + onParseError); } }, origin: { // readonly @@ -487,7 +487,7 @@ Object.defineProperties(URL.prototype, { return; } parse(scheme, kSchemeStart, null, ctx, - onParseProtocolComplete.bind(this)); + onParseProtocolComplete.bind(this)); } }, username: { @@ -607,7 +607,7 @@ Object.defineProperties(URL.prototype, { if (this[cannotBeBase]) return; parse(path, kPathStart, null, this[context], - onParsePathComplete.bind(this)); + onParsePathComplete.bind(this)); } }, search: {