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

Only install @@toStringTag on the prototype #357

Merged
merged 1 commit into from
Apr 23, 2020
Merged

Only install @@toStringTag on the prototype #357

merged 1 commit into from
Apr 23, 2020

Conversation

domenic
Copy link
Member

@domenic domenic commented May 1, 2017

This fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=28244, aligning Web IDL objects with the built-in ECMAScript objects (see #226), and updating the spec to match 1/4 browsers (Chrome) instead of 0/4 as it currently does.

(Currently no non-Chrome browsers install Symbol.toStringTag properties on any objects, but instead contradict the ECMAScript specification and use magic to produce [object InterfaceName] / [object InterfaceNamePrototype].)


/cc @bzbarsky, who tried to implement this in https://groups.google.com/forum/#!searchin/mozilla.dev.platform/toStringTag%7Csort:relevance/mozilla.dev.platform/IZNh8QAXkFA/me59gpo5PgAJ but got pushback.

/cc @travisleithead and @samweinig about implementing this in Edge and WebKit.


As with #356, if there is interest in introducing a brand-checking API which cannot be fooled by interface prototype objects, we (Chrome) would like to have that proposed separately. But we think it's more important for interop and for specification health to align the spec and browsers on something around this question, and get away from nonstandard violations of the ECMAScript spec. We don't want to block such convergence on an API addition.


Preview | Diff

@bzbarsky
Copy link
Collaborator

bzbarsky commented May 2, 2017

Why are we aligning with 1/4 implementations, and not 3/4?

I've thought about this a good bit since the thread mentioned above, and in general, I feel like TC39 made a serious mistake when going from ES5 builtings, for which the prototype was in fact an instance of the type (Array, Date) to the ES6 ones, where it's not (Map, Set). They kept the Object.prototype.toString behavior, which made sense for Array and Date but is nonsense for Map and Set.

I can't do much about that at this point, but I don't think we should let this author-hostile mistake infect the entire web platform. Yes, the proposed PR gives a behavior that is really easy to spec and implement, but also I suspect one that no sane author would never suggest. This is the tail wagging the dog.

In an ideal world, ES would have allowed preserving the existing long-standing behavior of browsers. But the ES committee allowed theoretical purity to stand in the way of speccing something useful, so I guess we have to make do with the @@toStringTag mechanism. We can still use that to create behavior that is at least sane in the common case, by using an accessor @@toStringTag property on DOM prototypes.

I should note that the argument that this can or should be decoupled from branding detection is very weak from my point of view. Given the brokenness of instanceof on the web, toString behavior is the go-to brand detection mechanism on the web right now, and we have no replacement. If we had one, and had been evangelizing it for a while, this behavior change would still be somewhat author-hostile, but at least we would be able to point unhappy authors to the other alternative. As things stand, it looks like we're just trying to take away any chance for authors to be able to do brand detection.

I should note that ES itself has these problems, and people keep bringing this up, and TC39's official answer for brand detection is "yeah, find something that does brand checks and is side-effect-free and check whether it throws". Which is quite unintuitive, author-hostile, and not even guaranteed to work on all IDL types (e.g. there might not be anything side-effect-free). I have yet to meet anyone who thinks that brand-detection this way is a good idea, outside TC39...

@domenic
Copy link
Member Author

domenic commented May 2, 2017

Why are we aligning with 1/4 implementations, and not 3/4?

It's not possible to align with 3/4 implementations without changing the ECMAScript spec.

I suspect one that no sane author would never suggest.

I disagree with that, as a sane author. I like having toString behavior inherit prototypically like every other behavior.

In general I find the tone of your post a little insulting, both to me as a web developer, and to me as a TC39 member, and to me as someone who works on a browser who finds this version more reasonable. I'd appreciate a little more civility.

I also don't think authors are nearly as concerned as you are with whether this one edge-case returns XPrototype or X; see below.

As things stand, it looks like we're just trying to take away any chance for authors to be able to do brand detection.

Object.prototype.toString is already not a brand detection mechanism.

const x = {
  [Symbol.toStringTag]: "Node"
};

Object.defineProperty(document, Symbol.toStringTag, {
  value: "NotDocument"
});

Now, x will "brand detect" as a Node, and document will "brand detect" as a NotDocument.

Given that Object.prototype.toString is already not a reliable brand, I don't think allowing one more class of object through such checks is a problem. Especially since such objects (interface prototype objects) are so very rarely passed around to functions that might try to branch on brand-detection.

I should note that ES itself has these problems, and people keep bringing this up, and TC39's official answer for brand detection is

The issue here is that we haven't seen sufficient author demand for unforgeable brand detection. (As opposed to simply usually-working brand detection, such as that provided by x.constructor.name or Object.prototype.toString.call().) You clearly think it's important, presumably for the web applications you are writing.

But a proposal for such facility needs to go through the normal standards process for a dedicated feature, where we present use cases and discuss APIs and get multiple-implementer commitment to implement. I don't think it's appropriate to repurpose JavaScript language hooks (hasInstance or toStringTag) to start working differently than they normally do, as a way of getting this API you want shipped. Especially since nobody has shipped it, in the case of toStringTag.

I admit Chrome was not a good citizen at the time you added these, in that we didn't raise our objections in a timely manner. But I hope you can forgive that and collaborate with us now.

@bzbarsky
Copy link
Collaborator

bzbarsky commented May 2, 2017

It's not possible to align with 3/4 implementations without changing the ECMAScript spec.

In edge cases, sure, but in the common case, it's possible.

I'd appreciate a little more civility

My apologies; I will try. None of this is meant to be personal, obviously!

Object.prototype.toString is already not a brand detection mechanism.

Not in the face of hostile code, sure (but neither is anything else in the platform, including Array.isArray, which could have been overridden). But for things like catching inadvertent mistakes, or just checking what sort of objects you're dealing with when debugging, it's a lot better than nothing.

Especially since such objects (interface prototype objects) are so very rarely passed around to functions that might try to branch on brand-detection

Just to check, is the "very rarely" based on gut feeling or data? [LenientThis] exists in IDL precisely because of libraries passing some prototype objects around to places that then didn't brand-detect... while other places did, so we didn't have to add LenientThis too widely.

The issue here is that we haven't seen sufficient author demand for unforgeable brand detection.

Sure. I don't think anyone is asking for unforgeable brand detection in the "works in the face of hostile code" sense.

As opposed to simply usually-working brand detection

I guess the disagreement is about what "usually" should mean. Again, no one is asking for brand detection in the face of hostile code, and it's very hard to provide anyway. But brand detection that catches simple mistakes in normal code (e.g. passing in a prototype when an instance is expected) is something some developers do want. That's why you end up with various people using Object.prototype.toString.call, or branching on instanceof checks, etc... My experience has been that when the facilities to do such detection exist (e.g. in Firefox extensions or Firefox front-end code), people easily come to rely on them. The times when we've made them go away there was all sorts of unpleasant surprise on the part of the authors of various JS bits.

I've definitely heard a number of people be surprised when they inspect an object in Chrome's developer tools and discover that while it says "Document" it's not actually a Document instance. And yes, I'm aware that developer tools don't have to use any of the stuff we're discussing here. This is merely a data point about what people expect or don't expect...

But a proposal for such facility needs to go through the normal standards process for a dedicated feature, where we present use cases and discuss APIs and get multiple-implementer commitment to implement.

Yes, I agree. We should do this. We've been saying for years we should do this, but it keeps getting shot down for ES types in TC39 because brand detection is seen as an anti-pattern there and in IDL discussions because it's been nearly impossible to get people to comment with anything other than "let's defer this to TC39; this should be defined for all types in the platform". So we keep going around in circles. :(

I don't think it's appropriate to repurpose JavaScript language hooks (hasInstance or toStringTag) to start working differently than they normally do, as a way of getting this API you want shipped.

Just to be clear... the toString behavior under discussion (the one you're proposing the non-Chrome engines remove) is at least 16 years old. I don't know how much older, because I wasn't involved in browser development before then, and I don't have any Netscape 3/4 or IE 3/4/5 instances around to test them. If I had to take a guess, this has been the behavior ever since there was a DOM at all. The instanceof behavior in Firefox is of similar vintage, I'm pretty sure.

So we're not talking about non-Chrome browsers hackily changing the behavior of toString to fulfill this branding-detection use case all of a sudden. We're talking about a very longstanding behavior of pretty much every browser (and until at least 2008, every browser) that you think should be changed. I respect and understand your arguments for changing it, but I think we should have a migration path for people who rely on the existing behavior.

I admit Chrome was not a good citizen at the time you added these

I didn't add them, per above. They've been around "forever" in web terms, as browser behaviors. I agree that in spec terms the vintage is more recent.

I am extremely leery of changing behavior that is almost certainly older than some of the people now working on web browsers, that I know based on past bug reports people are currently depending on or trying to depend on, without providing them with a migration path, at least.

@domenic
Copy link
Member Author

domenic commented May 2, 2017

OK, this is turning into a good discussion :). I think there are two main topics:


(1) In aligning Web IDL with modern ECMAScript, do we define [Symbol.toStringTag] as (a) on the prototype only, with value "X"; (b) as a getter on the prototype, which branches and returns either "X" or "XPrototype"; (c) as a data property on both the prototype ("XPrototype") and every instance ("X").

Note that if we choose anything but (a), we have zero browsers complying with the spec, whereas (a) at least has one browser.

Chrome believes the answer should be (a), for the following reasons:

  • It is more consistent with ECMAScript built-ins
  • It is simpler to make performant (unlike (b)) and memory efficient (unlike (c))
  • We have been shipping it for over a year with no reported compatibility issues

Just to check, is the "very rarely" based on gut feeling or data?

This is based on a gut feeling. Would it help if we provided data? Perhaps we could use-counter how often interface prototype objects are passed to Object.prototype.toString.call(). If we do this, we should pre-commit to a threshold below which you think it'd be OK to change to behavior (a).

I am extremely leery of changing behavior that is almost certainly older than some of the people now working on web browsers, that I know based on past bug reports people are currently depending on or trying to depend on, without providing them with a migration path, at least.

Our feeling is that the migration path should not be necessary as a prerequisite for changing the behavior, since we have not seen author demand for a brand-checking facility that excludes interface prototype objects. Specifically, we have seen no reported issues where people were frustrated by Chrome's behavior change. Similarly, we were hoping to show by shipping that there's no need to be so leery of the change.


(2) The topic of adding a general brand-detection API

Although our starting position is skepticism about the value of such an API---as you say, we disagree on the value of "usually"---we're definitely willing to discuss it. We should have this discussion.

It's pretty clear that waiting on TC39 is not the right approach. Whatever we do could either work for platform objects only, or could special-case the ECMAScript built-ins, like StructuredSerialize (née structured clone) does. There's a path here; we can do it.

@bzbarsky
Copy link
Collaborator

bzbarsky commented May 3, 2017

I agree with the (a), (b), (c) choices you lay out being the obvious choices. I also agree that (c) is not a great solution for all sorts of performance/memory reasons.

I strongly suspect (gut feeling, no data) that there is no performance-critical Object.prototype.toString usage on IDL objects.

Having data on how often people end up doing Object.prototype.toString with an IDL interface prototype as this may be helpful, yes. I agree that we should pre-commit to a threshold if we decided to use that data as the deciding factor. I don't think I'm in a position to unilaterally set this threshold, not least because I obviously can't speak for Apple and Microsoft, and am not sure I can speak for Mozilla on my own in this case. @smaug---- do you have any thoughts?

I would also be very interested in data on what web developers expect/want the behavior to be, but I'm not sure how to gather such data...

@ajklein
Copy link

ajklein commented May 3, 2017

@bzbarsky Regarding "performance-critical Object.prototype.toString usage", V8 has found it to be a performance bottleneck for some code, for example Angular calls it in a tight loop. This makes me think it'd be hard to gather data on (we currently don't have a way to efficiently collect usage data in optimized code). And also makes me disinclined to assume that no-one is depending on its performance on IDL objects.

The Angular case is definitely a point in favor of a branding API of some sort, but toString seems to be working for them for now, as long as it remains fast.

@bzbarsky
Copy link
Collaborator

bzbarsky commented May 3, 2017

Note that if we choose anything but (a), we have zero browsers complying with the spec, whereas (a) at least has one browser.

One other note: this is true in a literal sense (i.e. in terms of all observable details) but not in the common "what does Object.prototype.toString return?" observable.

@smaug----
Copy link

I would also be very interested in data on what web developers expect/want the behavior to be, but I'm not sure how to gather such data...

This would be indeed interesting, since my gut feeling is that no one would choose the behavior ES wants here, since it is very surprising.

I wonder if ES behavior should be changed. (I know, probably not very realistic to ask for)

@domenic
Copy link
Member Author

domenic commented May 3, 2017

What we can offer so far is that no web developers have complained about the ES behavior or about the behavior for all platform objects shipping in Chrome for over a year.

This indicates strongly to me that web developers don't care about interface object prototypes (or ES built-in prototypes) with respect to Object.prototype.toString.call().

I think that for those that do care (mostly people rather engaged in web standards and minutiae), they'll expect it to match ES built-ins. But that's a gut feeling, not backed by "data" like the above.

@travisleithead
Copy link
Member

I'm supportive of option a or b. Option b is interesting and implies that there is still some magic internal branding that the getter native code has access to that is not exposed. Whether making that distinction (between instance and prototype of instance) is important for web developers, I don't know. My gut feeling is that any change to prototype object's toString value is much less impactful than changes to the instance (where lots of old code checks against a literal string "[object X]"). Impact-wise, I expect this to be similar to the change of constructors (interface objects) to be functions instead of objects (which changed their toString value dramatically; yet saw virtually no web-compat impact).

We really need a reliable brand-detection API. It's pretty obvious, even with Chrome's changes, that there is something fundamentally immutable about a DOM instance that is needed for the DOM to operate correctly (at least while the DOM is not fully-implemented in script--a fact of life for the near future anyway). For example, consider this:

document.__proto__ = Text.prototype; // Is document now text?
document.toString(); // Chrome reports it is (using @@toStringTag from Text.prototype)
Object.prototype.toString.call(document); // Also reported as "[object Text]", so... rebranded?
// Yet the DOM itself can't be made to see the brand-change...
Document.prototype.getElementById.call(document, "no-matching-id"); // null (rather than exception)
Document.prototyep.getElementById.call(new Text("real Text instance"), "no-matching-id");
// That last call throws due to a brand-check done implicitly by the DOM; 
// it is not fooled by the previous document-looking-like-Text

So, let's make some progress (but not in this issue as you noted initially) against the API for reading out the reliable DOM branding of an instance/prototype. If we take it away via toString, let's put it back for the use cases that would really like it via something else. E.g.,

// would not lie, even with the previous example's manipulations (and would work cross-realm?)
Document.isInstance(document); 
// or
Object.getOwnNativeBrand(document); // returns "[object Document]"
Object.getOwnNativeBrand(Document.prototype); // returns "[object DocumentPrototype]"
@annevk
Copy link
Member

annevk commented Dec 7, 2017

@travisleithead there's an open issue on adding [[PlatformBrand]] internal slots. Thus far nobody has managed to convince TC39 that exposing such branding for built-ins through a nice API is a good idea. I don't think we should do it for platform objects if we don't have it for the standard library.

@travisleithead
Copy link
Member

I can understand that sentiment, since there's very little in native ECMAScript that needs to perform brand-checks; however, that's not the case in the DOM, and I think that's the overriding reason to push for something in WebIDL despite not having it in the standard library.

But we digress. What is the open issue you mentioned? I should probably focus my energy there...

@annevk
Copy link
Member

annevk commented Dec 8, 2017

Adding [[PlatformBrand]] is #97. I suggest raising a new issue for exposing it though, as that's a somewhat different discussion. (That is, it's clear we need a private API, which is what that issue is about. Exposing it publicly is controversial and doesn't have an issue.)

@domenic
Copy link
Member Author

domenic commented Dec 8, 2017

@travisleithead, are you on board with aligning with option (a)? That'd give us two implementers in support, which could perhaps allow us to move this spec patch forward...

@travisleithead
Copy link
Member

Of the options, a) seems the best to me. I'm curious though if we'd plan to spec the fallback behavior that Chrome has when there's no way to get the @@toStringTag?

Tested this:

// @toStringTag is on prototypes, so disconnect the instance from the prototype...
document.__proto__ = null;
// confirm that there's no @toStringTag (or other symbol) on the instance...
Object.getOwnPropertySymbols(document).length == 0
// true
// Ask for the toString of this instance... (grabs its native brand)?
Object.prototype.toString.call(document)
// "[object HTMLDocument]"
@bzbarsky
Copy link
Collaborator

bzbarsky commented Dec 8, 2017

As far as that goes, what does Object.prototype.toString.call do in Chrome on the proto if you delete the @@toStringTag prop from the proto?

@domenic
Copy link
Member Author

domenic commented Dec 8, 2017

Wow, interesting find. http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5623 confirms @travisleithead's findings.

http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5624 shows that in the case @bzbarsky asks about it falls back to document[Symbol.toStringTag] === Document.prototype[Symbol.toStringTag] (remember that document is a HTMLDocument in reality... yeah, we should probably fix that spec/reality mismatch).

http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5625 shows that if you go all the way down it then reverts to [object HTMLDocument], like in the null-proto case.

So there's some fallback logic in Chrome that is not captured by this spec patch, and is IMO not good and should be removed. I will file a Chrome bug. Edit: filed https://bugs.chromium.org/p/chromium/issues/detail?id=793406

@bzbarsky
Copy link
Collaborator

bzbarsky commented Dec 8, 2017

I was actually also interested in http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5627 which shows that the proto itself ends up with "[object Object]" (but the document does end up with "[object HTMLDocument]").

@littledan
Copy link
Collaborator

Another year has gone by, and Chrome continues to ship semantics in this PR, whereas Firefox continues to ship semantics in the editors' draft. I think it'd be good to resolve this issue one way or another. How should we decide from here?

The semantics in this PR make more sense to me (to the extent that @@toStringTag makes sense at all), and apparently it's web-compatible, so I wouldn't mind seeing the patch merge.

@bzbarsky
Copy link
Collaborator

Chrome is not shipping quite the semantics in this PR, per earlier discussion. What it's shipping seems to not be documented anywhere. The web compat data (such as we have; given the amount of browser-sniffing out there, having a single implementation do something is somewhat weak data, sorry) seems to be for what's shipping, not what's in the PR.

My position for Firefox is that no matter what we do not want to change behavior here more than once. We would want to see the actual proposal showing web compat as much as possible before we can consider it.

to the extent that @@toStringTag makes sense at all

Well, that is the other question, yes. Do we think @@toStringTag is a mechanism worth perpetuating? Especially if we end up needing some other fallback anyway.

@Ms2ger
Copy link
Member

Ms2ger commented Dec 18, 2018

This will fix #54.

@annevk annevk added interop Implementations are not interoperable with each other do not merge yet Pull request must not be merged per rationale in comment labels Jan 28, 2019
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 20, 2020
…pe instance test, a=testonly

Automatic update from web-platform-tests
WebIDL: Fix class string of null-prototype instance test

Follow-up of #23140.

Spec: whatwg/webidl#357.
--

wpt-commits: fca25d802d75842bd3ea9ed1c16ac341b18e4455
wpt-pr: 23371
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request May 20, 2020
…pe instance test, a=testonly

Automatic update from web-platform-tests
WebIDL: Fix class string of null-prototype instance test

Follow-up of #23140.

Spec: whatwg/webidl#357.
--

wpt-commits: fca25d802d75842bd3ea9ed1c16ac341b18e4455
wpt-pr: 23371
domenic pushed a commit that referenced this pull request Jul 23, 2020
A followup to #357.

This change aligns WebIDL namespace objects with ECMA-262 ones, per tc39/ecma262#2057 (comment).

Tests: web-platform-tests/wpt#24717
@github-actions github-actions bot mentioned this pull request Jan 21, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Implementations are not interoperable with each other