[binding] Check own property on named property accessor

[binding] Check own property on named property accessor

Current named property accessor overrides object's own property.

But unless [OverrideBuiltins] is specified to IDL,
object's own property should not be overridden by named property.
This patch fixes the problem.

Spec:
http://www.w3.org/TR/WebIDL/#OverrideBuiltins

FireFox and Opera do not override object's own property.

R=haraken@chromium.org
BUG=229740
TEST=collection-item-should-be-overridden-by-own-property.html,
     TestEventTarget.idl, TestInterface.idl, TestObject.idl

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151649

[kojih, 2013-05-30 09:54:15]

r?

Background:
StyleSheetList is checking own property
while all other named getter without [OverrideBuiltins]
currently do not check own property.
(checking property callback and properties in prototype chain)

Spec:
http://www.w3.org/TR/WebIDL/#OverrideBuiltins


Diff of generated code:
diff -u /mnt/hdd/diff/gen_rev/V8DOMNamedFlowCollection.cpp
/mnt/hdd/diff/gen_ch/V8DOMNamedFlowCollection.cpp
--- /mnt/hdd/diff/gen_rev/V8DOMNamedFlowCollection.cpp	2013-05-30
12:16:04.183982149 +0900
+++ /mnt/hdd/diff/gen_ch/V8DOMNamedFlowCollection.cpp	2013-05-30
18:32:15.973840768 +0900
@@ -130,6 +130,8 @@
         return v8Undefined();
     if (info.Holder()->HasRealNamedCallbackProperty(name))
         return v8Undefined();
+    if (info.Holder()->HasRealNamedProperty(name))
+        return v8Undefined();
 
     ASSERT(V8DOMWrapper::maybeDOMWrapper(info.Holder()));
     DOMNamedFlowCollection* collection = toNative(info.Holder());
diff -u /mnt/hdd/diff/gen_rev/V8HTMLAllCollection.cpp
/mnt/hdd/diff/gen_ch/V8HTMLAllCollection.cpp
--- /mnt/hdd/diff/gen_rev/V8HTMLAllCollection.cpp	2013-05-30 12:16:04.183982149
+0900
+++ /mnt/hdd/diff/gen_ch/V8HTMLAllCollection.cpp	2013-05-30 18:32:15.973840768
+0900
@@ -139,6 +139,8 @@
         return v8Undefined();
     if (info.Holder()->HasRealNamedCallbackProperty(name))
         return v8Undefined();
+    if (info.Holder()->HasRealNamedProperty(name))
+        return v8Undefined();
 
     ASSERT(V8DOMWrapper::maybeDOMWrapper(info.Holder()));
     HTMLAllCollection* collection = toNative(info.Holder());
diff -u /mnt/hdd/diff/gen_rev/V8HTMLCollection.cpp
/mnt/hdd/diff/gen_ch/V8HTMLCollection.cpp
--- /mnt/hdd/diff/gen_rev/V8HTMLCollection.cpp	2013-05-30 12:16:04.193982229
+0900
+++ /mnt/hdd/diff/gen_ch/V8HTMLCollection.cpp	2013-05-30 18:32:15.973840768
+0900
@@ -134,6 +134,8 @@
         return v8Undefined();
     if (info.Holder()->HasRealNamedCallbackProperty(name))
         return v8Undefined();
+    if (info.Holder()->HasRealNamedProperty(name))
+        return v8Undefined();
 
     ASSERT(V8DOMWrapper::maybeDOMWrapper(info.Holder()));
     HTMLCollection* collection = toNative(info.Holder());
diff -u /mnt/hdd/diff/gen_rev/V8HTMLFormControlsCollection.cpp
/mnt/hdd/diff/gen_ch/V8HTMLFormControlsCollection.cpp
--- /mnt/hdd/diff/gen_rev/V8HTMLFormControlsCollection.cpp	2013-05-30
12:16:04.193982229 +0900
+++ /mnt/hdd/diff/gen_ch/V8HTMLFormControlsCollection.cpp	2013-05-30
18:32:15.983840848 +0900
@@ -102,6 +102,8 @@
         return v8Undefined();
     if (info.Holder()->HasRealNamedCallbackProperty(name))
         return v8Undefined();
+    if (info.Holder()->HasRealNamedProperty(name))
+        return v8Undefined();
 
     ASSERT(V8DOMWrapper::maybeDOMWrapper(info.Holder()));
     HTMLFormControlsCollection* collection = toNative(info.Holder());
diff -u /mnt/hdd/diff/gen_rev/V8HTMLOptionsCollection.cpp
/mnt/hdd/diff/gen_ch/V8HTMLOptionsCollection.cpp
--- /mnt/hdd/diff/gen_rev/V8HTMLOptionsCollection.cpp	2013-05-30
12:16:04.193982229 +0900
+++ /mnt/hdd/diff/gen_ch/V8HTMLOptionsCollection.cpp	2013-05-30
18:32:15.983840848 +0900
@@ -161,6 +161,8 @@
         return v8Undefined();
     if (info.Holder()->HasRealNamedCallbackProperty(name))
         return v8Undefined();
+    if (info.Holder()->HasRealNamedProperty(name))
+        return v8Undefined();
 
     ASSERT(V8DOMWrapper::maybeDOMWrapper(info.Holder()));
     HTMLOptionsCollection* collection = toNative(info.Holder());
diff -u /mnt/hdd/diff/gen_rev/V8MimeTypeArray.cpp
/mnt/hdd/diff/gen_ch/V8MimeTypeArray.cpp
--- /mnt/hdd/diff/gen_rev/V8MimeTypeArray.cpp	2013-05-30 12:16:04.193982229
+0900
+++ /mnt/hdd/diff/gen_ch/V8MimeTypeArray.cpp	2013-05-30 18:32:15.983840848 +0900
@@ -126,6 +126,8 @@
         return v8Undefined();
     if (info.Holder()->HasRealNamedCallbackProperty(name))
         return v8Undefined();
+    if (info.Holder()->HasRealNamedProperty(name))
+        return v8Undefined();
 
     ASSERT(V8DOMWrapper::maybeDOMWrapper(info.Holder()));
     DOMMimeTypeArray* collection = toNative(info.Holder());
diff -u /mnt/hdd/diff/gen_rev/V8NamedNodeMap.cpp
/mnt/hdd/diff/gen_ch/V8NamedNodeMap.cpp
--- /mnt/hdd/diff/gen_rev/V8NamedNodeMap.cpp	2013-05-30 12:16:04.193982229 +0900
+++ /mnt/hdd/diff/gen_ch/V8NamedNodeMap.cpp	2013-05-30 18:32:15.983840848 +0900
@@ -215,6 +215,8 @@
         return v8Undefined();
     if (info.Holder()->HasRealNamedCallbackProperty(name))
         return v8Undefined();
+    if (info.Holder()->HasRealNamedProperty(name))
+        return v8Undefined();
 
     ASSERT(V8DOMWrapper::maybeDOMWrapper(info.Holder()));
     NamedNodeMap* collection = toNative(info.Holder());
diff -u /mnt/hdd/diff/gen_rev/V8PluginArray.cpp
/mnt/hdd/diff/gen_ch/V8PluginArray.cpp
--- /mnt/hdd/diff/gen_rev/V8PluginArray.cpp	2013-05-30 12:16:04.193982229 +0900
+++ /mnt/hdd/diff/gen_ch/V8PluginArray.cpp	2013-05-30 18:32:15.983840848 +0900
@@ -140,6 +140,8 @@
         return v8Undefined();
     if (info.Holder()->HasRealNamedCallbackProperty(name))
         return v8Undefined();
+    if (info.Holder()->HasRealNamedProperty(name))
+        return v8Undefined();
 
     ASSERT(V8DOMWrapper::maybeDOMWrapper(info.Holder()));
     DOMPluginArray* collection = toNative(info.Holder());
diff -u /mnt/hdd/diff/gen_rev/V8Plugin.cpp /mnt/hdd/diff/gen_ch/V8Plugin.cpp
--- /mnt/hdd/diff/gen_rev/V8Plugin.cpp	2013-05-30 12:16:04.193982229 +0900
+++ /mnt/hdd/diff/gen_ch/V8Plugin.cpp	2013-05-30 18:32:15.983840848 +0900
@@ -165,6 +165,8 @@
         return v8Undefined();
     if (info.Holder()->HasRealNamedCallbackProperty(name))
         return v8Undefined();
+    if (info.Holder()->HasRealNamedProperty(name))
+        return v8Undefined();
 
     ASSERT(V8DOMWrapper::maybeDOMWrapper(info.Holder()));
     DOMPlugin* collection = toNative(info.Holder());
diff -u /mnt/hdd/diff/gen_rev/V8RTCStatsResponse.cpp
/mnt/hdd/diff/gen_ch/V8RTCStatsResponse.cpp
--- /mnt/hdd/diff/gen_rev/V8RTCStatsResponse.cpp	2013-05-30 12:16:04.193982229
+0900
+++ /mnt/hdd/diff/gen_ch/V8RTCStatsResponse.cpp	2013-05-30 18:32:15.983840848
+0900
@@ -99,6 +99,8 @@
         return v8Undefined();
     if (info.Holder()->HasRealNamedCallbackProperty(name))
         return v8Undefined();
+    if (info.Holder()->HasRealNamedProperty(name))
+        return v8Undefined();
 
     ASSERT(V8DOMWrapper::maybeDOMWrapper(info.Holder()));
     RTCStatsResponse* collection = toNative(info.Holder());
diff -u /mnt/hdd/diff/gen_rev/V8Storage.cpp /mnt/hdd/diff/gen_ch/V8Storage.cpp
--- /mnt/hdd/diff/gen_rev/V8Storage.cpp	2013-05-30 12:16:04.193982229 +0900
+++ /mnt/hdd/diff/gen_ch/V8Storage.cpp	2013-05-30 18:32:15.993840928 +0900
@@ -292,6 +292,8 @@
         return v8Undefined();
     if (info.Holder()->HasRealNamedCallbackProperty(name))
         return v8Undefined();
+    if (info.Holder()->HasRealNamedProperty(name))
+        return v8Undefined();
 
     ASSERT(V8DOMWrapper::maybeDOMWrapper(info.Holder()));
     Storage* collection = toNative(info.Holder());
@@ -311,6 +313,8 @@
         return v8Undefined();
     if (info.Holder()->HasRealNamedCallbackProperty(name))
         return v8Undefined();
+    if (info.Holder()->HasRealNamedProperty(name))
+        return v8Undefined();
     Storage* collection = toNative(info.Holder());
     V8TRYCATCH_FOR_V8STRINGRESOURCE(V8StringResource<>, propertyName, name);
     ExceptionCode ec = 0;

[haraken, 2013-05-30 10:23:09]

https://codereview.chromium.org/15690020/diff/1/LayoutTests/fast/dom/collecti...
File
LayoutTests/fast/dom/collection-item-should-be-overridden-by-own-property.html
(right):

https://codereview.chromium.org/15690020/diff/1/LayoutTests/fast/dom/collecti...
LayoutTests/fast/dom/collection-item-should-be-overridden-by-own-property.html:23:
<tr id="length"><td></td></tr>

This test contains a bunch of unnecessary things. For example, <form> and <tr>
are not needed. Would you clean up the test?

https://codereview.chromium.org/15690020/diff/1/LayoutTests/fast/dom/collecti...
LayoutTests/fast/dom/collection-item-should-be-overridden-by-own-property.html:27:
<script
src="script-tests/collection-item-should-be-overridden-by-own-property.js"></script>

You can directly write JavaScript here.

https://codereview.chromium.org/15690020/diff/1/Source/bindings/scripts/CodeG...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://codereview.chromium.org/15690020/diff/1/Source/bindings/scripts/CodeG...
Source/bindings/scripts/CodeGeneratorV8.pm:3398: $code .= "        return
v8Undefined();\n";

I got a bit confused. I thought you were saying that
GetRealNamedPropertyInPrototypeChain() and HasRealNamedCallbackProperty() are
needed but HasRealNamedProperty() shouldn't be added, to keep compatibility with
other browsers. Maybe am I misunderstanding?

[kojih, 2013-06-03 04:53:13]

> I got a bit confused. I thought you were saying that
> GetRealNamedPropertyInPrototypeChain() and HasRealNamedCallbackProperty() are
> needed but HasRealNamedProperty() shouldn't be added, to keep compatibility
with
> other browsers. Maybe am I misunderstanding?

To be exact,
- If [OverrideBuiltins] is specified, call HasRealNamedProperty,
GetRealNamedPropertyInPrototypeChain and HasRealNamedCallbackProperty
- Otherwise, call nothing
is correct per spec and behavior of other browsers.

But since all named getters (seemed to) call just
GetRealNamedPropertyInPrototypeChain and HasRealNamedCallbackProperty, (not
HasRealNamedProperty)
we took downward compatibility of chromium.


But after that, I found above was wrong because HasRealNamedProperty was called
by StyleSheetList.
(only StyleSheetList call HasRealNamedProperty)

So this patch is to fix all named getters/setters so that call all of 3 checks
and make chromium along with spec and other browsers.

[haraken, 2013-06-03 07:03:19]

On 2013/06/03 04:53:13, kojih wrote:
> So this patch is to fix all named getters/setters so that call all of 3 checks
> and make chromium along with spec and other browsers.

Thanks for the clarification! Then the change looks good (other than we want to
clean up the test as I commented above).

The underlying core issue about our confusions is we don't have good test
coverage for named/indexed properties. Because I don't want to block you from
landing the CL anymore, I'm OK with landing the CL as is. However, I strongly
wish you add good test cases for all named/indexed properties (based on the
precious knowledge you've found recently) in follow-up CLs.

[kojih, 2013-06-03 09:28:14]

r?

removed unused tags in test.

[haraken, 2013-06-03 09:42:53]

LGTM. Let's add more test cases in follow-up patches!

https://codereview.chromium.org/15690020/diff/8001/LayoutTests/fast/dom/colle...
File
LayoutTests/fast/dom/collection-item-should-be-overridden-by-own-property.html
(right):

https://codereview.chromium.org/15690020/diff/8001/LayoutTests/fast/dom/colle...
LayoutTests/fast/dom/collection-item-should-be-overridden-by-own-property.html:1:
<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Nit: <!DOCTYPE> is for HTML5.

https://codereview.chromium.org/15690020/diff/8001/LayoutTests/fast/dom/colle...
LayoutTests/fast/dom/collection-item-should-be-overridden-by-own-property.html:37:


Nit: Unnecessary empty lines.

[commit-bot: I haz the power, 2013-06-03 09:46:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kojih@chromium.org/15690020/8002

[commit-bot: I haz the power, 2013-06-03 10:33:49]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-06-03 10:42:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kojih@chromium.org/15690020/8002

[commit-bot: I haz the power, 2013-06-03 11:26:40]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-06-03 11:29:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kojih@chromium.org/15690020/8002

[commit-bot: I haz the power, 2013-06-03 11:43:33]

Change committed as 151649