url: Fix error type in constructor #19299

benjamingr · 2018-03-12T13:42:57Z

Currently whatwg URLs fail with an incorrect error when null is passed as the base. Adding a check before accessing a symbol for the URL makes the URL error correctly.

Add test for it.

Fixes: #19254

cc @targos @nodejs/url

benjamingr · 2018-03-12T13:43:49Z

About semverness - I feel like this is a semver-patch since it's a bugfig. Technically it changes an error message in an edge case.

targos · 2018-03-12T14:23:44Z

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

@@ -22,6 +22,7 @@ const failureTests = tests.filter((test) => test.failure).concat([
  { input: null },
  { input: new Date() },
  { input: new RegExp() },
+  { input: 'test', base: null},


Sorry, my example in the issue was not very good. This throws because 'test' is not a valid URL. With this change, a valid input with a base of null will not throw. To make it work, we still need to reach the line with base = new URL(base);

Thanks for clarifying!

Added two relevant examples below

@targos please take another look - I've changed the code since that review.

watilde · 2018-03-12T14:35:44Z

lib/internal/url.js

@@ -312,6 +312,7 @@ class URL {
    // toUSVString is not needed.
    input = `${input}`;
    if (base !== undefined &&
+        base !== null &&


According to the current specification of URL, it might not be needed to checking base as like instanceof since base is guaranteed to be a USVString. Also, in this case, it should display an error for parsing base = null, so I think we can simply write like the following:

if (base !== undefined) { base = new URL(base); } /* The result in my local is here: > new URL('test', null) internal/url.js:228 throw error; ^ TypeError [ERR_INVALID_URL]: Invalid URL: null at onParseError (internal/url.js:226:17) at parse (internal/url.js:235:3) at new URL (internal/url.js:317:5) at new URL (internal/url.js:315:14) at Object.<anonymous> (/Users/watilde/Development/tmp/1/a.js:2:1) at Module._compile (module.js:671:30) at Object.Module._extensions..js (module.js:682:10) at Module.load (module.js:582:32) at tryModuleLoad (module.js:522:12) at Function.Module._load (module.js:514:3) */

Refs: https://github.com/jsdom/whatwg-url/blob/master/lib/URL-impl.js#L12-L17
Spec: https://github.com/whatwg/url/blob/master/url.bs#L2592-L2599

Yeah you're right, the undefined check is also incorrect

Thanks for your quick update!

benjamingr · 2018-03-12T15:20:52Z

@watilde @targos please take a look, I've added a test for a valid URL with invalid base and changed the implementation to not check if the object passed in is a URL and build a new URL anyway.

I didn't notice any regression compared to the optimization done in https://github.com/nodejs/node/pull/11690/files - I can readd the check here but I wonder how common the case a URL is passed in as a baseUrl (which becomes a little slower) is vs a string (which becomes a little faster, and is the more important case IMO).

If either of you feels strongly about it I can re-add that check inside a !== null

Another question - I currently check arguments.length === 2 but I can check >= 2 instead which is what the browser does (although passing an unmeaningful number of parameters isn't too relevant IMO. Opinions?

targos · 2018-03-12T15:38:54Z

lib/internal/url.js

@@ -311,8 +311,7 @@ class URL {
  constructor(input, base) {
    // toUSVString is not needed.
    input = `${input}`;
-    if (base !== undefined &&
-        (!base[searchParams] || !base[searchParams][searchParams])) {
+    if (arguments.length === 2) {


I think we should still allow to explicitly pass undefined. I don't know what the spec says, but Chrome accepts it.

@targos https://github.com/whatwg/url/blob/master/url.bs#L2592-L2599 implies it doesn't allow it - but I'm all for Chrome compatibility and I'm not sure what the spec reads as.

What do you think about just removing the base[searchParams] and staying compatible with the URL impl at https://github.com/jsdom/whatwg-url/blob/master/lib/URL-impl.js#L12-L17 ?

Staying compatible with whatwg-url SGTM. I would actually like us to throw a more precise error saying which URL us invalid but that's out of scope for this PR

This is about how we can write "If base is given" and both sound good to me if CI gets green.

@TimothyGu What do you think about this?

watilde · 2018-03-12T22:06:14Z

ci: https://ci.nodejs.org/job/node-test-pull-request/13650/

domenic · 2018-03-13T00:36:48Z

To be clear, undefined is allowed, it's just then the spec steps for "base is given" do not run. (Undefined = not present, as always in JS.) So parsedBase gets left as null. In that case the first argument has to be an absolute URL, or the parser will fail (because that's what happens when you parse a relative URL against a null base).

benjamingr · 2018-03-13T08:52:28Z

@domenic and what should the behavior for null be according to the spec? Should it attempt to create a base URL and raise an error?

domenic · 2018-03-13T08:54:03Z

null is the same as "null", which is not parseable as a base URL, so yeah.

benjamingr · 2018-03-13T09:25:59Z

Ok, I basically ended up removing the optimization for passing an "already URL" as base (since I suspect that's not the interesting case anyway) - that removes the bug and I don't see a big performance difference.

I think given the different approaches it ended up being both the simplest and most "obviously correct"' one. Thanks everyone!

@targos

@targos please take another look - I've changed the code to what I believe is correct

targos

LGTM if the test is fixed

targos · 2018-03-13T09:27:51Z

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

@@ -22,6 +22,9 @@ const failureTests = tests.filter((test) => test.failure).concat([
  { input: null },
  { input: new Date() },
  { input: new RegExp() },
+  { input: 'test', base: null},
+  { input: 'http://nodejs.org', base: null},
+  { input: 'http://nodejs.org', base: undefined},


Does this still error? It should not.

Good point! I forgot to push, I'll run the tests again on my box

Done, also squashed.

benjamingr · 2018-03-13T09:51:49Z

CI: https://ci.nodejs.org/job/node-test-pull-request/13654/console

watilde · 2018-03-13T14:25:40Z

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

@@ -22,6 +22,8 @@ const failureTests = tests.filter((test) => test.failure).concat([
  { input: null },
  { input: new Date() },
  { input: new RegExp() },
+  { input: 'test', base: null},


nits: space is required before }. Also on line 26.

Very concerned that the linter didn't fail locally - thanks!

Currently whatwg URLs fail with an incorrect error when null is passed as the base. Adding a check before accessing a symbol for the URL makes the URL error correctly. Add test for it. PR-URL: nodejs#19299 Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Fixes: nodejs#19254

TimothyGu · 2018-03-22T00:20:03Z

@benjamingr would you be interested in submitting a pull request to Web Platform Tests as well, to ensure that the same bug doesn't happen in browsers?

TimothyGu · 2018-03-22T00:20:57Z

Ok, I basically ended up removing the optimization for passing an "already URL" as base (since I suspect that's not the interesting case anyway) - that removes the bug and I don't see a big performance difference.

Just wondering, how did you measure the performance difference? I don't believe we have a benchmark for this case yet.

targos · 2018-04-03T09:15:47Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/14011/

Currently whatwg URLs fail with an incorrect error when null is passed as the base. Adding a check before accessing a symbol for the URL makes the URL error correctly. Add test for it. PR-URL: nodejs#19299 Fixes: nodejs#19254 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR · 2018-04-09T22:29:14Z

Landed in cc6abc6 🎉

Currently whatwg URLs fail with an incorrect error when null is passed as the base. Adding a check before accessing a symbol for the URL makes the URL error correctly. Add test for it. PR-URL: #19299 Fixes: #19254 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Mar 12, 2018

cjihrig approved these changes Mar 12, 2018

View reviewed changes

targos previously requested changes Mar 12, 2018

View reviewed changes

watilde reviewed Mar 12, 2018

View reviewed changes

watilde approved these changes Mar 12, 2018

View reviewed changes

targos reviewed Mar 12, 2018

View reviewed changes

BridgeAR approved these changes Mar 12, 2018

View reviewed changes

benjamingr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 13, 2018

watilde approved these changes Mar 13, 2018

View reviewed changes

targos approved these changes Mar 13, 2018

View reviewed changes

benjamingr force-pushed the fix-whatwg-url-error-type branch from 6c7130c to f50f6f1 Compare March 13, 2018 09:51

lpinca approved these changes Mar 13, 2018

View reviewed changes

watilde reviewed Mar 13, 2018

View reviewed changes

benjamingr force-pushed the fix-whatwg-url-error-type branch from f50f6f1 to b735681 Compare March 13, 2018 16:32

TimothyGu approved these changes Mar 22, 2018

View reviewed changes

trivikr approved these changes Apr 3, 2018

View reviewed changes

jasnell approved these changes Apr 4, 2018

View reviewed changes

BridgeAR closed this Apr 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

url: Fix error type in constructor #19299

url: Fix error type in constructor #19299

benjamingr commented Mar 12, 2018 •

edited

Loading

benjamingr commented Mar 12, 2018

targos Mar 12, 2018

benjamingr Mar 12, 2018

benjamingr Mar 12, 2018

benjamingr Mar 13, 2018

watilde Mar 12, 2018 •

edited

Loading

benjamingr Mar 12, 2018

watilde Mar 12, 2018

benjamingr commented Mar 12, 2018 •

edited

Loading

targos Mar 12, 2018

benjamingr Mar 12, 2018

targos Mar 12, 2018

watilde Mar 12, 2018

targos Mar 12, 2018

watilde commented Mar 12, 2018

domenic commented Mar 13, 2018

benjamingr commented Mar 13, 2018

domenic commented Mar 13, 2018

benjamingr commented Mar 13, 2018 •

edited

Loading

targos left a comment

targos Mar 13, 2018

benjamingr Mar 13, 2018 •

edited

Loading

benjamingr Mar 13, 2018

benjamingr commented Mar 13, 2018

watilde Mar 13, 2018

benjamingr Mar 13, 2018

TimothyGu commented Mar 22, 2018

TimothyGu commented Mar 22, 2018

targos commented Apr 3, 2018

BridgeAR commented Apr 9, 2018

url: Fix error type in constructor #19299

url: Fix error type in constructor #19299

Conversation

benjamingr commented Mar 12, 2018 • edited Loading

benjamingr commented Mar 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

watilde Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr commented Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

watilde commented Mar 12, 2018

domenic commented Mar 13, 2018

benjamingr commented Mar 13, 2018

domenic commented Mar 13, 2018

benjamingr commented Mar 13, 2018 • edited Loading

targos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr commented Mar 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Mar 22, 2018

TimothyGu commented Mar 22, 2018

targos commented Apr 3, 2018

BridgeAR commented Apr 9, 2018

benjamingr commented Mar 12, 2018 •

edited

Loading

watilde Mar 12, 2018 •

edited

Loading

benjamingr commented Mar 12, 2018 •

edited

Loading

benjamingr commented Mar 13, 2018 •

edited

Loading

benjamingr Mar 13, 2018 •

edited

Loading