url: use symbol properties correctly in URL class

[TimothyGu]

This PR has two commits, both concerning the usage of symbol properties.

The first is to avoid using a getter for @@toStringTag. It does mean that parts of #10562 are reverted. This is done for multiple reasons:

1. Web IDL says "the object must, at the time it is created, have a property whose name is the @@toStringTag symbol and whose value is the specified string." This implies that the object itself must possess the property, not its [[Prototype]].

2. From the same quote above, the spec says the property must have a value, (not a getter). Elsewhere in the spec, the distinction between getter, setter, and value of a property is always well-defined, and there is no reason to assume otherwise for this instance.

3. Ambiguity of behavior when something like this happens:

   Object.getOwnPropertyDescriptor(URL.prototype, Symbol.toStringTag).get();

   Web IDL is usually fairly strict with this values (see operations/methods, getters and setters). In fact, all of the following will result in an error in the browser:

   Object.getOwnPropertyDescriptor(URL.prototype, 'href').get();
   Object.getOwnPropertyDescriptor(URL.prototype, 'href').get.call(null);
   Object.getOwnPropertyDescriptor(URL.prototype, 'href').get.call(window);
   Object.getOwnPropertyDescriptor(URL.prototype, 'href').get.call({});

   However, because of the second concern above, no getter for @@toStringTag is defined in the spec, and therefore no algorithms exist for dealing with this situation: whether the getter should return 'URL', 'URLPrototype', '', undefined, or throw an error cannot be reasonably decided. Currently, even Object.getOwnPropertyDescriptor(URL.prototype, Symbol.toStringTag).get.call(null) returns 'URLPrototype', which makes little sense.

This change does also need to be applied to URLSearchParams class. I will do so in a subsequent PR.

The second is arguably less controversial. The issue is that an undocumented but public inspect() exists on URL.prototype, even though it is not part of the URL Standard. It was first raised by @watilde in #10857 (comment). This PR fixes this issue by renaming the property to an internal symbol reserved for inspect method, which is already how URLSearchParams' inspection operates. Tests are adjusted accordingly.

/cc @nodejs/url, @watilde

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-whatwg

[domenic]

Regarding toStringTag, this is actually a controversial part of the Web IDL spec. See https://www.w3.org/Bugs/Public/show_bug.cgi?id=28244. I would suggest following Chrome's version, of just putting it as a data property on the prototype, but I am somewhat biased there :).

+1 to fixing the inspect thing.

[joyeecheung]

FWIW, here are where the browsers stand at the moment:

Chrome Canary 57.0.2986.0:

> Object.getOwnPropertyDescriptor(URL.prototype, Symbol.toStringTag)
Object {value: "URL", writable: false, enumerable: false, configurable: true}
> Object.getOwnPropertyDescriptor(new URL('http://foo.bar.com'), Symbol.toStringTag)
undefined
> Object.prototype.toString.call(URL.prototype)
"[object URL]"
> Object.prototype.toString.call(new URL('http://foo.bar.com'))
"[object URL]"

Firefox Nightly 53.0a1

> Object.getOwnPropertyDescriptor(URL.prototype, Symbol.toStringTag)
undefined
> Object.getOwnPropertyDescriptor(new URL('http://foo.bar.com'), Symbol.toStringTag)
undefined
> Object.prototype.toString.call(URL.prototype)
[object URLPrototype]
> Object.prototype.toString.call(new URL('http://foo.bar.com'))
[object URL]

Safari 10.0.2

> Object.getOwnPropertyDescriptor(URL.prototype, Symbol.toStringTag)
undefined
> Object.getOwnPropertyDescriptor(new URL('http://foo.bar.com'), Symbol.toStringTag)
undefined
> Object.prototype.toString.call(URL.prototype)
[object URLPrototype]
> Object.prototype.toString.call(new URL('http://foo.bar.com'))
[object URL]

[jasnell]

LGTM. I'm trying to recall if there is a test that verifies the toStringTag. If not, it would be worthwhile to add one here. Good to go either way tho after CI

[joyeecheung]

There are tests(test/parallel/test-whatwg-url-tostringtag.js) that verify the behavior of toStringTag(though Symbol.toStringTag is not the only way to implement this behavior), but no tests to verify the attributes of the symbol property itself.

[joyeecheung]

I think it's OK to move closer to the spec even when the browsers don't at the moment, but I am a little curious about the performance hit to new URL, as brought up in https://www.w3.org/Bugs/Public/show_bug.cgi?id=28244?

[watilde]

Let me mention the following error I faced on the branch:

$ git remote add test https://github.com/TimothyGu/node.git
$ git fetch test
$ git checkout url-props
$ make
$ out/Release/node --expose-internals /Users/watilde/Development/node/test/parallel/test-whatwg-url-properties.js
assert.js:364
    throw actual;
    ^

TypeError: Cannot set property origin of [object URLPrototype] which has only a getter
    at assert.throws (/Users/watilde/Development/node/test/parallel/test-whatwg-url-properties.js:46:32)
    at _tryBlock (assert.js:323:5)
    at _throws (assert.js:342:12)
    at Function.throws (assert.js:372:3)
    at Object.<anonymous> (/Users/watilde/Development/node/test/parallel/test-whatwg-url-properties.js:46:8)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)

Potentially, the type annotation in the error message was changed to [object URLPrototype] from [object Object].

[TimothyGu]

Indeed, I see a nontrivial performance drop after this PR 😢

As recommended by @domenic, Chrome's behavior (which does not change performance significantly) is now adopted.

CI can't be launched right now due to overcrowding.

[TimothyGu]

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

[jasnell]

Not too happy with losing the URLPrototype. Please hold off on landing this so we can explore the options and the perf hit a bit more.

[TimothyGu]

@jasnell

After thinking about this issue a bit more, I am still convinced Chrome's behavior is currently the best one. It provides reasonable behaviors in all cases. In the only case it deviates from the Web IDL Standard, it follows the behavior a JS programmer would expect without reading DOM specs (Set.prototype[@@toStringTag] === 'Set').

See below for a comparison table. Explanations for certain table headings are provided also.

Approach	toString(URL.prototype)	toString(url)	@@toStringTag.get.call(null)	error message	performance	consistency
getter (current)	ok URLPrototype	ok URL	not ok URLPrototype	unclear	ok	no current implementation
spec (2616fd9)	ok URLPrototype	ok URL	ok	misleading	not ok (10% slower)	spec (no actual implementation)
data property on proto (this PR)	not ok URL	ok URL	ok	ok	ok	Chrome

@@toStringTag.get.call(null)

This was explained in the original PR.

Error message

(() => {
  'use strict';
  url.origin = 'example.com';
})();

getter	Cannot set property origin of [object Object] which has only a getter
spec	Cannot set property origin of [object URLPrototype] which has only a getter
data prop on proto	Cannot set property origin of [object URL] which has only a getter

Performance

The performance hit for using the specification behavior is unfortunately non-trivial.

getter vs. spec

                                                                           improvement confidence      p.value
 url/legacy-vs-whatwg-url-parse.js n=100000 method="whatwg" type="auth"        -9.25 %        *** 1.888128e-08
 url/legacy-vs-whatwg-url-parse.js n=100000 method="whatwg" type="dot"        -15.10 %        *** 3.261167e-25
 url/legacy-vs-whatwg-url-parse.js n=100000 method="whatwg" type="idn"        -13.13 %        *** 1.746267e-16
 url/legacy-vs-whatwg-url-parse.js n=100000 method="whatwg" type="long"        -8.71 %        *** 5.557978e-19
 url/legacy-vs-whatwg-url-parse.js n=100000 method="whatwg" type="percent"    -16.61 %        *** 1.859190e-22
 url/legacy-vs-whatwg-url-parse.js n=100000 method="whatwg" type="short"      -14.46 %        *** 3.478541e-27
 url/legacy-vs-whatwg-url-parse.js n=100000 method="whatwg" type="special"    -15.36 %        *** 1.582933e-27

getter vs. data property on proto

                                                                           improvement confidence    p.value
 url/legacy-vs-whatwg-url-parse.js n=100000 method="whatwg" type="auth"         0.76 %            0.53752737
 url/legacy-vs-whatwg-url-parse.js n=100000 method="whatwg" type="dot"         -0.16 %            0.83467996
 url/legacy-vs-whatwg-url-parse.js n=100000 method="whatwg" type="idn"         -0.67 %            0.46420488
 url/legacy-vs-whatwg-url-parse.js n=100000 method="whatwg" type="long"        -0.78 %            0.28338187
 url/legacy-vs-whatwg-url-parse.js n=100000 method="whatwg" type="percent"      1.53 %            0.05135193
 url/legacy-vs-whatwg-url-parse.js n=100000 method="whatwg" type="short"       -1.90 %          * 0.01902907
 url/legacy-vs-whatwg-url-parse.js n=100000 method="whatwg" type="special"      1.44 %            0.09545987

Consistency

No browsers currently implement the spec literally or getters.

This PR implements Chrome's behavior.

Edge, Firefox, and Safari do not implement @@toStringTag for DOM interfaces at this moment.

[jasnell]

Ok. I'll still likely play around with it a bit more to see what (if anything) we can do here to ensure that we're entirely spec compliant but I'll clear my objection and sign off

Ok to land...

[TimothyGu]

Will apply tomorrow if no objections.

[TimothyGu]

I'll still likely play around with it a bit more to see what (if anything) we can do here to ensure that we're entirely spec compliant

Please do so. I'd be interested in an alternative as well.

[TimothyGu]

Landed in d65904b and 06ecf4d.