window.document does not handle named properties correctly |
|||||||||
Issue descriptionWindowProxy depends on V8 to install named properties into a Document instance. This routine is run everytime we create documents, i.e. <iframe>, even if the document do not use V8 script. It is also not good that named properties for documents are handled here, without setting named property accessors in IDL.
,
May 13 2016
The named property visibility algorithm http://heycam.github.io/webidl/#dfn-named-property-visibility See "Note". The priority is: 1. indexed properties 2. unforgeable attributes and operations 3. others... Indexed properties and unforgeable attributes and operations should be always prioritized over named properties.
,
May 16 2016
Toom, Jochen, Do you know how unforgeable properties are handled in V8? IIRC, [Unforgeable] label is removed in binding layer, and V8 does not know which properties are unforgeable.
,
May 16 2016
The Web IDL spec requires to look up [Unforgeable] properties and own properties with a different priority. I think we need to introduce a dedicated V8 API to install [Unforgeable] properties?
,
May 20 2016
Toon, Don't you know if V8 has APIs to install [Unforgeable] properties? Or could you suggest someone who may know it?
,
May 20 2016
There is no such API indeed. The API for the first (appending interceptors) was only introduced fairly recently to be able to know within V8 what blink is trying to accomplish. No such equivalent for unforgeable was introduced. Previously any special ordering was done in blink by using the *RealNamedProperty APIs.
,
May 20 2016
I heard you have a plan to expose some callbacks so that the embedder can intercept property access more powerfully. Would you take into account the unforgeable-attribute use case when designing the callback API?
,
May 20 2016
I sounds orthogonal to what I'm trying to accomplish with the new interceptors. If you can point me at the requirements, I can have a look at it though. Is this something that's performance-critical in any way?
,
May 20 2016
See the comments in #2, #3 and #4. The spec is http://heycam.github.io/webidl/#dfn-named-property-visibility. We need the behavior not for performance but for correctness.
,
May 20 2016
I do not understand what those comments mean, there must be jargon mismatch. We have: 1) properties on the object (object) 2) properties returned by an interceptor attached to the object (interceptor) 3) properties from the prototype chain (prototype chain) regular interceptors do: 1) interceptor 2) object 3) prototype chain appending interceptors do: 1) object 2) prototype chain 3) interceptor I presume you are talking about an interceptor that (only?) implements unforgeable properties? In that case I presume no property can appear on the object for that particular name; otherwise it wouldn't be unforgeable. At the same time, if a new unforgeable property appears in the interceptor; it has to replace a property on the object; and if it can't, it should fail to introduce the unforgeable property. In that case there are only 2 lookups: 1) unforgeable property from the interceptor 2) property from the prototype chain That; or the unforgeable property has to be installed as a non-configurable property on the object; and the interceptor cannot interfere. In that case the 2 steps are: 1) object 2) prototype chain The first of those 2 options we would have to support. The second should just work out-of-the-box from the V8 side.
,
May 23 2016
> regular interceptors do: > 1) interceptor > 2) object > 3) prototype chain This is used for [OverrideBuiltin] attributes. > appending interceptors do: > 1) object > 2) prototype chain > 3) interceptor This is used for normal attributes. And what we need for [Unforgeable] attributes is: 1) object (own attributes including [Unforgeable] attributes) 2) interceptor (named properties) 3) prototype chain Does it make sense? Just to confirm: "Regular interceptors" mean masking interceptors, and "appending interceptors" means non-masking interceptors, right?
,
May 23 2016
Let me clarify the situation. 1. What the spec requires is here. The named property visibility algorithm http://heycam.github.io/webidl/#dfn-named-property-visibility We need to distinguish the following 4 types of properties in the case of [OverrideBuiltins]. 1) [Unforgeable] properties (unforgeable) 2) named properties, i.e. properties returned by an interceptor (interceptor) 3) own properties, i.e. properties on the object (object) 4) properties from the prototype chain (prototype chain) The key point is that [Unforgeable] attributes are defined on the object as "own properties", but it's prioritized over named properties while no-[Unforgeable] own properties are less prioritized than named properties. 2. What the binding team is expecting for V8 team. V8 handles [Unforgeable] attributes accordingly. The binding layer sets a special flag such as v8::UNFORGEABLE so that V8 can prioritize [Unforgeable] attributes. 3. What Toon suggested in #6 The binding layer can handle [Unforgeable] attributes and prioritize them over named properties. In the interceptor for the named properties, we can determine if the "property name" indicates an [Unforgeable] attribute or not, and return the [Unforgeable] attribute if found. 4. My opinion 4-1. We can fix this problem with either in V8 or in the binding layer. 4-2. There are only a few interfaces with [OverrideBuiltins]. 4-3. At a glance (not 100% sure), Document is the only interface that has [OverrideBuiltins] and also has an [Unforgeable] attribute. I think we can introduce a special casing in the binding layer and we can solve this problem. Handling only one exceptional case in V8 would be inefficient.
,
May 23 2016
Thanks shiino-san for the clarification! I think I'm on the same page and agree with Toon's approach (i.e., option 3 in comment #12).
,
May 24 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/09649cf329fd0644f50624b80f6a2f9c564dcb2d commit 09649cf329fd0644f50624b80f6a2f9c564dcb2d Author: peria <peria@chromium.org> Date: Tue May 24 11:39:50 2016 Add [OverrideBuiltins] label onto HTMLDocument interface. This CL also removes special handling of named properties using V8 script controller, which is no longer needed. Currently, we have no general routines to work for [Unforgeable] on [OverrideBuiltins] interface, and "location" in "HTMLDocument" is the only one which meets the situation. So now, we handle it as an edge case of named property. Spec: https://html.spec.whatwg.org/multipage/dom.html#dom-document-namedItem-which Updated a test expectation; Chrome has been FAILing it, while IE, FF, and Safari PASS it. BUG=611632 Review-Url: https://codereview.chromium.org/1837823003 Cr-Commit-Position: refs/heads/master@{#395570} [modify] https://crrev.com/09649cf329fd0644f50624b80f6a2f9c564dcb2d/third_party/WebKit/LayoutTests/imported/wpt/html/dom/documents/dom-tree-accessors/nameditem-06-expected.txt [modify] https://crrev.com/09649cf329fd0644f50624b80f6a2f9c564dcb2d/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp [modify] https://crrev.com/09649cf329fd0644f50624b80f6a2f9c564dcb2d/third_party/WebKit/Source/bindings/core/v8/ScriptController.h [modify] https://crrev.com/09649cf329fd0644f50624b80f6a2f9c564dcb2d/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp [modify] https://crrev.com/09649cf329fd0644f50624b80f6a2f9c564dcb2d/third_party/WebKit/Source/bindings/core/v8/WindowProxy.h [add] https://crrev.com/09649cf329fd0644f50624b80f6a2f9c564dcb2d/third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp [modify] https://crrev.com/09649cf329fd0644f50624b80f6a2f9c564dcb2d/third_party/WebKit/Source/bindings/core/v8/custom/custom.gypi [modify] https://crrev.com/09649cf329fd0644f50624b80f6a2f9c564dcb2d/third_party/WebKit/Source/core/html/HTMLDocument.cpp [modify] https://crrev.com/09649cf329fd0644f50624b80f6a2f9c564dcb2d/third_party/WebKit/Source/core/html/HTMLDocument.idl
,
May 25 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/17ab3580c8d3081a1c728eb33d6e5f35684fd5a5 commit 17ab3580c8d3081a1c728eb33d6e5f35684fd5a5 Author: peria <peria@chromium.org> Date: Wed May 25 07:30:04 2016 Revert of [Binding] Add [OverrideBuiltins] label onto HTMLDocument interface (patchset #3 id:460001 of https://codereview.chromium.org/1837823003/ ) Reason for revert: performance regression BUG= 614559 Original issue's description: > Add [OverrideBuiltins] label onto HTMLDocument interface. > > This CL also removes special handling of named properties > using V8 script controller, which is no longer needed. > > Currently, we have no general routines to work for [Unforgeable] > on [OverrideBuiltins] interface, and "location" in "HTMLDocument" > is the only one which meets the situation. > So now, we handle it as an edge case of named property. > > Spec: https://html.spec.whatwg.org/multipage/dom.html#dom-document-namedItem-which > > Updated a test expectation; Chrome has been FAILing it, while IE, FF, and Safari PASS it. > > BUG=611632 > > Committed: https://crrev.com/09649cf329fd0644f50624b80f6a2f9c564dcb2d > Cr-Commit-Position: refs/heads/master@{#395570} TBR=haraken@chromium.org,yukishiino@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=611632 Review-Url: https://codereview.chromium.org/2011553003 Cr-Commit-Position: refs/heads/master@{#395824} [modify] https://crrev.com/17ab3580c8d3081a1c728eb33d6e5f35684fd5a5/third_party/WebKit/LayoutTests/imported/wpt/html/dom/documents/dom-tree-accessors/nameditem-06-expected.txt [modify] https://crrev.com/17ab3580c8d3081a1c728eb33d6e5f35684fd5a5/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp [modify] https://crrev.com/17ab3580c8d3081a1c728eb33d6e5f35684fd5a5/third_party/WebKit/Source/bindings/core/v8/ScriptController.h [modify] https://crrev.com/17ab3580c8d3081a1c728eb33d6e5f35684fd5a5/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp [modify] https://crrev.com/17ab3580c8d3081a1c728eb33d6e5f35684fd5a5/third_party/WebKit/Source/bindings/core/v8/WindowProxy.h [delete] https://crrev.com/723e64da51aa205b4f69660be9d3b59c67f37fe4/third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp [modify] https://crrev.com/17ab3580c8d3081a1c728eb33d6e5f35684fd5a5/third_party/WebKit/Source/bindings/core/v8/custom/custom.gypi [modify] https://crrev.com/17ab3580c8d3081a1c728eb33d6e5f35684fd5a5/third_party/WebKit/Source/core/html/HTMLDocument.cpp [modify] https://crrev.com/17ab3580c8d3081a1c728eb33d6e5f35684fd5a5/third_party/WebKit/Source/core/html/HTMLDocument.idl
,
Jun 8 2016
I'm testing performance regressions with the reverted CL. We need some special hacks (i.e. non standard way) to avoid the regression. Even if we return immediately in V8HTMLDocument::namedPropertyGetterCustom(), the performance tests regressed largely. Hmmm... :( (e.g. 10^6 document.createElement() takes 380ms --> 770ms)
,
Jun 8 2016
verwaest, jochen: Do you have any idea to make the overhead of the interceptor faster? The overhead prevents us from landing https://codereview.chromium.org/1837823003. (Decreasing the intereceptor overhead will be very important for making window.foo faster as well.)
,
Jul 1 2016
,
Jul 1 2016
,
Jul 1 2016
,
Nov 10 2016
,
Nov 10 2016
,
Nov 10 2016
,
Dec 13 2016
,
Jul 14 2017
,
Aug 21 2017
Issue 756771 has been merged into this issue.
,
Aug 21 2017
peria@, Issue 756771 , name getter behavior bug, is independent from this one. |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by peria@chromium.org
, May 13 2016