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

Define [[OwnPropertyKeys]] of legacy platform objects #402

Merged
merged 2 commits into from Aug 12, 2017

Conversation

TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Aug 7, 2017

Fixes #400.


Preview | Diff

Copy link
Collaborator

@tobie tobie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for making this. Overall this looks really solid! Couple of comments and questions below (this is a part of the spec I'm not super familiar with, so please bear with me if some of those are silly).

property creation, [=list|append=] |P| to |keys|.
1. [=list|For each=] |P| of |O|’s own property keys that is a Symbol, in ascending chronological order of
property creation, [=list|append=] |P| to |keys|.
1. Assert: |keys| has no duplicate items.
Copy link
Collaborator

@tobie tobie Aug 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not used an ordered list instead (as you initially suggested)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Assuming you meant ordered set...) ES uses a plain List, and it would actually be a specification error if there are duplicates, since the named property visibility algorithm should filter out any supported property names that are also own properties on the object (or in its prototype chain).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Assuming you meant ordered set...)

I did. :)

ES uses a plain List, and it would actually be a specification error if there are duplicates, since the named property visibility algorithm should filter out any supported property names that are also own properties on the object (or in its prototype chain).

That makes sense.

the object’s [=supported property indices=] are
enumerated first, in numerical order.
1. If the object [=support named properties|supports named properties=] and doesn't implement an [=interface=] with the
[{{LegacyUnenumerableNamedProperties}}]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The effects of the [{{LegacyUnenumerableNamedProperties}}] extended attribute seem totally gone from the updated algorithm. Is that on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, LegacyUnenumerableNamedProperties controls the enumerable bit in the descriptor but not the existence of the key. See [[GetOwnProperty]]. The original pseudo-algorithm defines the "enumeration order", basically the order of for-in loops, which accounts for both enumerability and key existence.

|keys|.
1. If |O| [=support named properties|supports named properties=], then [=list|for each=] |P| of |O|’s
[=supported property names=] that is visible according to the [=named property visibility algorithm=],
[=list|append=] |P| to |keys|.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these also in ascending chronological order?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order is defined by the specification for the specific interface.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. That's the order in which they're spec'ed. In one of the toJSON-related algorithms we write this as:

If a toJSON operation with a [Default] extended attribute is declared on |I|,
then for each exposed regular attribute |attr| that is an interface member of |I|,
in order:

Maybe just add in order, before [=list|append=] |P| to |keys|?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"for each" from Infra already defines the order so you don't have to add that kind of thing. (At least, iirc that's the idea.)

Copy link
Collaborator

@tobie tobie Aug 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@annevk, you're right. I'd just like to be a little more explicit in both those cases that enumeration order follows the order in which the attributes are specified in the interface. But we can leave that out for now and fix them up both together later if necessary.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally mixed up a bunch of things here. Apologies for muddling the conversation. Please ignore this comment altogether.

index.bs Outdated
@@ -12014,19 +12014,26 @@ However, for [=legacy platform objects=],
properties on the object must be
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to add a small blurb here that this is done by the [[OwnPropertyKeys]] method, e.g:

This document does not define a complete property enumeration order
for [=platform objects=] implementing [=interfaces=]
(or for <a href="#es-exception-objects">platform objects representing exceptions</a>).
However, it does for [=legacy platform objects=] by defining the \[[OwnPropertyKeys]] method as follows:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I seem to have forgotten to update that blurb… will do.

@domenic
Copy link
Member

domenic commented Aug 7, 2017

First of all, this PR is awesome.

Second, I am concerned about the extent to which it matches implementations, especially with regard to ordering. In particular, it's possible implementations don't have a good order except in enumeration cases, if they don't map onto the recent ES spec updates which derive enumeration from own property keys.

So, we'll need some good tests of the various permutations here to be sure.

@tobie
Copy link
Collaborator

tobie commented Aug 7, 2017

So, we'll need some good tests of the various permutations here to be sure.

@domenic how do you want to go about this? Land this, then add the tests, then given the test results, file issues with rendering engines? Or do you want to wait until we have test results before we land this?

@TimothyGu
Copy link
Member Author

I am concerned about the extent to which it matches implementations, especially with regard to ordering

While I did write this PR to match what implementations are doing, I would agree that there are a lot of corner cases, like index prop names that are not array index prop names ([2^32, 2^53)), assigning array index prop names to objects that don't support indexed properties (and vice versa), and symbols. To that end I'd definitely agree that there should be tests.

if they don't map onto the recent ES spec updates which derive enumeration from own property keys

Can you point out the update you are referring to?

So, we'll need some good tests of the various permutations here to be sure.

Is there a specific section in WPT for Web IDL compliance or should I look into adding these tests to the section of the tested interface (e.g. HTMLCollection)?

@domenic
Copy link
Member

domenic commented Aug 8, 2017

While I did write this PR to match what implementations are doing

Ah OK, that's great. I thought it might have been just a translation of the previous enumeration section, in which case there was more cause for concern. But if you have validated this roughly matches browsers, then I would be comfortable merging before in-depth tests. @tobie

Can you point out the update you are referring to?

tc39/ecma262#367

Is there a specific section in WPT for Web IDL compliance or should I look into adding these tests to the section of the tested interface (e.g. HTMLCollection)?

https://github.com/w3c/web-platform-tests/tree/master/WebIDL, almost always under ecmascript-binding. As you can see from some tests there, we generally pick representative objects. https://github.com/w3c/web-platform-tests/blob/255d54144a82ce76d8e50a4aa8de284151119f8b/WebIDL/ecmascript-binding/legacy-platform-object.html is probably the file to work on, although I think nobody would mind having a separate file for the OwnPropertyKeys sub-cases as that file is so generically named it could grow without bound.

@TimothyGu
Copy link
Member Author

TimothyGu commented Aug 8, 2017

I've completed a more comprehensive audit of what browsers currently do. And the results are, well, inconsistently inconsistent. The test is live at https://timothygu.me/webidl-keys-test/, and the files available in https://github.com/TimothyGu/webidl-keys-test.

I specifically looked at three interfaces: NamedNodeMap (used for element.attributes), NodeList (through node.childNodes), and Storage (through window.sessionStorage). For browsers, I checked Chrome 60 (stable) and 61 (beta), Firefox 55 beta, and Edge 15. I couldn't get used to Safari's developer console, but at a glance its behavior is fairly similar to Chrome's.


From results from NamedNodeMap and NodeList:

Chrome and Edge both exhibit the original issue I opened #400 for, namely they do not include named properties on an interface annotated with [LegacyUnenumerableNamedProperties] in Reflect.ownKeys(), while Firefox does. This PR follows the Firefox behavior, as IMO [LegacyUnenumerableNamedProperties] should only set the enumerable bit rather than hide it from getting all property names. I'm open for some discussion around this however, since it's 1 vs. 2.

While Chrome and Firefox (and this proposal) consistently put own property symbols at the end for legacy platform objects in the same way as ES operates, Edge puts symbols in the front of all own properties.

Not directly related to this proposal, Chrome 60 and Edge follow the current [[Set]] semantics in that for interfaces not supporting indexed properties, it still sets any indexed properties as own properties. However, Chrome 61 and Firefox will throw when trying to do so.

Also not directly related to this proposal, Edge has a bug where its Reflect.ownKeys() returns duplicated property names when an object has both an own property and a supported indexed property link. In that situation, when getting the duplicated key it gets the own property instead. Chrome 60 does it correctly, but the enumeration order is a bit off. Chrome 61 and Firefox did not get to this test because it threw an error when trying to set the own property earlier.


As for Storage, all hell broke loose. No pairs of browsers do the same, and no browsers implement the spec for [[Set]]. It does reveal an issue in [[Set]] that we can fix fairly easily, but other than that there is no agreement. It also reveals browsers have subtly different [[Set]] semantics re. forbidden property names (prototype method shadowing) and symbol properties. As such, it's impossible to properly derive a pattern for enumeration from the Storage interface we can reference in [[OwnPropertyKeys]].


From the results, I see an even stronger need for standardization around [[OwnPropertyKeys]]. Other than [LegacyUnenumerableNamedProperties] behavior, I don't see anything that should be changed in this PR.

@bzbarsky
Copy link
Collaborator

bzbarsky commented Aug 8, 2017

The comments in the Storage test don't quite make sense to me. [[Set]] there should land in https://heycam.github.io/webidl/#legacy-platform-object-set which will call the named setter regardless of what the named property algorithm says (this is purposeful, fwiw). It will not add a normal own property. What it will do is add a thing named "getItem" to the storage. But getting the list of own properties will check the visibility algorithm and nor claim a "getItem" property.

Hence typeof sessionStorage.getItem should be "function", Reflect.ownKeys(sessionStorage) should not include "getItem", etc.

The bit about whether an index should be treated as a name for Storage seems to be #366 which has stalled for some reason. :(

I agree compat around that stuff is poor.

@TimothyGu
Copy link
Member Author

@bzbarsky Thanks! I've updated the comments in the file, and it does make things clearer. To summarize, for Storage:

  • Firefox and Edge's setters incorrectly ignore symbols
  • Chrome's getter incorrectly gets the shadowed prototype property even when it should not be visible
  • Edge's getter correctly handles shadowed properties, but still makes them visible through Reflect.ownKeys().
  • Chrome enumerates symbols in chronological order instead of last.
  • Chrome and Firefox put integer index properties first, even when it is really a named property and should be enumerated chronologically. Within integer index properties, Chrome then sorts them numerically. Firefox sorts them chronologically.

Do you have any comments regarding this PR itself though?

@tobie
Copy link
Collaborator

tobie commented Aug 8, 2017

So we avoid doing duplicate work, heads-up that I've started refactoring legacy-platform-object.html and turning @TimothyGu's tests into wpt in a dedicated branch.

@bzbarsky
Copy link
Collaborator

Firefox and Edge's setters incorrectly ignore symbols

The behavior for symbols is actually not defined right now; the spec's text for them makes no sense and isn't implementable. In particular, [[Set]] in the spec ends up calling OrdinarySet for symbols, which eventually lands in the webidl-defined [[DefineOwnProperty]], which is broken in the face of symbols; see #175. Once that issue is resolved, I'll update Firefox to whatever that resolution is.

Chrome and Firefox put integer index properties first even when it is really a named property and should be enumerated chronologically

If we're still talking about Storage, then in the case of Firefox the order is rather random. In particular, it's just iterating the keys of a non-ordered hashtable, so the precise order will depend on how the keys collided, where they landed in the hashtable, etc. As in, we just don't implement the "chronological order" bit from the spec at all.

As far as the test goes, it might also be worth checking that while sessionStorage.getItem keeps returning the function, sessionStorage.getItem("getItem") returns 42.

Do you have any comments regarding this PR itself though?

Yes, I do. It's totally awesome. ;) I was worried about duplication in the list, but the fix we did for named props not shadowing existing own props even on [OverridBuiltins] interfaces makes everything work out great.

Thank you for doing this!

@bzbarsky bzbarsky merged commit 14792cb into whatwg:master Aug 12, 2017
@TimothyGu
Copy link
Member Author

In particular, it's just iterating the keys of a non-ordered hashtable

Ah, thanks for clarifying then.

Glad I could help! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants