Two URL parser fixes #24495

TimothyGu · 2018-11-19T18:40:38Z

This PR is an alternative to #24218 that resolves #24211 and #24345 without #24218's performance regression.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
~~documentation is changed or added~~
commit message follows commit guidelines

This reverts commit 5e1bf05, as it causes major performance regressions during object construction. Refs: nodejs#24218

Fixes: nodejs#24211

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: nodejs#24345 Refs: nodejs#24218 (comment)

nodejs-github-bot · 2018-11-19T18:40:40Z

@TimothyGu sadly an error occured when I tried to trigger a build :(

joyeecheung · 2018-11-19T20:14:55Z

lib/internal/url.js

-    configurable: false,
-    value: ctx
-  });
+  this[context] = ctx;


I am wondering if we can just remove this class and inline the calls

const url = new NativeURL(ctx); url[searchParams] = new URLSearchParams(); url[searchParams][context] = url;

with

const url = Object.create(URL.prototype); url[context] = ctx; const params = new URLSearchParams(); params[context] = url; url[searchParams] = params;

(Not sure about the performance implications, though, but it seems cleaner if we reduce one more class that looks like URL-related since this is only used by the C++ classes, and there are already many URL classes so it's a bit hard to find which one is the one you are looking for)

joyeecheung · 2018-11-19T20:36:07Z

CI: https://ci.nodejs.org/job/node-test-pull-request/18769/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/277/

Co-authored-by: Joyee Cheung <[email protected]>

Fix linting

cjihrig · 2018-11-20T21:05:40Z

lib/internal/url.js

@@ -325,10 +310,13 @@ class URL {
  constructor(input, base) {
    // toUSVString is not needed.
    input = `${input}`;
+    let base_context;


Should this be camelCase, for consistency?

Trott · 2018-11-24T20:33:16Z

CI: https://ci.nodejs.org/job/node-test-pull-request/18943/ (although it will need another CI if the camel-casing comment results in a change)

Trott · 2018-12-01T05:37:01Z

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

Trott · 2018-12-01T16:22:43Z

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19120/

Trott · 2018-12-01T16:53:27Z

Landed in e9de435...f0b6b39

This reverts commit 5e1bf05, as it causes major performance regressions during object construction. Refs: nodejs#24218 PR-URL: nodejs#24495 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fixes: nodejs#24211 PR-URL: nodejs#24495 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

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: nodejs#24345 Refs: nodejs#24218 (comment) PR-URL: nodejs#24495 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Co-authored-by: Joyee Cheung <[email protected]> PR-URL: nodejs#24495 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

This reverts commit 5e1bf05, as it causes major performance regressions during object construction. Refs: #24218 PR-URL: #24495 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fixes: #24211 PR-URL: #24495 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

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: #24218 (comment) PR-URL: #24495 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Co-authored-by: Joyee Cheung <[email protected]> PR-URL: #24495 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

This reverts commit 5e1bf05, as it causes major performance regressions during object construction. Refs: nodejs#24218 PR-URL: nodejs#24495 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fixes: nodejs#24211 PR-URL: nodejs#24495 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

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: nodejs#24345 Refs: nodejs#24218 (comment) PR-URL: nodejs#24495 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Co-authored-by: Joyee Cheung <[email protected]> PR-URL: nodejs#24495 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

This reverts commit 5e1bf05, as it causes major performance regressions during object construction. Refs: #24218 PR-URL: #24495 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fixes: #24211 PR-URL: #24495 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

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: #24218 (comment) PR-URL: #24495 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Co-authored-by: Joyee Cheung <[email protected]> PR-URL: #24495 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

This reverts commit 5e1bf05, as it causes major performance regressions during object construction. Refs: #24218 PR-URL: #24495 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fixes: #24211 PR-URL: #24495 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

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: #24218 (comment) PR-URL: #24495 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Co-authored-by: Joyee Cheung <[email protected]> PR-URL: #24495 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

TimothyGu added 3 commits November 19, 2018 10:25

Revert "url: make the context non-enumerable"

9fed0e0

This reverts commit 5e1bf05, as it causes major performance regressions during object construction. Refs: nodejs#24218

src: set HAS_USERNAME/PASSWORD more strictly

6cf1769

Fixes: nodejs#24211

TimothyGu requested a review from joyeecheung November 19, 2018 18:40

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 19, 2018

addaleax approved these changes Nov 19, 2018

View reviewed changes

joyeecheung approved these changes Nov 19, 2018

View reviewed changes

devsnek approved these changes Nov 19, 2018

View reviewed changes

TimothyGu and others added 2 commits November 19, 2018 16:54

url: simplify native URL object construction

19634c7

Co-authored-by: Joyee Cheung <[email protected]>

fixup! url: reuse existing context in href setter

61114b1

Fix linting

fhinkel approved these changes Nov 20, 2018

View reviewed changes

watilde approved these changes Nov 20, 2018

View reviewed changes

watilde added the url Issues and PRs related to the legacy built-in url module. label Nov 20, 2018

BridgeAR approved these changes Nov 20, 2018

View reviewed changes

TimothyGu added whatwg-url Issues and PRs related to the WHATWG URL implementation. and removed url Issues and PRs related to the legacy built-in url module. labels Nov 20, 2018

cjihrig approved these changes Nov 20, 2018

View reviewed changes

jasnell approved these changes Nov 21, 2018

View reviewed changes

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

Trott closed this Dec 1, 2018

BridgeAR mentioned this pull request Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 tasks

TimothyGu deleted the url-flags branch December 13, 2018 19:56

BethGriggs added the land-on-v10.x label Feb 12, 2019

BethGriggs mentioned this pull request Feb 12, 2019

v10.15.3 proposal #26063

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Two URL parser fixes #24495

Two URL parser fixes #24495

TimothyGu commented Nov 19, 2018

nodejs-github-bot commented Nov 19, 2018

joyeecheung Nov 19, 2018

joyeecheung commented Nov 19, 2018

cjihrig Nov 20, 2018

Trott commented Nov 24, 2018

Trott commented Dec 1, 2018

Trott commented Dec 1, 2018

Trott commented Dec 1, 2018

Two URL parser fixes #24495

Two URL parser fixes #24495

Conversation

TimothyGu commented Nov 19, 2018

Checklist

nodejs-github-bot commented Nov 19, 2018

joyeecheung Nov 19, 2018

Choose a reason for hiding this comment

joyeecheung commented Nov 19, 2018

cjihrig Nov 20, 2018

Choose a reason for hiding this comment

Trott commented Nov 24, 2018

Trott commented Dec 1, 2018

Trott commented Dec 1, 2018

Trott commented Dec 1, 2018