Skip to content

Commit

Permalink
url: reuse existing context in href setter
Browse files Browse the repository at this point in the history
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 <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
TimothyGu authored and BridgeAR committed Dec 5, 2018
1 parent 0e88f44 commit 0d7ee19
Showing 3 changed files with 33 additions and 21 deletions.
38 changes: 17 additions & 21 deletions lib/internal/url.js
Original file line number Diff line number Diff line change
@@ -43,7 +43,7 @@ const {
domainToUnicode: _domainToUnicode,
encodeAuth,
toUSVString: _toUSVString,
parse: _parse,
parse,
setURLConstructor,
URL_FLAGS_CANNOT_BE_BASE,
URL_FLAGS_HAS_FRAGMENT,
@@ -243,14 +243,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];
@@ -319,10 +311,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]() {
@@ -445,7 +440,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
@@ -491,8 +487,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: {
@@ -555,7 +551,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: {
@@ -572,7 +568,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: {
@@ -592,7 +588,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: {
@@ -611,8 +607,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: {
@@ -635,7 +631,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);
@@ -669,7 +665,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: {
1 change: 1 addition & 0 deletions src/node_url.cc
Original file line number Diff line number Diff line change
@@ -1376,6 +1376,7 @@ void URL::Parse(const char* input,
else
break;
}
input = p;
len = end - p;
}

15 changes: 15 additions & 0 deletions test/parallel/test-whatwg-url-custom-href-side-effect.js
Original file line number Diff line number Diff line change
@@ -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);

0 comments on commit 0d7ee19

Please sign in to comment.