v8binding: Don't allow author script to define indexed accessor prop.

v8binding: Don't allow author script to define indexed accessor prop.

Author script is not allowed to define an accessor property with
an array index property name if the object supports indexed properties.

This CL disallows it.

BUG=695385
Review-Url: https://codereview.chromium.org/2832923003
Cr-Commit-Position: refs/heads/master@{#484871}
Committed: https://chromium.googlesource.com/chromium/src/+/58ab4a971b06dec13e4edf9de8382ca6847f6190

[Yuki, 2017-04-21 09:06:43]

yukishiino@chromium.org changed reviewers:
+ domenic@chromium.org

[Yuki, 2017-04-21 09:06:43]

domenic@, could you tell me if I understand the spec correctly or I'm
misunderstanding?  My understanding is as follows.

https://tc39.github.io/ecma262/#sec-integer-indexed-exotic-objects-defineownp...
step 3. b. vi. If IsAccessorDescriptor(Desc) is true, return false.

ECMAScript says that author script cannot define an accessor property whose
property name is an array index property name on an Integer Indexed exotic
object.

I interpret it as "platform objects that support indexed properties are Integer
Indexed exotic object".  (Is this correct?)

Then, on platform objects that support indexed properties, it's not allowed to
define an accessor property with an array index property name.

In this CL, I'm planning to disallow it for all platform objects that support
"indexed setter".

I'm not sure if my understanding is really correct or not.  Also I'm not sure
what we should do in case of platform objects that only support "indexed
getter".

Could you help me understand things?

[domenic, 2017-04-22 00:09:46]

On 2017/04/21 at 09:06:43, yukishiino wrote:
> domenic@, could you tell me if I understand the spec correctly or I'm
misunderstanding?  My understanding is as follows.
> 
>
https://tc39.github.io/ecma262/#sec-integer-indexed-exotic-objects-defineownp...
> step 3. b. vi. If IsAccessorDescriptor(Desc) is true, return false.
> 
> ECMAScript says that author script cannot define an accessor property whose
property name is an array index property name on an Integer Indexed exotic
object.
> 
> I interpret it as "platform objects that support indexed properties are
Integer Indexed exotic object".  (Is this correct?)
> 
> Then, on platform objects that support indexed properties, it's not allowed to
define an accessor property with an array index property name.
> 
> In this CL, I'm planning to disallow it for all platform objects that support
"indexed setter".
> 
> I'm not sure if my understanding is really correct or not.  Also I'm not sure
what we should do in case of platform objects that only support "indexed
getter".
> 
> Could you help me understand things?

This isn't quite correct. ECMAScript only defines integer indexed exotic objects
for its own purposes. They are unrelated to platform objects with indexed
getters.

The spec for platform objects with indexed getters/setters is at
https://heycam.github.io/webidl/#es-legacy-platform-objects. However it contains
a very similar step in
https://heycam.github.io/webidl/#legacy-platform-object-defineownproperty step
1.1. So probably the patch is fine.

I hope you can add tests in external/wpt somewhere. I guess a new top-level
webidl directory might make sense?

[Yuki, 2017-04-26 11:19:17]

On 2017/04/22 00:09:46, domenic wrote:
> The spec for platform objects with indexed getters/setters is at
> https://heycam.github.io/webidl/#es-legacy-platform-objects. However it
contains
> a very similar step in
> https://heycam.github.io/webidl/#legacy-platform-object-defineownproperty step
> 1.1. So probably the patch is fine.
> 
> I hope you can add tests in external/wpt somewhere. I guess a new top-level
> webidl directory might make sense?

Thanks for the pointer.  It makes sense.  Let me give it a try.

[Yuki, 2017-04-26 11:48:19]

On 2017/04/22 00:09:46, domenic wrote:
> I hope you can add tests in external/wpt somewhere. I guess a new top-level
> webidl directory might make sense?

Do you mean that, if I landed a test in Chromium's external/wpt/, it's
automatically upstreamed to github.com/w3c/web-platform-tests?  Or are you just
suggesting to add a test directly to github.com/w3c/web-platform-tests?

[domenic, 2017-04-26 17:20:57]

On 2017/04/26 at 11:48:19, yukishiino wrote:
> On 2017/04/22 00:09:46, domenic wrote:
> > I hope you can add tests in external/wpt somewhere. I guess a new top-level
> > webidl directory might make sense?
> 
> Do you mean that, if I landed a test in Chromium's external/wpt/, it's
automatically upstreamed to github.com/w3c/web-platform-tests?  Or are you just
suggesting to add a test directly to github.com/w3c/web-platform-tests?

My understanding is that it's automatically upstreamed, although I admit it
might be different for top-level directories which don't exist in the upstream
web-platform-tests repository. (There is no webidl/ in
https://github.com/w3c/web-platform-tests right now.) Maybe +qyearsley or
+jeffcarp knows about that case.

Either way works for me, but I hope we get a test into web platform tests one
way or another.

[jeffcarp, 2017-04-26 17:28:01]

On 2017/04/26 at 17:20:57, domenic wrote:
> On 2017/04/26 at 11:48:19, yukishiino wrote:
> > On 2017/04/22 00:09:46, domenic wrote:
> > > I hope you can add tests in external/wpt somewhere. I guess a new
top-level
> > > webidl directory might make sense?
> > 
> > Do you mean that, if I landed a test in Chromium's external/wpt/, it's
automatically upstreamed to github.com/w3c/web-platform-tests?  Or are you just
suggesting to add a test directly to github.com/w3c/web-platform-tests?
> 
> My understanding is that it's automatically upstreamed, although I admit it
might be different for top-level directories which don't exist in the upstream
web-platform-tests repository. (There is no webidl/ in
https://github.com/w3c/web-platform-tests right now.) Maybe +qyearsley or
+jeffcarp knows about that case.
> 
> Either way works for me, but I hope we get a test into web platform tests one
way or another.

Yep, creating a new top level directory should work fine for export. I'm less
sure about import, but I can keep an eye on this CL as it goes thorough the
process.

[Yuki, 2017-07-04 09:29:30]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-04 09:29:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-04 10:35:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-04 10:35:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yuki, 2017-07-04 13:20:51]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-04 13:21:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuki, 2017-07-04 13:21:32]

Description was changed from

==========
v8binding: Don't allow author script to define indexed accessor prop.

Author script is not allowed to define an accessor property with
an array index property name if the object supports indexed properties,
i.e. Integer Indexed exotic object.

This CL disallows it.

BUG=695385
==========

to

==========
v8binding: Don't allow author script to define indexed accessor prop.

Author script is not allowed to define an accessor property with
an array index property name if the object supports indexed properties.

This CL disallows it.

BUG=695385
==========

[commit-bot: I haz the power, 2017-07-04 13:49:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-04 13:49:47]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[Yuki, 2017-07-04 13:53:00]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-04 13:53:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuki, 2017-07-04 13:59:12]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-04 13:59:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-04 14:59:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-04 14:59:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yuki, 2017-07-05 06:07:41]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-05 06:07:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuki, 2017-07-05 07:40:01]

yukishiino@chromium.org changed reviewers:
+ haraken@chromium.org

[Yuki, 2017-07-05 07:40:03]

I'm sorry for not working on this CL for a long time.  I'm finally back.

domenic@, could you review this CL especially for layout tests including WPTs? 
I'm adding external/wpt/WebIDL/ecmascript-binding/legacy-platform-object.html to
test indexed properties of legacy platform objects.  For WindowProxy's indexed
properties, it turned out that we already have
external/wpt/html/browsers/the-window-object/window-indexed-properties.html and
it's testing well.

haraken@, could you review overall?

Cheers,
Yuki Shiino

[commit-bot: I haz the power, 2017-07-05 07:53:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-05 07:53:58]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-07-05 11:51:13]

https://codereview.chromium.org/2832923003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2832923003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:144: if
(info.ShouldThrowOnError()) {

What is info.ShouldThrowOnError()? I saw this the first time. When should we
call it?

https://codereview.chromium.org/2832923003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:239:
V8SetReturnValue(info, v8::Null(info.GetIsolate()));

Does this mean we "return false"?

https://codereview.chromium.org/2832923003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:249: // Retutn
nothing and fall back to indexedPropertySetterCallback.

Return

[Yuki, 2017-07-05 12:33:15]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-05 12:33:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuki, 2017-07-05 12:37:42]

https://codereview.chromium.org/2832923003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2832923003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:144: if
(info.ShouldThrowOnError()) {
On 2017/07/05 11:51:13, haraken wrote:
> 
> What is info.ShouldThrowOnError()? I saw this the first time.

It corresponds to "use strict"; in ES.
  window[0] = true;
should be simple noop (just ignore the assignment), but
  "use strict";
  window[0] = true;
should throw a TypeError.

info.ShouldThrowOnError() tells us whether we should report an error as an
exception or not.

> When should we call it?

I don't have a simple answer.  It depends on API specifications.  There are
cases that we should throw even if no "use strict" specified.

https://codereview.chromium.org/2832923003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:239:
V8SetReturnValue(info, v8::Null(info.GetIsolate()));
On 2017/07/05 11:51:13, haraken wrote:
> 
> Does this mean we "return false"?

This V8 callback API doesn't correspond to the spec here.  The return value has
nothing to do with "return false" in the spec.

V8 falls back to OrdinaryDefineOwnProperty when the return value is the empty
handle (not even undefined), and stops further processing when the return value
is a non-empty handle (whatever the value is).  Only empty or non-empty matters.

Could be a little confusing, but it's documented in v8.h.
https://cs.chromium.org/chromium/src/v8/include/v8.h?q=GenericNamedPropertyDe...

https://codereview.chromium.org/2832923003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:249: // Retutn
nothing and fall back to indexedPropertySetterCallback.
On 2017/07/05 11:51:13, haraken wrote:
> 
> Return

Done.

[commit-bot: I haz the power, 2017-07-05 14:13:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-05 14:13:30]

Dry run: This issue passed the CQ dry run.

[domenic, 2017-07-05 20:12:26]

Some minor suggestions for additional things to test, but the tests written look
great.

https://codereview.chromium.org/2832923003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/WebIDL/ecmascript-binding/legacy-platform-object.html
(right):

https://codereview.chromium.org/2832923003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/WebIDL/ecmascript-binding/legacy-platform-object.html:30:
});
Like below, it would be good to check that the property descriptor remains
unchanged, not just the value. In particular it should be enumerable true,
configurable true, writable false.

https://codereview.chromium.org/2832923003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/WebIDL/ecmascript-binding/legacy-platform-object.html:40:
});
You should also test the success case. In particular you should test that no
matter what the values for configurable/writable/enumerable are, they are
ignored. The property is set according to the `value` of the descriptor, but
when you do `getOwnPropertyDescriptor` back, it still returns according to
https://heycam.github.io/webidl/#LegacyPlatformObjectGetOwnProperty (with
enumerable/configurable/writable true)

[haraken, 2017-07-06 01:13:19]

LGTM

[Yuki, 2017-07-06 09:11:06]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 09:11:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuki, 2017-07-06 09:11:49]

Patchset #8 (id:140001) has been deleted

[Yuki, 2017-07-06 09:14:46]

domenic@, your comments totally make sense.  I've updated the test.  Could you
take another look?

(Note: Blink now fails with the new (improved) test.  Even Firefox fails a
little.)

https://codereview.chromium.org/2832923003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/WebIDL/ecmascript-binding/legacy-platform-object.html
(right):

https://codereview.chromium.org/2832923003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/WebIDL/ecmascript-binding/legacy-platform-object.html:30:
});
On 2017/07/05 20:12:26, domenic wrote:
> Like below, it would be good to check that the property descriptor remains
> unchanged, not just the value. In particular it should be enumerable true,
> configurable true, writable false.

Done.

https://codereview.chromium.org/2832923003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/WebIDL/ecmascript-binding/legacy-platform-object.html:40:
});
On 2017/07/05 20:12:26, domenic wrote:
> You should also test the success case. In particular you should test that no
> matter what the values for configurable/writable/enumerable are, they are
> ignored. The property is set according to the `value` of the descriptor, but
> when you do `getOwnPropertyDescriptor` back, it still returns according to
> https://heycam.github.io/webidl/#LegacyPlatformObjectGetOwnProperty (with
> enumerable/configurable/writable true)

Done.

[commit-bot: I haz the power, 2017-07-06 10:36:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-06 10:36:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[domenic, 2017-07-06 18:42:19]

Really awesome test coverage; lgtm. I guess it needs some -expected.txt files to
pass CI though.

[Yuki, 2017-07-07 07:55:09]

Oops, I forgot to do "git add".  Done.

[Yuki, 2017-07-07 07:55:15]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2017-07-07 07:55:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, domenic@chromium.org
Link to the patchset: https://codereview.chromium.org/2832923003/#ps180001
(title: "Added the test expectation.")

[commit-bot: I haz the power, 2017-07-07 07:55:27]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-07 09:59:54]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1499414115455140,
"parent_rev": "af5eed337751ca4f227f80a722c3c374dcf7973e", "commit_rev":
"58ab4a971b06dec13e4edf9de8382ca6847f6190"}

[commit-bot: I haz the power, 2017-07-07 10:00:08]

Description was changed from

==========
v8binding: Don't allow author script to define indexed accessor prop.

Author script is not allowed to define an accessor property with
an array index property name if the object supports indexed properties.

This CL disallows it.

BUG=695385
==========

to

==========
v8binding: Don't allow author script to define indexed accessor prop.

Author script is not allowed to define an accessor property with
an array index property name if the object supports indexed properties.

This CL disallows it.

BUG=695385

Review-Url: https://codereview.chromium.org/2832923003
Cr-Commit-Position: refs/heads/master@{#484871}
Committed:
https://chromium.googlesource.com/chromium/src/+/58ab4a971b06dec13e4edf9de838...
==========

[commit-bot: I haz the power, 2017-07-07 10:00:10]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/58ab4a971b06dec13e4edf9de838...