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

Various named property stuff seems confused in the face of symbols #175

Closed
bzbarsky opened this issue Sep 9, 2016 · 10 comments
Closed

Various named property stuff seems confused in the face of symbols #175

bzbarsky opened this issue Sep 9, 2016 · 10 comments

Comments

@bzbarsky
Copy link
Collaborator

bzbarsky commented Sep 9, 2016

Consider this simple testcase:

var x = document.documentElement.dataset;
var s = Symbol("foo");
Object.defineProperty(x, s, { value: "hey" });

What should happen? We land in http://heycam.github.io/webidl/#defineownproperty and the interface supports named properties. s is not a supported property name, so creating ends up true. There is a named property setter, so we're supposed to invoke the named property setter... but P is a symbol and named property setters expect strings.

Seems to me like all the named property stuff should be skipped for symbol names or something.

@domenic
Copy link
Member

domenic commented Sep 9, 2016

Seems to me like all the named property stuff should be skipped for symbol names or something.

Agreed, FWIW. Just never reach into that machinery when a symbol comes along.

@TimothyGu
Copy link
Member

Am I correct in understanding that only a simple fastpath to OrdinaryDefineOwnProperty for symbols w/o any additional processing is needed?

@domenic
Copy link
Member

domenic commented Aug 12, 2017

Yes, I believe that's the right thing to do. I wouldn't quite call it a fastpath, since the current state probably leads to spec bugs where the named property setter algorithm in various specs does bad things when given a symbol. (E.g., perhaps adding an attribute to the DOM element whose name is given by a symbol.)

In practice this probably means adding "P is not a symbol" to the first two bullets. We do also want to flip the configurability for symbols, so step 3 still needs to happen.

@bzbarsky
Copy link
Collaborator Author

@tobie, @TimothyGu are there web platform tests that got added for this? Issues filed on browsers?

@tobie
Copy link
Collaborator

tobie commented Aug 29, 2017

@bzbarsky filed web-platform-tests/wpt#6912 not to forget about it.

Do we want to file issues against browsers that fail the tests once those are written, or should we assume failing tests in wpt will be handled somehow?

@bzbarsky
Copy link
Collaborator Author

Do we want to file issues against browsers that fail the tests once those are written

That would be ideal, yes! There are literally tens of thousands of failing WPTs in browsers (I just checked in Firefox and it's over 50k) and many newly added tests fail due to browser or test bugs, so depending on people noticing a new failing test is pretty unreliable, unfortunately. :( I wish we were closer to that being something that sets off alarms...

@bzbarsky
Copy link
Collaborator Author

@tobie Do you know what the status is of those tests and browser bugs? It's not great having this situation where the spec says X but we have no tests and browsers have no idea the spec says X...

@tobie
Copy link
Collaborator

tobie commented Nov 13, 2017

I agree. I need to look into it.

@bzbarsky
Copy link
Collaborator Author

Amusingly enough, html/webappapis/dynamic-markup-insertion/opening-the-input-stream/no-new-global.window.js is testing this behavior to some extent: it uses symbol-named properties on sessionStorage and localStorage...

@bzbarsky
Copy link
Collaborator Author

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1520831 on this for Gecko.

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

No branches or pull requests

4 participants