| Summary: | WebIDL attributes should be implemented as getters and setters on the prototype object. | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Erik Arvidsson <arv> |
| Component: | DOM | Assignee: | Zan Dobersek <zan> |
| Status: | RESOLVED FIXED | ||
| Severity: | Major | CC: | aestes, ap, cdumez, dglazkov, dino, dominicc, erights, haraken, heycam, ian.eng.webkit, joepeck, marfalkov, mevrael, mike, mounir, oliver, p.jacquemart, rbyers, sam, shadow2531, silverma, simon.fraser, slightlyoff, t.brain, treshugart, webkit, zcorpan, zimbabao |
| Priority: | P1 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | All | ||
| OS: | All | ||
| Bug Depends on: | 66449, 75297 | ||
| Bug Blocks: | 75575, 43224, 91148, 111476 | ||
Also see https://bugzilla.mozilla.org/show_bug.cgi?id=608651 and http://code.google.com/p/chromium/issues/detail?id=43394 Do we have an idea about how to do this without slowing down DOM property access in the common case? (In reply to comment #2) > Do we have an idea about how to do this without slowing down DOM property access in the common case? This is probably better answered by someone who knows JSC better but isn't this just the same as accessing normal JavaScript objects that have getters on their [[Prototype]]? How do you optimize getters/setters today? How do you optimize [[Get]] when present on a [[Prototype]]? (In reply to comment #3) > (In reply to comment #2) > > Do we have an idea about how to do this without slowing down DOM property access in the common case? > > This is probably better answered by someone who knows JSC better but isn't this just the same as accessing normal JavaScript objects that have getters on their [[Prototype]]? > > How do you optimize getters/setters today? > > How do you optimize [[Get]] when present on a [[Prototype]]? My question is not if/how to optimize getters on a prototype, but rather, if we can make this change without a regression in performance (through some witchcraft). The answer is probably no. (In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > Do we have an idea about how to do this without slowing down DOM property access in the common case? > > > > This is probably better answered by someone who knows JSC better but isn't this just the same as accessing normal JavaScript objects that have getters on their [[Prototype]]? > > > > How do you optimize getters/setters today? > > > > How do you optimize [[Get]] when present on a [[Prototype]]? > > My question is not if/how to optimize getters on a prototype, but rather, if we can make this change without a regression in performance (through some witchcraft). The answer is probably no. I'm unsure how much impact this will have on setters, as i'm 90% sure that setters are completely uncached currently. Getters will be hit by additional branches to validate the proto chain which is unfortunate. The big change will be the property accessors in the dom will need to do type checks and that's an additional virtual call. Could be fixed if class info was hung off of structure but then Structure becomes much larger. :-/ Additionally JSC API probably won't be able to do this for custom getters, etc I believe classes with index property accessors are going to magically become proxies, but i don't believe the semantics for that is sufficiently described yet. I intend to investigate the actual performance impact of this change in JSC. I think that there is a limitation in the V8 API to even be able to specify properties with getters and setters, so I have filed <http://code.google.com/p/v8/issues/detail?id=1482> for that. (In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (In reply to comment #2) > > > > Do we have an idea about how to do this without slowing down DOM property access in the common case? > > > > > > This is probably better answered by someone who knows JSC better but isn't this just the same as accessing normal JavaScript objects that have getters on their [[Prototype]]? > > > > > > How do you optimize getters/setters today? > > > > > > How do you optimize [[Get]] when present on a [[Prototype]]? > > > > My question is not if/how to optimize getters on a prototype, but rather, if we can make this change without a regression in performance (through some witchcraft). The answer is probably no. > > I'm unsure how much impact this will have on setters, as i'm 90% sure that setters are completely uncached currently. Correction: I know that setters aren't cached. Assigning to dom properties would be hammered, but they are theoretically already slow. If we added caching to setters we would probably end up with a net win. > > Getters will be hit by additional branches to validate the proto chain which is unfortunate. This will suck, but given that they're getter/setter pairs that we provide we might be able to remove at least _a_ branch from the current cached path. We should also be able to avoid constructing an actual function wrapper until we absolutely have to (getOwnPropertyDescriptor requires us to provide the function object) > > The big change will be the property accessors in the dom will need to do type checks and that's an additional virtual call. Could be fixed if class info was hung off of structure but then Structure becomes much larger. :-/ > A virtual call is no longer necessary but there's still going to be an additional (unavoidable) cost here, but also we'll nee to go through _every_ custom getter or setter and ensure correct behaviour. > Additionally JSC API probably won't be able to do this for custom getters, etc This has been dealt with already by geoff -- API objects kill optimisation :( > > I believe classes with index property accessors are going to magically become proxies, but i don't believe the semantics for that is sufficiently described yet. This is really an issue that needs to be resolves -- do index properties move to the prototype or do they stay own properties? (In reply to comment #7) Thanks for the insight. > Correction: I know that setters aren't cached. Assigning to dom properties would be hammered, but they are theoretically already slow. If we added caching to setters we would probably end up with a net win. > > > Getters will be hit by additional branches to validate the proto chain which is unfortunate. > > This will suck, but given that they're getter/setter pairs that we provide we might be able to remove at least _a_ branch from the current cached path. We should also be able to avoid constructing an actual function wrapper until we absolutely have to (getOwnPropertyDescriptor requires us to provide the function object) Could you point me to where this cache is in JSC? > > The big change will be the property accessors in the dom will need to do type checks and that's an additional virtual call. Could be fixed if class info was hung off of structure but then Structure becomes much larger. :-/ > > A virtual call is no longer necessary but there's still going to be an additional (unavoidable) cost here, but also we'll nee to go through _every_ custom getter or setter and ensure correct behaviour. > > > Additionally JSC API probably won't be able to do this for custom getters, etc > This has been dealt with already by geoff -- API objects kill optimisation :( > > > I believe classes with index property accessors are going to magically become proxies, but i don't believe the semantics for that is sufficiently described yet. > > This is really an issue that needs to be resolves -- do index properties move to the prototype or do they stay own properties? According to the Web IDL spec, indexed properties are on the host object, not the prototype <http://www.w3.org/TR/WebIDL/#indexed-properties> (In reply to comment #8) > According to the Web IDL spec, indexed properties are on the host object, not the prototype <http://www.w3.org/TR/WebIDL/#indexed-properties> Please look at the copy in dev.w3.org, which has changed somewhat since the last TR publication: http://dev.w3.org/2006/webapi/WebIDL/#indexed-and-named-properties This section did change just recently, from being a description of when properties get set and removed from the object, to being a description of [[GetOwnProperty]]/[[DefineOwnProperty]] behaviour to expose indexed and named properties. There was a thread on public-script-coord around this recently: http://www.w3.org/mid/20110503052431.GN2576@wok.mcc.id.au (In reply to comment #8) > (In reply to comment #7) > > Thanks for the insight. > > > Correction: I know that setters aren't cached. Assigning to dom properties would be hammered, but they are theoretically already slow. If we added caching to setters we would probably end up with a net win. > > > > > Getters will be hit by additional branches to validate the proto chain which is unfortunate. > > > > This will suck, but given that they're getter/setter pairs that we provide we might be able to remove at least _a_ branch from the current cached path. We should also be able to avoid constructing an actual function wrapper until we absolutely have to (getOwnPropertyDescriptor requires us to provide the function object) > > Could you point me to where this cache is in JSC? What cache? > > This is really an issue that needs to be resolves -- do index properties move to the prototype or do they stay own properties? > > According to the Web IDL spec, indexed properties are on the host object, not the prototype <http://www.w3.org/TR/WebIDL/#indexed-properties> Do they end up as getters or values? (In reply to comment #10) > (In reply to comment #8) > > (In reply to comment #7) > > > > Thanks for the insight. > > > > > Correction: I know that setters aren't cached. Assigning to dom properties would be hammered, but they are theoretically already slow. If we added caching to setters we would probably end up with a net win. > > > > > > > Getters will be hit by additional branches to validate the proto chain which is unfortunate. > > > > > > This will suck, but given that they're getter/setter pairs that we provide we might be able to remove at least _a_ branch from the current cached path. We should also be able to avoid constructing an actual function wrapper until we absolutely have to (getOwnPropertyDescriptor requires us to provide the function object) > > > > Could you point me to where this cache is in JSC? > > What cache? When you say setters aren’t cached, but because getters/setters come in pairs it could be possible to remove a branch from the "current cached path", what specifically are you referring too? I am still getting up to speed on JSC… > > > This is really an issue that needs to be resolves -- do index properties move to the prototype or do > they stay own properties? > > > > According to the Web IDL spec, indexed properties are on the host object, not the prototype <http://www.w3.org/TR/WebIDL/#indexed-properties> > > Do they end up as getters or values? From the link Cam sent, it looks like they appear to JS to be values in the current editor’s draft: <http://dev.w3.org/2006/webapi/WebIDL/#indexed-and-named-properties> If a host object implements an interface that supports indexed or named properties, the object will appear to have additional properties that correspond to the object’s indexed and named properties. These properties are not “real” own properties on the object, but are made to look like they are by being exposed by the [[GetOwnProperty]] internal method. … The internal [[GetOwnProperty]] method of every host object O that implements an interface that supports indexed or named properties must behave as follows … Let desc be a newly created property descriptor ([ECMA-262], section 8.10) with no fields. … Set desc.[[Value]] to the result of converting value to an ECMAScript value. (In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #8) > > > (In reply to comment #7) > > > > > > Thanks for the insight. > > > > > > > Correction: I know that setters aren't cached. Assigning to dom properties would be hammered, but they are theoretically already slow. If we added caching to setters we would probably end up with a net win. > > > > > > > > > Getters will be hit by additional branches to validate the proto chain which is unfortunate. > > > > > > > > This will suck, but given that they're getter/setter pairs that we provide we might be able to remove at least _a_ branch from the current cached path. We should also be able to avoid constructing an actual function wrapper until we absolutely have to (getOwnPropertyDescriptor requires us to provide the function object) > > > > > > Could you point me to where this cache is in JSC? > > > > What cache? > > When you say setters aren’t cached, but because getters/setters come in pairs it could be possible to remove a branch from the "current cached path", what specifically are you referring too? I am still getting up to speed on JSC… JSC has an internal idiosyncrasy/bug that means an object's structure is influenced by a new property being added as a getter or setter, but subsequently changing that getter or setter does not change the structure. This means we have to do a null check in the "cached" path. For builtin getters/setters we know they're well behaved and should be able to drop that bit of stupid, even if we don't fix the main getter/setter stupidity *** Bug 75297 has been marked as a duplicate of this bug. *** *** Bug 68002 has been marked as a duplicate of this bug. *** *** Bug 80060 has been marked as a duplicate of this bug. *** *** Bug 18737 has been marked as a duplicate of this bug. *** *** Bug 12721 has been marked as a duplicate of this bug. *** *** Bug 82031 has been marked as a duplicate of this bug. *** I just noticed that the descriptors have changed. However, both get and set are undefined.
Object.getOwnPropertyDescriptor(HTMLElement.prototype, 'title');
{
configurable: false,
enumerable: true,
get: undefined,
set: undefined
}
This is pretty bad since it looks like you support accessors but you really do not. I hope you are not shipping anything with this any time soon.
P.S. The only thing correct in that descriptor is the enumerable bit.
(In reply to comment #19) > I just noticed that the descriptors have changed. However, both get and set are undefined. > > Object.getOwnPropertyDescriptor(HTMLElement.prototype, 'title'); > { > configurable: false, > enumerable: true, > get: undefined, > set: undefined > } > > This is pretty bad since it looks like you support accessors but you really do not. I hope you are not shipping anything with this any time soon. This seems to be due to this change, by Oliver: http://trac.webkit.org/changeset/163035 > > P.S. The only thing correct in that descriptor is the enumerable bit. Configurability of WebIDL attributes is covered in bug #122018. (In reply to comment #19) > I just noticed that the descriptors have changed. However, both get and set are undefined. > > Object.getOwnPropertyDescriptor(HTMLElement.prototype, 'title'); > { > configurable: false, > enumerable: true, > get: undefined, > set: undefined > } > > This is pretty bad since it looks like you support accessors but you really do not. I hope you are not shipping anything with this any time soon. > > P.S. The only thing correct in that descriptor is the enumerable bit. It is preferable to get the property in the right place, even if the accessors are not initially accessible. Even the simple act of moving the property has been problematic due to sites doing stupid things with them. At least by getting these onto the prototype it becomes possible to introduce your own shim object to intercept if you really want to. As far as exposing the accessor functions we'll happily accept patches but the problem is avoiding huge amounts of code bloat This is now breaking real world sites because the code never assumes a falsy value for get/set
```js
if (descriptor.writable || descriptor.set) {
```
Like I've said before, the current behavior is a violation of ECMA 262
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-ordinary-object-internal-methods-and-internal-slots-get-p-receiver
Olli, would you accept patches that reverts this behavior?
(In reply to comment #22) > This is now breaking real world sites because the code never assumes a falsy value for get/set > Which sites? (In reply to comment #23) > (In reply to comment #22) > > This is now breaking real world sites because the code never assumes a falsy value for get/set > > > > Which sites? Anything based on Polymer but most likely other sites as well. www.chromestatus.com/features zeitgeist-globe.appspot.com/ (In reply to comment #21) > (In reply to comment #19) > > I just noticed that the descriptors have changed. However, both get and set are undefined. > > > > Object.getOwnPropertyDescriptor(HTMLElement.prototype, 'title'); > > { > > configurable: false, > > enumerable: true, > > get: undefined, > > set: undefined > > } > > > > <snip> > > As far as exposing the accessor functions we'll happily accept patches but > the problem is avoiding huge amounts of code bloat I believe I am addressing this in: <https://webkit.org/b/140575> Native Bindings Descriptors are Incomplete That said, there are still properties that haven't moved to the prototype chain. I won't be addressing those, and I'll continue outputting a broken descriptor. I filed: <https://webkit.org/b/141585> Some IDL attributes appear on the instances instead of on prototypes FYI, V8 & Blink moved almost all DOM attributes to prototype chains in Chrome 43. We're planning to complete the remaining ones in Chrome 44. Hi Kentaro, Are you aware of any compatibility issues because for this? We had a lot of properties that had to stay on the instance due to the delay in chromium implementing this :-/ As far as we know, the only website broken by this change is https://code.google.com/p/chromium/issues/detail?id=474436 However, we have just shipped the feature to a dev channel (Note: the feature had been kept in a canary channel for one month), so it's possible we haven't yet noticed the problems. Note that we successfully shipped this in Chrome. We did have a few compat issues, but pushed through them and most sites seem fixed now. See https://developers.google.com/web/updates/2015/04/DOM-attributes-now-on-the-prototype-chain?hl=en for details. Not having getters/setters makes it impossible, for example, to polyfill the new behavior of Event.timeStamp in Safari: https://github.com/majido/high-resolution-timestamp-polyfill/blob/master/high-resolution-timestamp-polyfill.js Skate also has to work around this behaviour: - https://github.com/skatejs/skatejs/blob/0445f33da2d6b8497b87deabad15ed060e69fc35/src/index.js#L72 - https://github.com/skatejs/skatejs/blob/5ba2048dc030b430e1cb14f3565f43dfe9e0fc7b/src/lifecycle/properties-created.js#L6 It also makes polyfilling any sort of Shadow DOM behaviour very painful and some of it impossible. I guess the same could be said about anything that requires access to native accessors which is more common than most think. This being a 6 year old bug (IE9 can also do this) makes me think that this isn't high on the priority list for anyone. I wish there was some way I could help with this but jumping into a bug like this would probably put me in way over my head. Would donating money help? I can do that. I will do that because I experience pain from this issue on a weekly basis. +1 here. This bug is very important for modern web apps. It is part of web standards and is supported in all other browsers and even IE.
1. Imagine a form. Form has inputs. All forms are stored in document.forms collection and all inputs are available through document.forms[form_id].elements accessor.
Now imagine form have input with name="elements". Try now to access .elements property, you will get this form input element not a collection. To fix this I am using simple getInputs() function which just calls constructor's .elements:
return Object.getOwnPropertyDescriptor(this._form.constructor.prototype, 'elements').get.call(this._form);
2. Second example: WebKit and Safari already allows overriding default setters/getters but in many cases developers need to call parent (prototype's) getters and setters:
Object.defineProperty(form_control, 'value', {
set: function (value) {
// call parent setter
Object.getOwnPropertyDescriptor(form_control.constructor.prototype, 'value').set.call(form_control, value);
}
// custom logic
},
// same for getter
});
(In reply to comment #32) > +1 here. This bug is very important for modern web apps. It is part of web > standards and is supported in all other browsers and even IE. I believe Chris Dumez already addressed a large number of these in bug 140575 and a number of follow-ups for sites that broke. There are some that are still some special cases, but the vast majority should be addressed. In fact, Chris, can we close this bug? The cases specifically mentioned by Erik in the first comment have been addressed. > 1. Imagine a form. Form has inputs. All forms are stored in document.forms > collection and all inputs are available through > document.forms[form_id].elements accessor. > > Now imagine form have input with name="elements". Try now to access > .elements property, you will get this form input element not a collection. > To fix this I am using simple getInputs() function which just calls > constructor's .elements: > > return Object.getOwnPropertyDescriptor(this._form.constructor.prototype, > 'elements').get.call(this._form); > > 2. Second example: WebKit and Safari already allows overriding default > setters/getters but in many cases developers need to call parent > (prototype's) getters and setters: > > Object.defineProperty(form_control, 'value', { > > set: function (value) { > // call parent setter > > Object.getOwnPropertyDescriptor(form_control.constructor.prototype, > 'value').set.call(form_control, value); > } > > // custom logic > }, > // same for getter > }); In latest Safari Technology Preview, these both appear to work for me. Are you seeing a case where it doesn't? If so, could you file a more specific bug with a test case? Yes, this has been implemented. Please file a new bug if some properties do not behave as expected still. |
This was discussed and agreed upon at the joint WebIDL ECMA TC39 meeting. Today, we store methods on the prototype but we always store the attributes on the instance. Current behavior: Object.getOwnPropertyDescriptor(document.body, 'nodeType') => {"value":1,"writable":true,"enumerable":true,"configurable":true} Object.getOwnPropertyDescriptor(Node.prototype, 'nodeType') => undefined Expected behavior: Object.getOwnPropertyDescriptor(document.body, 'nodeType') => undefined Object.getOwnPropertyDescriptor(Node.prototype, 'nodeType') => { get: function () { [native code] }, enumerable: true, configurable: true }