binding: Returns a reject promise when |this| is not of the type.

binding: Returns a reject promise when |this| is not of the type.

Returns a reject promise when the receiver object is not of type
of the interface, instead of throwing a TypeError.

This CL disables the type check on V8 side and does the type check
on Blink side, then, if the receiver object is not of the type,
converts a TypeError to a reject promise and returns it.

BUG=627309
Committed: https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa
Cr-Commit-Position: refs/heads/master@{#432798}

[Yuki, 2016-10-20 09:16:13]

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

[commit-bot: I haz the power, 2016-10-20 09:16:24]

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, 2016-10-20 10:35:06]

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

[commit-bot: I haz the power, 2016-10-20 10:35:07]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Yuki, 2016-10-20 11:59:03]

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

[commit-bot: I haz the power, 2016-10-20 11:59:10]

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, 2016-10-20 13:34:52]

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

[commit-bot: I haz the power, 2016-10-20 13:34:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yuki, 2016-10-21 07:09:12]

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

[commit-bot: I haz the power, 2016-10-21 07:09:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuki, 2016-10-21 07:10:17]

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

[Yuki, 2016-10-21 07:10:18]

Could you review this CL?

[haraken, 2016-10-21 08:11:28]

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

[haraken, 2016-10-21 08:11:28]

+jochen

I'm fine with landing this as a short-term fix, but would it make more sense to
fix it in the V8 side? i.e., make V8 return a rejected promise when the holder
type check fails.

https://codereview.chromium.org/2441593002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right):

https://codereview.chromium.org/2441593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:33:
info.Holder()->GetPrototype().As<v8::Object>();

Can we remove the additional complexity by using
V8XXX::findInstanceInPrototypeChain?

[commit-bot: I haz the power, 2016-10-21 08:59:47]

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

[commit-bot: I haz the power, 2016-10-21 08:59:48]

Dry run: This issue passed the CQ dry run.

[Yuki, 2016-10-21 10:11:19]

https://codereview.chromium.org/2441593002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right):

https://codereview.chromium.org/2441593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:33:
info.Holder()->GetPrototype().As<v8::Object>();
On 2016/10/21 08:11:28, haraken wrote:
> 
> Can we remove the additional complexity by using
> V8XXX::findInstanceInPrototypeChain?

I tried it, and I found it didn't work as expected.

If we used it,
    x = {}
    x.__proto__ = window
    window.fetch.call(x, 'foo')
works, although |x| is not an instance of Window interface.  IIUC, |fetch| must
return a reject promise of TypeError because |x| is not of type Window.

Instead of using finInstanceInPrototypeChain, I'd like to ask V8 team to pass
the global object (instead of the global *proxy* object) as info.Holder().

[haraken, 2016-10-21 10:24:49]

On 2016/10/21 10:11:19, Yuki wrote:
>
https://codereview.chromium.org/2441593002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right):
> 
>
https://codereview.chromium.org/2441593002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:33:
> info.Holder()->GetPrototype().As<v8::Object>();
> On 2016/10/21 08:11:28, haraken wrote:
> > 
> > Can we remove the additional complexity by using
> > V8XXX::findInstanceInPrototypeChain?
> 
> I tried it, and I found it didn't work as expected.
> 
> If we used it,
>     x = {}
>     x.__proto__ = window
>     window.fetch.call(x, 'foo')
> works, although |x| is not an instance of Window interface.  IIUC, |fetch|
must
> return a reject promise of TypeError because |x| is not of type Window.

I think a right solution would be to fix this in the V8 side. V8 is already
doing the holder type check (when we pass in an non-empty signature) and
throwing a type error if the type doesn't match. We should introduce a V8 API to
make V8 return a rejected promise instead of throwing a type error.

> 
> Instead of using finInstanceInPrototypeChain, I'd like to ask V8 team to pass
> the global object (instead of the global *proxy* object) as info.Holder().

FWIW, info.This() is pointing to the global object, isn't it?

[Yuki, 2016-10-21 10:28:58]

On 2016/10/21 10:24:49, haraken wrote:
> On 2016/10/21 10:11:19, Yuki wrote:
> >
>
https://codereview.chromium.org/2441593002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right):
> > 
> >
>
https://codereview.chromium.org/2441593002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:33:
> > info.Holder()->GetPrototype().As<v8::Object>();
> > On 2016/10/21 08:11:28, haraken wrote:
> > > 
> > > Can we remove the additional complexity by using
> > > V8XXX::findInstanceInPrototypeChain?
> > 
> > I tried it, and I found it didn't work as expected.
> > 
> > If we used it,
> >     x = {}
> >     x.__proto__ = window
> >     window.fetch.call(x, 'foo')
> > works, although |x| is not an instance of Window interface.  IIUC, |fetch|
> must
> > return a reject promise of TypeError because |x| is not of type Window.
> 
> I think a right solution would be to fix this in the V8 side. V8 is already
> doing the holder type check (when we pass in an non-empty signature) and
> throwing a type error if the type doesn't match. We should introduce a V8 API
to
> make V8 return a rejected promise instead of throwing a type error.

I'm fine with the idea.  In that case, we need to tell V8 that the function is
expected to always return a promise.


> > Instead of using finInstanceInPrototypeChain, I'd like to ask V8 team to
pass
> > the global object (instead of the global *proxy* object) as info.Holder().
> 
> FWIW, info.This() is pointing to the global object, isn't it?

I checked it.  info.This() == info.Holder().  They point to the same object.

[jochen (gone - plz use gerrit), 2016-10-21 11:07:23]

can you file a bug for v8 to be able to do this (and put it on go/v8-backlog)?

[Yuki, 2016-10-24 12:20:31]

On 2016/10/21 11:07:23, jochen wrote:
> can you file a bug for v8 to be able to do this (and put it on go/v8-backlog)?

Filed two issues:
https://bugs.chromium.org/p/chromium/issues/detail?id=658704
https://bugs.chromium.org/p/chromium/issues/detail?id=658706

Also added entries at go/v8-backlog.

[Yuki, 2016-10-25 09:44:22]

Nothing has changed, but could you take another look?

Is it okay to go with this for the time being?

[haraken, 2016-10-25 09:52:54]

On 2016/10/25 09:44:22, Yuki wrote:
> Nothing has changed, but could you take another look?
> 
> Is it okay to go with this for the time being?

Shall we chat with Toon offline? If it's really hard to fix it on the V8 side,
we can land this CL though...

[Yuki, 2016-10-25 09:59:24]

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

[Yuki, 2016-10-25 09:59:25]

+R=verwaest@

[jochen (gone - plz use gerrit), 2016-10-26 08:29:30]

(deferring to Kentaro and Toon)

[haraken, 2016-10-26 08:32:40]

On 2016/10/26 08:29:30, jochen wrote:
> (deferring to Kentaro and Toon)

Per the discussion offline, let's land this CL after making V8 forward
globalProxy->GetInternalField() to the global object.

[Yuki, 2016-11-09 13:34:59]

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

[commit-bot: I haz the power, 2016-11-09 13:35:10]

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, 2016-11-09 14:14:12]

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

[commit-bot: I haz the power, 2016-11-09 14:14:13]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Yuki, 2016-11-14 13:24:39]

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

[commit-bot: I haz the power, 2016-11-14 13:24:52]

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, 2016-11-14 14:06:06]

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

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

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

[Yuki, 2016-11-16 13:06:00]

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

[commit-bot: I haz the power, 2016-11-16 13:06:18]

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, 2016-11-16 14:29:55]

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

[commit-bot: I haz the power, 2016-11-16 14:29:56]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Yuki, 2016-11-16 15:16:16]

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

[commit-bot: I haz the power, 2016-11-16 15:17:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuki, 2016-11-16 15:17:30]

Could you take another look?

I've made necessary changes in V8 and Blink, so we no longer need a special hack
for global proxy objects.

[commit-bot: I haz the power, 2016-11-16 17:03:11]

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

[commit-bot: I haz the power, 2016-11-16 17:03:12]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-11-16 17:37:31]

LGTM

https://codereview.chromium.org/2441593002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2441593002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:399: void
installMethodInternal(

Not related to this CL, why do we (need to) have two installMethodInternal()s in
this file?

[Yuki, 2016-11-17 07:14:28]

https://codereview.chromium.org/2441593002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2441593002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:399: void
installMethodInternal(
On 2016/11/16 17:37:31, haraken wrote:
> 
> Not related to this CL, why do we (need to) have two installMethodInternal()s
in
> this file?

One for the interface template / prototype tempalte / instance template (shared
beyond v8::Context), and the other for the interface object / prototype object /
instance object (used only in a specific v8::Context).

The latter is used for runtime-enabled features.

[Yuki, 2016-11-17 07:14:44]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2016-11-17 07:15:09]

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, 2016-11-17 07:21:22]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-17 07:23:44]

Description was changed from

==========
binding: Returns a reject promise when |this| is not of the type.

Returns a reject promise when the receiver object is not of type
of the interface, instead of throwing a TypeError.

This CL disables the type check on V8 side and does the type check
on Blink side, then, if the receiver object is not of the type,
converts a TypeError to a reject promise and returns it.

BUG=627309
==========

to

==========
binding: Returns a reject promise when |this| is not of the type.

Returns a reject promise when the receiver object is not of type
of the interface, instead of throwing a TypeError.

This CL disables the type check on V8 side and does the type check
on Blink side, then, if the receiver object is not of the type,
converts a TypeError to a reject promise and returns it.

BUG=627309

Committed: https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa
Cr-Commit-Position: refs/heads/master@{#432798}
==========

[commit-bot: I haz the power, 2016-11-17 07:23:47]

Patchset 7 (id:??) landed as
https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa
Cr-Commit-Position: refs/heads/master@{#432798}