Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

url: call toString before valueOf when stringifying #11737

Closed
wants to merge 1 commit into from

Conversation

@TimothyGu
Copy link
Member

commented Mar 8, 2017

The ES addition operator calls the ToPrimitive() abstract operation without hint String, leading a subsequent OrdinaryToPrimitive() to call valueOf() first on an object rather than the desired toString().

Instead, use template literals which directly call ToString() abstract operation, per Web IDL spec.

This issue is particularly visible in the example below:

const { URLSearchParams } = url;
const searchParams = new URLSearchParams();
searchParams.set('name', {
  toString() { return 'toString'; },
  valueOf() { return 'valueOf'; }
});
console.log(searchParams.get('name'));

In the browser, the above prints toString, while in current master it prints valueOf.

Fixes: b610a4d "url: enforce valid UTF-8 in WHATWG parser"
Refs: b610a4d#commitcomment-21200056
Refs: https://tc39.github.io/ecma262/#sec-addition-operator-plus-runtime-semantics-evaluation
Refs: https://tc39.github.io/ecma262/#sec-template-literals-runtime-semantics-evaluation

/cc @nodejs/url

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

url

TimothyGu referenced this pull request Mar 8, 2017
url: enforce valid UTF-8 in WHATWG parser
This commit implements the Web IDL USVString conversion, which mandates
all unpaired Unicode surrogates be turned into U+FFFD REPLACEMENT
CHARACTER. It also disallows Symbols to be used as USVString per spec.

Certain functions call into C++ methods in the binding that use the
Utf8Value class to access string arguments. Utf8Value already does the
normalization using V8's String::Write, so in those cases, instead of
doing the full USVString normalization, only a symbol check is done
(`'' + val`, which uses ES's ToString, versus `String()` which has
special provisions for symbols).

PR-URL: #11436
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos
targos approved these changes Mar 8, 2017
@mscdex

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2017

Second commit message is a tad too long.

@watilde
watilde approved these changes Mar 8, 2017
@jasnell
jasnell approved these changes Mar 8, 2017
@jasnell

This comment has been minimized.

url: call toString before valueOf in stringifying
The ES addition operator calls the ToPrimitive() abstract operation
without hint String, leading a subsequent OrdinaryToPrimitive() to call
valueOf() first on an object rather than the desired toString().

Instead, use template literals which directly call ToString() abstract
operation, per Web IDL spec.

Ref: b610a4d#commitcomment-21200056
Ref: https://tc39.github.io/ecma262/#sec-addition-operator-plus-runtime-semantics-evaluation
Ref: https://tc39.github.io/ecma262/#sec-template-literals-runtime-semantics-evaluation

@TimothyGu TimothyGu force-pushed the TimothyGu:url-tostring branch to 3aaec8b Mar 11, 2017

@TimothyGu

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2017

Commit message fixed.

One more CI: https://ci.nodejs.org/job/node-test-pull-request/6797/

@TimothyGu

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2017

Landed in 99b27ce.

@TimothyGu TimothyGu closed this Mar 11, 2017

@TimothyGu TimothyGu deleted the TimothyGu:url-tostring branch Mar 11, 2017

TimothyGu added a commit that referenced this pull request Mar 11, 2017
url: prioritize toString when stringifying
The ES addition operator calls the ToPrimitive() abstract operation
without hint String, leading a subsequent OrdinaryToPrimitive() to call
valueOf() first on an object rather than the desired toString().

Instead, use template literals which directly call ToString() abstract
operation, per Web IDL spec.

PR-URL: #11737
Fixes: b610a4d "url: enforce valid UTF-8 in WHATWG parser"
Refs: b610a4d#commitcomment-21200056
Refs: https://tc39.github.io/ecma262/#sec-addition-operator-plus-runtime-semantics-evaluation
Refs: https://tc39.github.io/ecma262/#sec-template-literals-runtime-semantics-evaluation
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@italoacasas

This comment has been minimized.

Copy link
Member

commented Mar 13, 2017

@TimothyGu this is not landing clearly in v7.x, can backport?

@TimothyGu

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2017

@italoacasas, I'll open a separate PR for many recent URL changes.

@italoacasas

This comment has been minimized.

Copy link
Member

commented Mar 13, 2017

Thank you @TimothyGu

jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017
url: prioritize toString when stringifying
The ES addition operator calls the ToPrimitive() abstract operation
without hint String, leading a subsequent OrdinaryToPrimitive() to call
valueOf() first on an object rather than the desired toString().

Instead, use template literals which directly call ToString() abstract
operation, per Web IDL spec.

PR-URL: nodejs#11737
Fixes: b610a4d "url: enforce valid UTF-8 in WHATWG parser"
Refs: nodejs@b610a4d#commitcomment-21200056
Refs: https://tc39.github.io/ecma262/#sec-addition-operator-plus-runtime-semantics-evaluation
Refs: https://tc39.github.io/ecma262/#sec-template-literals-runtime-semantics-evaluation
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell referenced this pull request Apr 4, 2017
@TimothyGu TimothyGu referenced this pull request Apr 19, 2017
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
8 participants
You can’t perform that action at this time.