Implement cross-origin attributes using access check interceptors.

Implement cross-origin attributes using access check interceptors.

BUG=618305
R=haraken@chromium.org, jochen@chromium.org, yukishiino@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/3387c1777b355f2da7a02ad1fc833745f0589915

[dcheng, 2016-10-24 07:31:07]

Description was changed from

==========
Implement Window's indexed getter using failed access interceptors.

BUG=618305
==========

to

==========
Implement cross-origin attributes using access check interceptors.

BUG=618305
==========

[dcheng, 2016-10-24 07:56:37]

dcheng@chromium.org changed reviewers:
+ haraken@chromium.org, jochen@chromium.org, yukishiino@chromium.org

[dcheng, 2016-10-24 07:56:38]

This CL isn't complete, but I wanted to get some general thoughts on the
direction of this CL.

Some open questions:

1) Handling methods: I'm planning on using FunctionTemplate::NewRemoteInstance()
for postMessage(), etc. However, it's not clear to me where the new instance
should be cached.
 - Originally, I thought about saving it on the This() object using SetPrivate /
GetPrivate. However, I'm not sure if this will work with the access checks. I
also noticed something interesting: window.postMessage ===
window[0].postMessage, even if window[0] is cross-origin.
 - Because of this, I thought about caching one static local instantiation and
reusing it for all cross-origin accesses.
 - However, it looks like the actual behavior is that each context has its own
copy of postMessage? If I set window.postMessage.hello = 'world',
window[0].postMessage.hello is also set. However, from inside window[0], it does
not see any of the attributes (which makes sense; otherwise, this would be a
cross-origin info leak). So what is the best way to represent this?

2) I hope to reuse the same ObjectTemplate to back remote contexts as well,
though I'm not sure if that's the ideal approach: does it matter that the
ObjectTemplate will have a bunch of properties configured that are never used?
If we care, it's not too hard to also generate a remote object template that
only contains cross-origin things. That will happen in a followup though.

3) After I migrated to the bindings generator, things are a bit broken: we're
not setting the internal field on the V8 object that points back to the C++
impl:

ASSERTION FAILED: offset < wrapper->InternalFieldCount()
../../third_party/WebKit/Source/bindings/core/v8/WrapperTypeInfo.h(221) : T
*blink::getInternalField(v8::Local<v8::Object>) [T = blink::ScriptWrappable,
offset = 1]
#0 0x7f50a446e9de base::debug::StackTrace::StackTrace()
#1 0x7f50a446e51f base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f50a4bef330 <unknown>
#3 0x7f509f92d035 blink::getInternalField<>()
#4 0x7f509f92cfbd blink::toScriptWrappable()
#5 0x7f509f92828d blink::V8CSSStyleDeclaration::toImpl()
#6 0x7f50a10c6a0f blink::DOMWindowV8Internal::lengthAttributeGetter()
#7 0x7f50a10eb829 blink::DOMWindowV8Internal::crossOriginNamedGetter()
#8 0x7f50a3b6365c v8::internal::PropertyCallbackArguments::Call()
#9 0x7f50a3bff351 v8::internal::(anonymous
namespace)::GetPropertyWithInterceptorInternal()
#10 0x7f50a3bfc139 v8::internal::JSObject::GetPropertyWithFailedAccessCheck()
#11 0x7f50a3bfbc43 v8::internal::Object::GetProperty()
#12 0x7f50a3b4ef24 v8::internal::LoadIC::Load()
#13 0x7f50a3b58e7c v8::internal::__RT_impl_Runtime_LoadIC_Miss()
#14 0x7f50a3b58a00 v8::internal::Runtime_LoadIC_Miss()

I'm currently tracing through to figure out why it's not getting set here, and
will hopefully have a fix for that soon.

https://codereview.chromium.org/2439013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt (right):

https://codereview.chromium.org/2439013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt:44:
CrossOrigin=|Setter
I'm uncertain about adding a new attribute. For now, it makes migrating code
from my handrolled manual bindings into the bindings generator easier. For the
final patchset to be reviewed, we definitely don't want both. We should either:
- Remove DoNotCheckSecurity
- Reuse it, but update the usage in the scripts as needed.

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

https://codereview.chromium.org/2439013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:215: //
TODO(dcheng): Can we / should we use AtomicString here? That means using
DEFINE_STATIC_LOCAL here.
This is a easy change, but there's no precedent for it in the bindings either.
Maybe it's worth it here, though all the strings are also quite short...

(I will try benchmarking this)

https://codereview.chromium.org/2439013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:223: {% for
world_suffix in attribute.world_suffixes %}
There's a lot of different variations here. I think for cross-origin / do not
check security attributes, it would be best to just handle the cases we need
rather than trying to handle everything. WDYT?

[dcheng, 2016-10-24 08:12:49]

On 2016/10/24 07:56:38, dcheng wrote:
> 3) After I migrated to the bindings generator, things are a bit broken: we're
> not setting the internal field on the V8 object that points back to the C++
> impl:
> 
> ASSERTION FAILED: offset < wrapper->InternalFieldCount()
> ../../third_party/WebKit/Source/bindings/core/v8/WrapperTypeInfo.h(221) : T
> *blink::getInternalField(v8::Local<v8::Object>) [T = blink::ScriptWrappable,
> offset = 1]
> #0 0x7f50a446e9de base::debug::StackTrace::StackTrace()
> #1 0x7f50a446e51f base::debug::(anonymous namespace)::StackDumpSignalHandler()
> #2 0x7f50a4bef330 <unknown>
> #3 0x7f509f92d035 blink::getInternalField<>()
> #4 0x7f509f92cfbd blink::toScriptWrappable()
> #5 0x7f509f92828d blink::V8CSSStyleDeclaration::toImpl()
> #6 0x7f50a10c6a0f blink::DOMWindowV8Internal::lengthAttributeGetter()
> #7 0x7f50a10eb829 blink::DOMWindowV8Internal::crossOriginNamedGetter()
> #8 0x7f50a3b6365c v8::internal::PropertyCallbackArguments::Call()
> #9 0x7f50a3bff351 v8::internal::(anonymous
> namespace)::GetPropertyWithInterceptorInternal()
> #10 0x7f50a3bfc139 v8::internal::JSObject::GetPropertyWithFailedAccessCheck()
> #11 0x7f50a3bfbc43 v8::internal::Object::GetProperty()
> #12 0x7f50a3b4ef24 v8::internal::LoadIC::Load()
> #13 0x7f50a3b58e7c v8::internal::__RT_impl_Runtime_LoadIC_Miss()
> #14 0x7f50a3b58a00 v8::internal::Runtime_LoadIC_Miss()
> 
> I'm currently tracing through to figure out why it's not getting set here, and
> will hopefully have a fix for that soon.

Btw, I think this is because it's getting called with info.Holder() ==
context.Global(). Since we don't save the pointer to the DOMWindow* on
context->Global(), the toImpl() call doesn't work.

I need to think about the best way to solve this: it's currently not easy to
plumb it in as the |data| value to SetAccessCheckCallbackAndHandler, and it's
also not easy to allow setNativeInfo() call on context->Global() (and we
probably don't want to do that anyway).

[haraken, 2016-10-24 09:18:16]

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

[haraken, 2016-10-24 09:18:16]

On 2016/10/24 07:56:38, dcheng wrote:
> This CL isn't complete, but I wanted to get some general thoughts on the
> direction of this CL.
> 
> Some open questions:
> 
> 1) Handling methods: I'm planning on using
FunctionTemplate::NewRemoteInstance()
> for postMessage(), etc. However, it's not clear to me where the new instance
> should be cached.
>  - Originally, I thought about saving it on the This() object using SetPrivate
/
> GetPrivate. However, I'm not sure if this will work with the access checks. I
> also noticed something interesting: window.postMessage ===
> window[0].postMessage, even if window[0] is cross-origin.
>  - Because of this, I thought about caching one static local instantiation and
> reusing it for all cross-origin accesses.
>  - However, it looks like the actual behavior is that each context has its own
> copy of postMessage? If I set window.postMessage.hello = 'world',
> window[0].postMessage.hello is also set. However, from inside window[0], it
does
> not see any of the attributes (which makes sense; otherwise, this would be a
> cross-origin info leak). So what is the best way to represent this?

jochen@ or verwaest@: Any idea on this?
 
> 2) I hope to reuse the same ObjectTemplate to back remote contexts as well,
> though I'm not sure if that's the ideal approach: does it matter that the
> ObjectTemplate will have a bunch of properties configured that are never used?
> If we care, it's not too hard to also generate a remote object template that
> only contains cross-origin things. That will happen in a followup though.

I'm a bit confused. If you create an object from an ObjectTemplate that has more
attributes than needed, won't you end up with creating an object that has more
attributes than needed? (e.g., it will affect whether hasOwnProperty(object,
"attr") returns true or false.)

[haraken, 2016-10-24 09:22:23]

On 2016/10/24 08:12:49, dcheng wrote:
> On 2016/10/24 07:56:38, dcheng wrote:
> > 3) After I migrated to the bindings generator, things are a bit broken:
we're
> > not setting the internal field on the V8 object that points back to the C++
> > impl:
> > 
> > ASSERTION FAILED: offset < wrapper->InternalFieldCount()
> > ../../third_party/WebKit/Source/bindings/core/v8/WrapperTypeInfo.h(221) : T
> > *blink::getInternalField(v8::Local<v8::Object>) [T = blink::ScriptWrappable,
> > offset = 1]
> > #0 0x7f50a446e9de base::debug::StackTrace::StackTrace()
> > #1 0x7f50a446e51f base::debug::(anonymous
namespace)::StackDumpSignalHandler()
> > #2 0x7f50a4bef330 <unknown>
> > #3 0x7f509f92d035 blink::getInternalField<>()
> > #4 0x7f509f92cfbd blink::toScriptWrappable()
> > #5 0x7f509f92828d blink::V8CSSStyleDeclaration::toImpl()
> > #6 0x7f50a10c6a0f blink::DOMWindowV8Internal::lengthAttributeGetter()
> > #7 0x7f50a10eb829 blink::DOMWindowV8Internal::crossOriginNamedGetter()
> > #8 0x7f50a3b6365c v8::internal::PropertyCallbackArguments::Call()
> > #9 0x7f50a3bff351 v8::internal::(anonymous
> > namespace)::GetPropertyWithInterceptorInternal()
> > #10 0x7f50a3bfc139
v8::internal::JSObject::GetPropertyWithFailedAccessCheck()
> > #11 0x7f50a3bfbc43 v8::internal::Object::GetProperty()
> > #12 0x7f50a3b4ef24 v8::internal::LoadIC::Load()
> > #13 0x7f50a3b58e7c v8::internal::__RT_impl_Runtime_LoadIC_Miss()
> > #14 0x7f50a3b58a00 v8::internal::Runtime_LoadIC_Miss()
> > 
> > I'm currently tracing through to figure out why it's not getting set here,
and
> > will hopefully have a fix for that soon.
> 
> Btw, I think this is because it's getting called with info.Holder() ==
> context.Global(). Since we don't save the pointer to the DOMWindow* on
> context->Global(), the toImpl() call doesn't work.
> 
> I need to think about the best way to solve this: it's currently not easy to
> plumb it in as the |data| value to SetAccessCheckCallbackAndHandler, and it's
> also not easy to allow setNativeInfo() call on context->Global() (and we
> probably don't want to do that anyway).

Just to confirm:

The prototype chain looks like this:

  global proxy object --(prototype)--> Window's wrapper --(prototype)-->
Window.prototype --...

context.Global() is pointing to the global proxy object. The pointer to
DOMWindow* is set on Window's wrapper but not set on the global proxy object.
Hence you're crashing. Am I understanding correctly?

Can we tweak DOMWindow::toImpl() so that it returns a DOMWindow* object even
when we pass in the global proxy object?

[Yuki, 2016-10-24 09:50:38]

On 2016/10/24 09:22:23, haraken wrote:
> On 2016/10/24 08:12:49, dcheng wrote:
> > On 2016/10/24 07:56:38, dcheng wrote:
> > > 3) After I migrated to the bindings generator, things are a bit broken:
> we're
> > > not setting the internal field on the V8 object that points back to the
C++
> > > impl:
> > > 
> > > ASSERTION FAILED: offset < wrapper->InternalFieldCount()
> > > ../../third_party/WebKit/Source/bindings/core/v8/WrapperTypeInfo.h(221) :
T
> > > *blink::getInternalField(v8::Local<v8::Object>) [T =
blink::ScriptWrappable,
> > > offset = 1]
> > > #0 0x7f50a446e9de base::debug::StackTrace::StackTrace()
> > > #1 0x7f50a446e51f base::debug::(anonymous
> namespace)::StackDumpSignalHandler()
> > > #2 0x7f50a4bef330 <unknown>
> > > #3 0x7f509f92d035 blink::getInternalField<>()
> > > #4 0x7f509f92cfbd blink::toScriptWrappable()
> > > #5 0x7f509f92828d blink::V8CSSStyleDeclaration::toImpl()
> > > #6 0x7f50a10c6a0f blink::DOMWindowV8Internal::lengthAttributeGetter()
> > > #7 0x7f50a10eb829 blink::DOMWindowV8Internal::crossOriginNamedGetter()
> > > #8 0x7f50a3b6365c v8::internal::PropertyCallbackArguments::Call()
> > > #9 0x7f50a3bff351 v8::internal::(anonymous
> > > namespace)::GetPropertyWithInterceptorInternal()
> > > #10 0x7f50a3bfc139
> v8::internal::JSObject::GetPropertyWithFailedAccessCheck()
> > > #11 0x7f50a3bfbc43 v8::internal::Object::GetProperty()
> > > #12 0x7f50a3b4ef24 v8::internal::LoadIC::Load()
> > > #13 0x7f50a3b58e7c v8::internal::__RT_impl_Runtime_LoadIC_Miss()
> > > #14 0x7f50a3b58a00 v8::internal::Runtime_LoadIC_Miss()
> > > 
> > > I'm currently tracing through to figure out why it's not getting set here,
> and
> > > will hopefully have a fix for that soon.
> > 
> > Btw, I think this is because it's getting called with info.Holder() ==
> > context.Global(). Since we don't save the pointer to the DOMWindow* on
> > context->Global(), the toImpl() call doesn't work.
> > 
> > I need to think about the best way to solve this: it's currently not easy to
> > plumb it in as the |data| value to SetAccessCheckCallbackAndHandler, and
it's
> > also not easy to allow setNativeInfo() call on context->Global() (and we
> > probably don't want to do that anyway).
> 
> Just to confirm:
> 
> The prototype chain looks like this:
> 
>   global proxy object --(prototype)--> Window's wrapper --(prototype)-->
> Window.prototype --...
> 
> context.Global() is pointing to the global proxy object. The pointer to
> DOMWindow* is set on Window's wrapper but not set on the global proxy object.
> Hence you're crashing. Am I understanding correctly?
> 
> Can we tweak DOMWindow::toImpl() so that it returns a DOMWindow* object even
> when we pass in the global proxy object?

We'd like to follow the prototype chain iff it's the hidden prototype chain from
the global proxy object to the global object.  But there seems not way to
determine if it's a hidden prototype chain or not.  I think we need help from
V8.  I'm hitting the same issue at https://codereview.chromium.org/2441593002/ ,
I worked around though.

[haraken, 2016-10-24 09:54:14]

On 2016/10/24 09:50:38, Yuki wrote:
> On 2016/10/24 09:22:23, haraken wrote:
> > On 2016/10/24 08:12:49, dcheng wrote:
> > > On 2016/10/24 07:56:38, dcheng wrote:
> > > > 3) After I migrated to the bindings generator, things are a bit broken:
> > we're
> > > > not setting the internal field on the V8 object that points back to the
> C++
> > > > impl:
> > > > 
> > > > ASSERTION FAILED: offset < wrapper->InternalFieldCount()
> > > > ../../third_party/WebKit/Source/bindings/core/v8/WrapperTypeInfo.h(221)
:
> T
> > > > *blink::getInternalField(v8::Local<v8::Object>) [T =
> blink::ScriptWrappable,
> > > > offset = 1]
> > > > #0 0x7f50a446e9de base::debug::StackTrace::StackTrace()
> > > > #1 0x7f50a446e51f base::debug::(anonymous
> > namespace)::StackDumpSignalHandler()
> > > > #2 0x7f50a4bef330 <unknown>
> > > > #3 0x7f509f92d035 blink::getInternalField<>()
> > > > #4 0x7f509f92cfbd blink::toScriptWrappable()
> > > > #5 0x7f509f92828d blink::V8CSSStyleDeclaration::toImpl()
> > > > #6 0x7f50a10c6a0f blink::DOMWindowV8Internal::lengthAttributeGetter()
> > > > #7 0x7f50a10eb829 blink::DOMWindowV8Internal::crossOriginNamedGetter()
> > > > #8 0x7f50a3b6365c v8::internal::PropertyCallbackArguments::Call()
> > > > #9 0x7f50a3bff351 v8::internal::(anonymous
> > > > namespace)::GetPropertyWithInterceptorInternal()
> > > > #10 0x7f50a3bfc139
> > v8::internal::JSObject::GetPropertyWithFailedAccessCheck()
> > > > #11 0x7f50a3bfbc43 v8::internal::Object::GetProperty()
> > > > #12 0x7f50a3b4ef24 v8::internal::LoadIC::Load()
> > > > #13 0x7f50a3b58e7c v8::internal::__RT_impl_Runtime_LoadIC_Miss()
> > > > #14 0x7f50a3b58a00 v8::internal::Runtime_LoadIC_Miss()
> > > > 
> > > > I'm currently tracing through to figure out why it's not getting set
here,
> > and
> > > > will hopefully have a fix for that soon.
> > > 
> > > Btw, I think this is because it's getting called with info.Holder() ==
> > > context.Global(). Since we don't save the pointer to the DOMWindow* on
> > > context->Global(), the toImpl() call doesn't work.
> > > 
> > > I need to think about the best way to solve this: it's currently not easy
to
> > > plumb it in as the |data| value to SetAccessCheckCallbackAndHandler, and
> it's
> > > also not easy to allow setNativeInfo() call on context->Global() (and we
> > > probably don't want to do that anyway).
> > 
> > Just to confirm:
> > 
> > The prototype chain looks like this:
> > 
> >   global proxy object --(prototype)--> Window's wrapper --(prototype)-->
> > Window.prototype --...
> > 
> > context.Global() is pointing to the global proxy object. The pointer to
> > DOMWindow* is set on Window's wrapper but not set on the global proxy
object.
> > Hence you're crashing. Am I understanding correctly?
> > 
> > Can we tweak DOMWindow::toImpl() so that it returns a DOMWindow* object even
> > when we pass in the global proxy object?
> 
> We'd like to follow the prototype chain iff it's the hidden prototype chain
from
> the global proxy object to the global object.  But there seems not way to
> determine if it's a hidden prototype chain or not.  I think we need help from
> V8.  I'm hitting the same issue at https://codereview.chromium.org/2441593002/
,
> I worked around though.

Maybe we should have a way on the global proxy object to indicate that that is
the global proxy object. For example, we can add the DOMWindow* pointer on the
global proxy object. (Actually I remember we had that pointer in the good old
WebKit days.)

[Yuki, 2016-10-24 09:57:36]

On 2016/10/24 09:54:14, haraken wrote:
> On 2016/10/24 09:50:38, Yuki wrote:
> > On 2016/10/24 09:22:23, haraken wrote:
> > > On 2016/10/24 08:12:49, dcheng wrote:
> > > > On 2016/10/24 07:56:38, dcheng wrote:
> > > > > 3) After I migrated to the bindings generator, things are a bit
broken:
> > > we're
> > > > > not setting the internal field on the V8 object that points back to
the
> > C++
> > > > > impl:
> > > > > 
> > > > > ASSERTION FAILED: offset < wrapper->InternalFieldCount()
> > > > >
../../third_party/WebKit/Source/bindings/core/v8/WrapperTypeInfo.h(221)
> :
> > T
> > > > > *blink::getInternalField(v8::Local<v8::Object>) [T =
> > blink::ScriptWrappable,
> > > > > offset = 1]
> > > > > #0 0x7f50a446e9de base::debug::StackTrace::StackTrace()
> > > > > #1 0x7f50a446e51f base::debug::(anonymous
> > > namespace)::StackDumpSignalHandler()
> > > > > #2 0x7f50a4bef330 <unknown>
> > > > > #3 0x7f509f92d035 blink::getInternalField<>()
> > > > > #4 0x7f509f92cfbd blink::toScriptWrappable()
> > > > > #5 0x7f509f92828d blink::V8CSSStyleDeclaration::toImpl()
> > > > > #6 0x7f50a10c6a0f blink::DOMWindowV8Internal::lengthAttributeGetter()
> > > > > #7 0x7f50a10eb829 blink::DOMWindowV8Internal::crossOriginNamedGetter()
> > > > > #8 0x7f50a3b6365c v8::internal::PropertyCallbackArguments::Call()
> > > > > #9 0x7f50a3bff351 v8::internal::(anonymous
> > > > > namespace)::GetPropertyWithInterceptorInternal()
> > > > > #10 0x7f50a3bfc139
> > > v8::internal::JSObject::GetPropertyWithFailedAccessCheck()
> > > > > #11 0x7f50a3bfbc43 v8::internal::Object::GetProperty()
> > > > > #12 0x7f50a3b4ef24 v8::internal::LoadIC::Load()
> > > > > #13 0x7f50a3b58e7c v8::internal::__RT_impl_Runtime_LoadIC_Miss()
> > > > > #14 0x7f50a3b58a00 v8::internal::Runtime_LoadIC_Miss()
> > > > > 
> > > > > I'm currently tracing through to figure out why it's not getting set
> here,
> > > and
> > > > > will hopefully have a fix for that soon.
> > > > 
> > > > Btw, I think this is because it's getting called with info.Holder() ==
> > > > context.Global(). Since we don't save the pointer to the DOMWindow* on
> > > > context->Global(), the toImpl() call doesn't work.
> > > > 
> > > > I need to think about the best way to solve this: it's currently not
easy
> to
> > > > plumb it in as the |data| value to SetAccessCheckCallbackAndHandler, and
> > it's
> > > > also not easy to allow setNativeInfo() call on context->Global() (and we
> > > > probably don't want to do that anyway).
> > > 
> > > Just to confirm:
> > > 
> > > The prototype chain looks like this:
> > > 
> > >   global proxy object --(prototype)--> Window's wrapper --(prototype)-->
> > > Window.prototype --...
> > > 
> > > context.Global() is pointing to the global proxy object. The pointer to
> > > DOMWindow* is set on Window's wrapper but not set on the global proxy
> object.
> > > Hence you're crashing. Am I understanding correctly?
> > > 
> > > Can we tweak DOMWindow::toImpl() so that it returns a DOMWindow* object
even
> > > when we pass in the global proxy object?
> > 
> > We'd like to follow the prototype chain iff it's the hidden prototype chain
> from
> > the global proxy object to the global object.  But there seems not way to
> > determine if it's a hidden prototype chain or not.  I think we need help
from
> > V8.  I'm hitting the same issue at
https://codereview.chromium.org/2441593002/
> ,
> > I worked around though.
> 
> Maybe we should have a way on the global proxy object to indicate that that is
> the global proxy object. For example, we can add the DOMWindow* pointer on the
> global proxy object. (Actually I remember we had that pointer in the good old
> WebKit days.)

Rather than adding the DOMWindow* pointer on the global proxy object, I think
the global proxy object's GetInternalField() should proxy to the global object.

[jochen (gone - plz use gerrit), 2016-10-24 11:16:33]

On 2016/10/24 at 09:18:16, haraken wrote:
> On 2016/10/24 07:56:38, dcheng wrote:
> > This CL isn't complete, but I wanted to get some general thoughts on the
> > direction of this CL.
> > 
> > Some open questions:
> > 
> > 1) Handling methods: I'm planning on using
FunctionTemplate::NewRemoteInstance()
> > for postMessage(), etc. However, it's not clear to me where the new instance
> > should be cached.
> >  - Originally, I thought about saving it on the This() object using
SetPrivate /
> > GetPrivate. However, I'm not sure if this will work with the access checks.
I
> > also noticed something interesting: window.postMessage ===
> > window[0].postMessage, even if window[0] is cross-origin.
> >  - Because of this, I thought about caching one static local instantiation
and
> > reusing it for all cross-origin accesses.
> >  - However, it looks like the actual behavior is that each context has its
own
> > copy of postMessage? If I set window.postMessage.hello = 'world',
> > window[0].postMessage.hello is also set. However, from inside window[0], it
does
> > not see any of the attributes (which makes sense; otherwise, this would be a
> > cross-origin info leak). So what is the best way to represent this?
> 
> jochen@ or verwaest@: Any idea on this?

The new spec says that if you get a function from a cross origin object, it'll
magically be from your context. I'm not sure whether this has to be always the
same getter, however, so we could just not cache it at all?

That also means that you don't need a remote instance, but you just create a
regular instance in the caller's context.

>  
> > 2) I hope to reuse the same ObjectTemplate to back remote contexts as well,
> > though I'm not sure if that's the ideal approach: does it matter that the
> > ObjectTemplate will have a bunch of properties configured that are never
used?
> > If we care, it's not too hard to also generate a remote object template that
> > only contains cross-origin things. That will happen in a followup though.
> 
> I'm a bit confused. If you create an object from an ObjectTemplate that has
more attributes than needed, won't you end up with creating an object that has
more attributes than needed? (e.g., it will affect whether
hasOwnProperty(object, "attr") returns true or false.)

Ideally, the template used for local and remote frames is the same.

[dcheng, 2016-10-24 15:25:41]

> > > > Just to confirm:
> > > > 
> > > > The prototype chain looks like this:
> > > > 
> > > >   global proxy object --(prototype)--> Window's wrapper --(prototype)-->
> > > > Window.prototype --...
> > > > 
> > > > context.Global() is pointing to the global proxy object. The pointer to
> > > > DOMWindow* is set on Window's wrapper but not set on the global proxy
> > object.
> > > > Hence you're crashing. Am I understanding correctly?
> > > > 
> > > > Can we tweak DOMWindow::toImpl() so that it returns a DOMWindow* object
> even
> > > > when we pass in the global proxy object?
> > > 
> > > We'd like to follow the prototype chain iff it's the hidden prototype
chain
> > from
> > > the global proxy object to the global object.  But there seems not way to
> > > determine if it's a hidden prototype chain or not.  I think we need help
> from
> > > V8.  I'm hitting the same issue at
> https://codereview.chromium.org/2441593002/
> > ,
> > > I worked around though.
> > 
> > Maybe we should have a way on the global proxy object to indicate that that
is
> > the global proxy object. For example, we can add the DOMWindow* pointer on
the
> > global proxy object. (Actually I remember we had that pointer in the good
old
> > WebKit days.)

This was the first thing I tried, but it doesn't work, because the global proxy
doesn't have the right number of internal fields.

> 
> Rather than adding the DOMWindow* pointer on the global proxy object, I think
> the global proxy object's GetInternalField() should proxy to the global
object.

This makes sense, but I'm not sure how hard it will be. Let me poke at this
today...

> The new spec says that if you get a function from a cross origin object, it'll
> magically be from your context. I'm not sure whether this has to be always the
> same getter, however, so we could just not cache it at all?
> 
> That also means that you don't need a remote instance, but you just create a
> regular instance in the caller's context.
> 

I see. I wonder how this works today? I guess it is shared today because the
function object lives in the prototype object? I wonder if I can grab it from
there...

> > > only contains cross-origin things. That will happen in a followup though.
> > 
> > I'm a bit confused. If you create an object from an ObjectTemplate that has
> more attributes than needed, won't you end up with creating an object that has
> more attributes than needed? (e.g., it will affect whether
> hasOwnProperty(object, "attr") returns true or false.)
> 
> Ideally, the template used for local and remote frames is the same.

Yes, it will have more attributes than needed. However, this shouldn't be
observable for the remote frame case.

[haraken, 2016-10-24 18:12:26]

On 2016/10/24 15:25:41, dcheng wrote:
> > > > > Just to confirm:
> > > > > 
> > > > > The prototype chain looks like this:
> > > > > 
> > > > >   global proxy object --(prototype)--> Window's wrapper
--(prototype)-->
> > > > > Window.prototype --...
> > > > > 
> > > > > context.Global() is pointing to the global proxy object. The pointer
to
> > > > > DOMWindow* is set on Window's wrapper but not set on the global proxy
> > > object.
> > > > > Hence you're crashing. Am I understanding correctly?
> > > > > 
> > > > > Can we tweak DOMWindow::toImpl() so that it returns a DOMWindow*
object
> > even
> > > > > when we pass in the global proxy object?
> > > > 
> > > > We'd like to follow the prototype chain iff it's the hidden prototype
> chain
> > > from
> > > > the global proxy object to the global object.  But there seems not way
to
> > > > determine if it's a hidden prototype chain or not.  I think we need help
> > from
> > > > V8.  I'm hitting the same issue at
> > https://codereview.chromium.org/2441593002/
> > > ,
> > > > I worked around though.
> > > 
> > > Maybe we should have a way on the global proxy object to indicate that
that
> is
> > > the global proxy object. For example, we can add the DOMWindow* pointer on
> the
> > > global proxy object. (Actually I remember we had that pointer in the good
> old
> > > WebKit days.)
> 
> This was the first thing I tried, but it doesn't work, because the global
proxy
> doesn't have the right number of internal fields.

I think that the number of internal fields is controlled by the binding side via
v8::ObjectTemplate::SetInternalFieldCount(). Maybe can you just tweak the number
passed in the API?

> 
> > 
> > Rather than adding the DOMWindow* pointer on the global proxy object, I
think
> > the global proxy object's GetInternalField() should proxy to the global
> object.
> 
> This makes sense, but I'm not sure how hard it will be. Let me poke at this
> today...
> 
> > The new spec says that if you get a function from a cross origin object,
it'll
> > magically be from your context. I'm not sure whether this has to be always
the
> > same getter, however, so we could just not cache it at all?
> > 
> > That also means that you don't need a remote instance, but you just create a
> > regular instance in the caller's context.
> > 
> 
> I see. I wonder how this works today? I guess it is shared today because the
> function object lives in the prototype object? I wonder if I can grab it from
> there...
> 
> > > > only contains cross-origin things. That will happen in a followup
though.
> > > 
> > > I'm a bit confused. If you create an object from an ObjectTemplate that
has
> > more attributes than needed, won't you end up with creating an object that
has
> > more attributes than needed? (e.g., it will affect whether
> > hasOwnProperty(object, "attr") returns true or false.)
> > 
> > Ideally, the template used for local and remote frames is the same.
> 
> Yes, it will have more attributes than needed. However, this shouldn't be
> observable for the remote frame case.

I'm just curious but how is the visibility of remote frame's attributes
controlled?

[dcheng, 2016-10-24 18:15:32]

On 2016/10/24 18:12:26, haraken wrote:
> On 2016/10/24 15:25:41, dcheng wrote:
> > > > > > Just to confirm:
> > > > > > 
> > > > > > The prototype chain looks like this:
> > > > > > 
> > > > > >   global proxy object --(prototype)--> Window's wrapper
> --(prototype)-->
> > > > > > Window.prototype --...
> > > > > > 
> > > > > > context.Global() is pointing to the global proxy object. The pointer
> to
> > > > > > DOMWindow* is set on Window's wrapper but not set on the global
proxy
> > > > object.
> > > > > > Hence you're crashing. Am I understanding correctly?
> > > > > > 
> > > > > > Can we tweak DOMWindow::toImpl() so that it returns a DOMWindow*
> object
> > > even
> > > > > > when we pass in the global proxy object?
> > > > > 
> > > > > We'd like to follow the prototype chain iff it's the hidden prototype
> > chain
> > > > from
> > > > > the global proxy object to the global object.  But there seems not way
> to
> > > > > determine if it's a hidden prototype chain or not.  I think we need
help
> > > from
> > > > > V8.  I'm hitting the same issue at
> > > https://codereview.chromium.org/2441593002/
> > > > ,
> > > > > I worked around though.
> > > > 
> > > > Maybe we should have a way on the global proxy object to indicate that
> that
> > is
> > > > the global proxy object. For example, we can add the DOMWindow* pointer
on
> > the
> > > > global proxy object. (Actually I remember we had that pointer in the
good
> > old
> > > > WebKit days.)
> > 
> > This was the first thing I tried, but it doesn't work, because the global
> proxy
> > doesn't have the right number of internal fields.
> 
> I think that the number of internal fields is controlled by the binding side
via
> v8::ObjectTemplate::SetInternalFieldCount(). Maybe can you just tweak the
number
> passed in the API?

I think we already set this, but I think v8 is only using the ObjectTemplate for
configuring the global object, not the global proxy.

> 
> > 
> > > 
> > > Rather than adding the DOMWindow* pointer on the global proxy object, I
> think
> > > the global proxy object's GetInternalField() should proxy to the global
> > object.
> > 
> > This makes sense, but I'm not sure how hard it will be. Let me poke at this
> > today...
> > 
> > > The new spec says that if you get a function from a cross origin object,
> it'll
> > > magically be from your context. I'm not sure whether this has to be always
> the
> > > same getter, however, so we could just not cache it at all?
> > > 
> > > That also means that you don't need a remote instance, but you just create
a
> > > regular instance in the caller's context.
> > > 
> > 
> > I see. I wonder how this works today? I guess it is shared today because the
> > function object lives in the prototype object? I wonder if I can grab it
from
> > there...
> > 
> > > > > only contains cross-origin things. That will happen in a followup
> though.
> > > > 
> > > > I'm a bit confused. If you create an object from an ObjectTemplate that
> has
> > > more attributes than needed, won't you end up with creating an object that
> has
> > > more attributes than needed? (e.g., it will affect whether
> > > hasOwnProperty(object, "attr") returns true or false.)
> > > 
> > > Ideally, the template used for local and remote frames is the same.
> > 
> > Yes, it will have more attributes than needed. However, this shouldn't be
> > observable for the remote frame case.
> 
> I'm just curious but how is the visibility of remote frame's attributes
> controlled?

It's controlled by the standard securityCheck: it's just that for a remote
frame, this will always return false, so the attributes won't be visible. Thus,
all attribute access gets redirected through the cross-origin interceptors.

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

On 2016/10/24 18:15:32, dcheng wrote:
> On 2016/10/24 18:12:26, haraken wrote:
> > On 2016/10/24 15:25:41, dcheng wrote:
> > > > > > > Just to confirm:
> > > > > > > 
> > > > > > > The prototype chain looks like this:
> > > > > > > 
> > > > > > >   global proxy object --(prototype)--> Window's wrapper
> > --(prototype)-->
> > > > > > > Window.prototype --...
> > > > > > > 
> > > > > > > context.Global() is pointing to the global proxy object. The
pointer
> > to
> > > > > > > DOMWindow* is set on Window's wrapper but not set on the global
> proxy
> > > > > object.
> > > > > > > Hence you're crashing. Am I understanding correctly?
> > > > > > > 
> > > > > > > Can we tweak DOMWindow::toImpl() so that it returns a DOMWindow*
> > object
> > > > even
> > > > > > > when we pass in the global proxy object?
> > > > > > 
> > > > > > We'd like to follow the prototype chain iff it's the hidden
prototype
> > > chain
> > > > > from
> > > > > > the global proxy object to the global object.  But there seems not
way
> > to
> > > > > > determine if it's a hidden prototype chain or not.  I think we need
> help
> > > > from
> > > > > > V8.  I'm hitting the same issue at
> > > > https://codereview.chromium.org/2441593002/
> > > > > ,
> > > > > > I worked around though.
> > > > > 
> > > > > Maybe we should have a way on the global proxy object to indicate that
> > that
> > > is
> > > > > the global proxy object. For example, we can add the DOMWindow*
pointer
> on
> > > the
> > > > > global proxy object. (Actually I remember we had that pointer in the
> good
> > > old
> > > > > WebKit days.)
> > > 
> > > This was the first thing I tried, but it doesn't work, because the global
> > proxy
> > > doesn't have the right number of internal fields.
> > 
> > I think that the number of internal fields is controlled by the binding side
> via
> > v8::ObjectTemplate::SetInternalFieldCount(). Maybe can you just tweak the
> number
> > passed in the API?
> 
> I think we already set this, but I think v8 is only using the ObjectTemplate
for
> configuring the global object, not the global proxy.
> 
> > 
> > > 
> > > > 
> > > > Rather than adding the DOMWindow* pointer on the global proxy object, I
> > think
> > > > the global proxy object's GetInternalField() should proxy to the global
> > > object.
> > > 
> > > This makes sense, but I'm not sure how hard it will be. Let me poke at
this
> > > today...
> > > 
> > > > The new spec says that if you get a function from a cross origin object,
> > it'll
> > > > magically be from your context. I'm not sure whether this has to be
always
> > the
> > > > same getter, however, so we could just not cache it at all?
> > > > 
> > > > That also means that you don't need a remote instance, but you just
create
> a
> > > > regular instance in the caller's context.
> > > > 
> > > 
> > > I see. I wonder how this works today? I guess it is shared today because
the
> > > function object lives in the prototype object? I wonder if I can grab it
> from
> > > there...
> > > 
> > > > > > only contains cross-origin things. That will happen in a followup
> > though.
> > > > > 
> > > > > I'm a bit confused. If you create an object from an ObjectTemplate
that
> > has
> > > > more attributes than needed, won't you end up with creating an object
that
> > has
> > > > more attributes than needed? (e.g., it will affect whether
> > > > hasOwnProperty(object, "attr") returns true or false.)
> > > > 
> > > > Ideally, the template used for local and remote frames is the same.
> > > 
> > > Yes, it will have more attributes than needed. However, this shouldn't be
> > > observable for the remote frame case.
> > 
> > I'm just curious but how is the visibility of remote frame's attributes
> > controlled?
> 
> It's controlled by the standard securityCheck: it's just that for a remote
> frame, this will always return false, so the attributes won't be visible.
Thus,
> all attribute access gets redirected through the cross-origin interceptors.

Thanks, then as jochen mentioned, it makes sense to use the same ObjectTemplate
for local and remote frames.

[dcheng, 2016-10-27 08:08:57]

Patchset #5 (id:80001) has been deleted

[dcheng, 2016-11-02 01:46:42]

PTAL.

Unfortunately, jochen's v8 change
(https://bugs.chromium.org/p/v8/issues/detail?id=5588) was reverted, so I can't
send this through the trybots. But as of last week, things were working
(unfortunately, I reverted my own v8 changes this morning since I was expecting
v8 to roll in with jochen's changes... I'll re-create my patch later to finish
local testing at least)

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:120: // TODO(dcheng):
Check if this is needed.
We only detach globals on navigation, so the internal fields will be overwritten
shortly after this anyway.

That being said, it seems nice to do this on a conceptual basis.

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/v8_attributes.py:74: not
is_cross_origin)
I'm not sure this is actually necessary: it seems weird to specify CheckSecurity
*and* CrossOrigin on the same attribute. I propose that we just get rid of "and
not is_cross_origin" here.

(Once I can run the layout tests again, I am going to try removing this)

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/v8_attributes.py:139:
'has_cross_origin_getter': has_extended_attribute_value(attribute,
'CrossOrigin', None),
I'm not actually sure if it's OK to make this mutually exclusive with Setter:
the main attribute I see this being problematic for is window.location. As far
as I can tell, we don't rely it in our implementation (thanks to
PutForwards=href).

(This is another thing I will test again...)

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/scripts/v8_methods.py (left):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/v8_methods.py:199: #
TODO(yukishiino): Retire has_custom_registration flag.  Should be
I'm not sure if there were any other things linked to this TODO, but I think
I've removed everything related to this.

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (left):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:507:
V8HiddenValue::setHiddenValue(ScriptState::current(info.GetIsolate()),
v8::Local<v8::Object>::Cast(info.Holder()), name.As<v8::String>(), v8Value);
This isn't needed anymore: cross-origin calls to things like postMessage always
go through the custom named property getter interceptor, which will only ever
return the function.

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
(right):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:187: {%
block security_check_functions %}
This block is moved so it can reference the generated helpers for attributes /
methods.

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:220: {#####
TODO(dcheng): error out on attribute.has_custom_getter and
attribute.constructor_type? #####}
For all these TODOs, I'm open to the best way to proceed.

The current attributes / methods that are exposed cross-origin don't need to use
the complexity of per-world bindings or custom getters or constructor types.
Should we enforce this by making v8_attributes.py raise an exception if there
are attributes we don't support? Or is it OK to just let it fail later on when
it tries to compile the generated code?

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceCheckSecurity.idl
(right):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceCheckSecurity.idl:43:
[CrossOrigin, PerWorldBindings] void
doNotCheckSecurityPerWorldBindingsVoidMethod();
The IDL tests this case, but we never actually use it in practice. Should we
just remove this test case, or is this something important to support?

(Supporting it would mean additional complexity in the generated bindings for
the cross-origin interceptor)

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/DOMWindow.h (right):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/DOMWindow.h:105: // Blink, it's currently
marked as IsCrossOrigin.
This comment is actually obsolete; I've removed it locally and will upload it
with my next patchset.

[haraken, 2016-11-02 04:30:32]

Overall looks good.

This CL is doing multiple things in one go -- would it be possible to split it
into pieces and land them incrementally?

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:120: // TODO(dcheng):
Check if this is needed.
On 2016/11/02 01:46:42, dcheng wrote:
> We only detach globals on navigation, so the internal fields will be
overwritten
> shortly after this anyway.
> 
> That being said, it seems nice to do this on a conceptual basis.

Makes sense. Let's make the change in a separate CL.

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:377:
m_globalProxy.get().SetWrapperClassId(wrapperTypeInfo->wrapperClassId);

Can we call V8DOMWrapper::associateObjectWithWrapper instead of calling
setNativeInfo and etWrapperClassId?

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/v8_attributes.py:74: not
is_cross_origin)
On 2016/11/02 01:46:42, dcheng wrote:
> I'm not sure this is actually necessary: it seems weird to specify
CheckSecurity
> *and* CrossOrigin on the same attribute. I propose that we just get rid of
"and
> not is_cross_origin" here.
> 
> (Once I can run the layout tests again, I am going to try removing this)

Yeah, agreed. Let's make the change in a separate CL.

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:420:
has_cross_origin_indexed_getter = False

What about an indexed setter?

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:430:
has_cross_origin_named_getter = True

Why do we treat cross-origin methods as named getters?

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (left):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:507:
V8HiddenValue::setHiddenValue(ScriptState::current(info.GetIsolate()),
v8::Local<v8::Object>::Cast(info.Holder()), name.As<v8::String>(), v8Value);
On 2016/11/02 01:46:42, dcheng wrote:
> This isn't needed anymore: cross-origin calls to things like postMessage
always
> go through the custom named property getter interceptor, which will only ever
> return the function.

This is awesome!

After you land this CL, how much do you think our implementation will align with
the CrossOriginProperties in the spec?
(https://html.spec.whatwg.org/multipage/browsers.html#cross-origin-objects)

[dcheng, 2016-11-02 07:45:32]

I'm currently working on updating the docs, but sending out the updated CL
first.

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:120: // TODO(dcheng):
Check if this is needed.
On 2016/11/02 04:30:32, haraken wrote:
> On 2016/11/02 01:46:42, dcheng wrote:
> > We only detach globals on navigation, so the internal fields will be
> overwritten
> > shortly after this anyway.
> > 
> > That being said, it seems nice to do this on a conceptual basis.
> 
> Makes sense. Let's make the change in a separate CL.

It kind of makes sense in this CL though? Previously, we weren't setting these
fields or using them: now that we are, I think we should clean them up?

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:377:
m_globalProxy.get().SetWrapperClassId(wrapperTypeInfo->wrapperClassId);
On 2016/11/02 04:30:32, haraken wrote:
> 
> Can we call V8DOMWrapper::associateObjectWithWrapper instead of calling
> setNativeInfo and etWrapperClassId?

My concern with this is associateObjectWithWrapper doesn't have any way to
unassociate, so subsequent calls will not work correctly.

Perhaps a followup?

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/v8_attributes.py:74: not
is_cross_origin)
On 2016/11/02 04:30:32, haraken wrote:
> On 2016/11/02 01:46:42, dcheng wrote:
> > I'm not sure this is actually necessary: it seems weird to specify
> CheckSecurity
> > *and* CrossOrigin on the same attribute. I propose that we just get rid of
> "and
> > not is_cross_origin" here.
> > 
> > (Once I can run the layout tests again, I am going to try removing this)
> 
> Yeah, agreed. Let's make the change in a separate CL.

Added a TODO back.

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:420:
has_cross_origin_indexed_getter = False
On 2016/11/02 04:30:32, haraken wrote:
> 
> What about an indexed setter?

There are no cross-origin indexed setters. We can add them if it becomes
necessary later.

This actually goes along with my question in interface_base.cpp.tmpl. There are
a lot of things we don't really support with CrossOrigin attributes, for
example, per-world bindings. There is no real code that actually uses them.

For these things, is it better to try to make the bindings generator raise an
exception? Is it OK if we don't raise exceptions for anything? For example, it's
likely we'll never support cross-origin deleter, but do we need to raise an
exception for that?

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:430:
has_cross_origin_named_getter = True
On 2016/11/02 04:30:32, haraken wrote:
> 
> Why do we treat cross-origin methods as named getters?

Added a comment.

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (left):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:507:
V8HiddenValue::setHiddenValue(ScriptState::current(info.GetIsolate()),
v8::Local<v8::Object>::Cast(info.Holder()), name.As<v8::String>(), v8Value);
On 2016/11/02 04:30:32, haraken wrote:
> On 2016/11/02 01:46:42, dcheng wrote:
> > This isn't needed anymore: cross-origin calls to things like postMessage
> always
> > go through the custom named property getter interceptor, which will only
ever
> > return the function.
> 
> This is awesome!
> 
> After you land this CL, how much do you think our implementation will align
with
> the CrossOriginProperties in the spec?
> (https://html.spec.whatwg.org/multipage/browsers.html#cross-origin-objects)

I was reading this earlier and we almost match: our assign() function is exposed
cross-origin, but that is not in the spec.

(There is some cleanup I'd like to do in a followup for the named getter though)

https://codereview.chromium.org/2439013002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt (right):

https://codereview.chromium.org/2439013002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt:44:
CrossOrigin=|Getter|Setter
It turns out it can't be mutually exclusive.

I've changed this attribute so it works like this:

CrossOrigin with no arguments: defaults to just cross-origin getter.
CrossOrigin=Setter: only setter is cross-origin.
CrossOrigin=Getter: legal, but redundant with no argument version.
CrossOrigin=(Getter,Setter): both getter and setter are cross-origin (used for
window.location)

[Yuki, 2016-11-02 08:26:39]

Could you upload V8CrossOriginSetterInfo.h, too?

Looks good overall.

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:120: // TODO(dcheng):
Check if this is needed.
On 2016/11/02 04:30:32, haraken wrote:
> On 2016/11/02 01:46:42, dcheng wrote:
> > We only detach globals on navigation, so the internal fields will be
> overwritten
> > shortly after this anyway.
> > 
> > That being said, it seems nice to do this on a conceptual basis.
> 
> Makes sense. Let's make the change in a separate CL.

I vote to keep |clearNativeInfo| here.  Just my preference.

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:420:
has_cross_origin_indexed_getter = False
On 2016/11/02 07:45:32, dcheng wrote:
> On 2016/11/02 04:30:32, haraken wrote:
> > 
> > What about an indexed setter?
> 
> There are no cross-origin indexed setters. We can add them if it becomes
> necessary later.
> 
> This actually goes along with my question in interface_base.cpp.tmpl. There
are
> a lot of things we don't really support with CrossOrigin attributes, for
> example, per-world bindings. There is no real code that actually uses them.
> 
> For these things, is it better to try to make the bindings generator raise an
> exception? Is it OK if we don't raise exceptions for anything? For example,
it's
> likely we'll never support cross-origin deleter, but do we need to raise an
> exception for that?

I'm happy with raising an exception in the binding generator for unsupported
things (cross-origin deleter, etc).  People should claim to the binding team.

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceCheckSecurity.idl
(right):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceCheckSecurity.idl:43:
[CrossOrigin, PerWorldBindings] void
doNotCheckSecurityPerWorldBindingsVoidMethod();
On 2016/11/02 01:46:42, dcheng wrote:
> The IDL tests this case, but we never actually use it in practice. Should we
> just remove this test case, or is this something important to support?
> 
> (Supporting it would mean additional complexity in the generated bindings for
> the cross-origin interceptor)

I'm happy with removing this entirely.

[dcheng, 2016-11-02 09:07:42]

Oops, added the missing file. Also updated the markdown for CheckSecurity and
CrossOrigin, but there are some interesting things I need to look into more
tomorrow.

https://codereview.chromium.org/2439013002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md (right):

https://codereview.chromium.org/2439013002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:1414: all methods of
an interface. The security check verifies that the caller still
I need to figure out what's going on here tomorrow. I noticed that this security
check doesn't seem to be emitted for any getter or setter, and I'm not sure why.

For example, Window.history's generated bindings don't have this security check:

https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8W...

But Window is CheckSecurity=Receiver and history is a normal access attribute...

https://codereview.chromium.org/2439013002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:1438: };
I added some examples, because I didn't initially understand why this is
important.

https://codereview.chromium.org/2439013002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:1663:
[CrossOriginProperties]:
https://html.spec.whatwg.org/multipage/browsers.html#crossoriginproperties-(-o-)
I think this is more readable than having all the links inline: hopefully the
markdown generator agrees (I'll check this when I'm in the office tomorrow)

[dcheng, 2016-11-02 09:10:23]

(I will address the remaining comments tomorrow morning.)

[Yuki, 2016-11-02 09:43:49]

https://codereview.chromium.org/2439013002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md (right):

https://codereview.chromium.org/2439013002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:1414: all methods of
an interface. The security check verifies that the caller still
On 2016/11/02 09:07:41, dcheng wrote:
> I need to figure out what's going on here tomorrow. I noticed that this
security
> check doesn't seem to be emitted for any getter or setter, and I'm not sure
why.
> 
> For example, Window.history's generated bindings don't have this security
check:
> 
>
https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8W...
> 
> But Window is CheckSecurity=Receiver and history is a normal access
attribute...

Based on the old (i.e. current) implementation, Window.history is not
cross-origin accessible, so it's not specified with kAllCanRead.  So, V8Window's
securityCheck() rejects the access to Window.history.

For cross-origin accessible attributes/operations (i.e. [DoNotCheckSecurity] ==
kAllCanRead), you can access to the property even if it's cross origin.  So
we're generating the call to BindingSecurity::shouldAllowAccessTo(receiver)
because of [CheckSecurity=Receiver].

This is what I remember.  Sorry if I remember wrong things.

[haraken, 2016-11-02 12:15:50]

I'm just curious but is this CL going to change some web-exposed behavior? In
particular, I'm interested in the impact of replacing OriginSafeGetter/Setter
with the cross-origin interceptor.

[dcheng, 2016-11-03 07:44:45]

I haven't removed jochen's temporary hack in ScriptState yet, because I'm trying
to figure out how we should handle this. I think we should just change v8 so the
global proxy's internal fields are configured based on the object template (if
any). Otherwise, this leads to awkward code where we have to explicitly clear
the internal fields on code that wouldn't otherwise care. WDYT?

There are also some weird binding generator issues that I don't quite
understand, so if anyone has ideas, that would be helpful.

On 2016/11/02 09:43:49, Yuki wrote:
>
https://codereview.chromium.org/2439013002/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md (right):
> 
>
https://codereview.chromium.org/2439013002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:1414: all methods
of
> an interface. The security check verifies that the caller still
> On 2016/11/02 09:07:41, dcheng wrote:
> > I need to figure out what's going on here tomorrow. I noticed that this
> security
> > check doesn't seem to be emitted for any getter or setter, and I'm not sure
> why.
> > 
> > For example, Window.history's generated bindings don't have this security
> check:
> > 
> >
>
https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8W...
> > 
> > But Window is CheckSecurity=Receiver and history is a normal access
> attribute...
> 
> Based on the old (i.e. current) implementation, Window.history is not
> cross-origin accessible, so it's not specified with kAllCanRead.  So,
V8Window's
> securityCheck() rejects the access to Window.history.
> 
> For cross-origin accessible attributes/operations (i.e. [DoNotCheckSecurity]
==
> kAllCanRead), you can access to the property even if it's cross origin.  So
> we're generating the call to BindingSecurity::shouldAllowAccessTo(receiver)
> because of [CheckSecurity=Receiver].
> 
> This is what I remember.  Sorry if I remember wrong things.

Right, I think my question is why we don't emit this check at all: if I change
the comment on lines 62 and 285 so the comment has a unique string, rebuild, and
search the generated code, this comment never appears. It looks like we have the
code for this... so why isn't it getting generated at all? This is true with and
without my change.

However, in the test IDLs, the unique string appears, so it's very weird: what
causes it to show up in the generated tests but not actual Blink bindings? I
think something is broken either way.

On 2016/11/02 12:15:50, haraken wrote:
> I'm just curious but is this CL going to change some web-exposed behavior? In
> particular, I'm interested in the impact of replacing OriginSafeGetter/Setter
> with the cross-origin interceptor.

This should not affect behavior. For same-origin, the behavior is the same as
any other replaceable method: script can choose to override that attribute to
hook the method.

However, this hooking should not ever be visible cross-origin. If we're
cross-origin, we fail the security check, so we always use the cross origin
method getter, which will only ever return the original function.

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:420:
has_cross_origin_indexed_getter = False
On 2016/11/02 08:26:39, Yuki wrote:
> On 2016/11/02 07:45:32, dcheng wrote:
> > On 2016/11/02 04:30:32, haraken wrote:
> > > 
> > > What about an indexed setter?
> > 
> > There are no cross-origin indexed setters. We can add them if it becomes
> > necessary later.
> > 
> > This actually goes along with my question in interface_base.cpp.tmpl. There
> are
> > a lot of things we don't really support with CrossOrigin attributes, for
> > example, per-world bindings. There is no real code that actually uses them.
> > 
> > For these things, is it better to try to make the bindings generator raise
an
> > exception? Is it OK if we don't raise exceptions for anything? For example,
> it's
> > likely we'll never support cross-origin deleter, but do we need to raise an
> > exception for that?
> 
> I'm happy with raising an exception in the binding generator for unsupported
> things (cross-origin deleter, etc).  People should claim to the binding team.

I updated the bindings generator to raise exceptions for explicitly unsupported
use cases (which can be implemented later if we needed them).

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceCheckSecurity.idl
(right):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceCheckSecurity.idl:43:
[CrossOrigin, PerWorldBindings] void
doNotCheckSecurityPerWorldBindingsVoidMethod();
On 2016/11/02 08:26:39, Yuki wrote:
> On 2016/11/02 01:46:42, dcheng wrote:
> > The IDL tests this case, but we never actually use it in practice. Should we
> > just remove this test case, or is this something important to support?
> > 
> > (Supporting it would mean additional complexity in the generated bindings
for
> > the cross-origin interceptor)
> 
> I'm happy with removing this entirely.

However, it's weird: the bindings generator isn't failing on this line, even
though it should be. Any ideas?

[dcheng, 2016-11-03 07:46:23]

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

[commit-bot: I haz the power, 2016-11-03 07:46:45]

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

[dcheng, 2016-11-03 08:17:52]

(Please ignore trybot failures, I thought v8 had rolled to include jochen's
latest patch)

[commit-bot: I haz the power, 2016-11-03 08:40:48]

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

[commit-bot: I haz the power, 2016-11-03 08:40:49]

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

[dcheng, 2016-11-04 08:42:22]

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

[commit-bot: I haz the power, 2016-11-04 08:42:45]

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-04 09:25:11]

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

[commit-bot: I haz the power, 2016-11-04 09:25:12]

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...)

[dcheng, 2016-11-04 22:03:06]

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

[commit-bot: I haz the power, 2016-11-04 22:04:07]

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-04 22:24:31]

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

[commit-bot: I haz the power, 2016-11-04 22:24:35]

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

[blink-reviews, 2016-11-07 03:58:18]

Replied inline.


2016-11-03 16:44 GMT+09:00 <dcheng@chromium.org>:

> I haven't removed jochen's temporary hack in ScriptState yet, because I'm
> trying
> to figure out how we should handle this. I think we should just change v8
> so the
> global proxy's internal fields are configured based on the object template
> (if
> any). Otherwise, this leads to awkward code where we have to explicitly
> clear
> the internal fields on code that wouldn't otherwise care. WDYT?
>

Sounds good to me.



> There are also some weird binding generator issues that I don't quite
> understand, so if anyone has ideas, that would be helpful.
>
>
> On 2016/11/02 09:43:49, Yuki wrote:
> >
> https://codereview.chromium.org/2439013002/diff/200001/
> third_party/WebKit/Source/bindings/IDLExtendedAttributes.md
> > File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md
> (right):
> >
> >
> https://codereview.chromium.org/2439013002/diff/200001/
> third_party/WebKit/Source/bindings/IDLExtendedAttributes.md#newcode1414
> > third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:1414: all
> methods
> of
> > an interface. The security check verifies that the caller still
> > On 2016/11/02 09:07:41, dcheng wrote:
> > > I need to figure out what's going on here tomorrow. I noticed that this
> > security
> > > check doesn't seem to be emitted for any getter or setter, and I'm not
> sure
> > why.
> > >
> > > For example, Window.history's generated bindings don't have this
> security
> > check:
> > >
> > >
> >
> https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/
> V8Window.cpp?rcl=0&l=558
> > >
> > > But Window is CheckSecurity=Receiver and history is a normal access
> > attribute...
> >
> > Based on the old (i.e. current) implementation, Window.history is not
> > cross-origin accessible, so it's not specified with kAllCanRead. So,
> V8Window's
> > securityCheck() rejects the access to Window.history.
> >
> > For cross-origin accessible attributes/operations (i.e.
> [DoNotCheckSecurity]
> ==
> > kAllCanRead), you can access to the property even if it's cross origin.
> So
> > we're generating the call to BindingSecurity::
> shouldAllowAccessTo(receiver)
> > because of [CheckSecurity=Receiver].
> >
> > This is what I remember. Sorry if I remember wrong things.
>
> Right, I think my question is why we don't emit this check at all: if I
> change
> the comment on lines 62 and 285 so the comment has a unique string,
> rebuild, and
> search the generated code, this comment never appears. It looks like we
> have the
> code for this... so why isn't it getting generated at all? This is true
> with and
> without my change.
>

Oh, I'm sorry, line 62 and 285 on attributes.cpp.tmpl are currently dead
code because cross-origin accessible attributes on Window interface are
currently implemented as *data properties*.  They should be accessor
properties, but we're still in the middle of transition.

For data properties, securityCheck() in V8Window.cpp should handle the
cross-origin accessible data properties.



> However, in the test IDLs, the unique string appears, so it's very weird:
> what
> causes it to show up in the generated tests but not actual Blink bindings?
> I
> think something is broken either way.
>
> On 2016/11/02 12:15:50, haraken wrote:
> > I'm just curious but is this CL going to change some web-exposed
> behavior? In
> > particular, I'm interested in the impact of replacing
> OriginSafeGetter/Setter
> > with the cross-origin interceptor.
>
> This should not affect behavior. For same-origin, the behavior is the same
> as
> any other replaceable method: script can choose to override that attribute
> to
> hook the method.
>
> However, this hooking should not ever be visible cross-origin. If we're
> cross-origin, we fail the security check, so we always use the cross origin
> method getter, which will only ever return the original function.
>
>
> https://codereview.chromium.org/2439013002/diff/160001/
> third_party/WebKit/Source/bindings/scripts/v8_interface.py
> File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):
>
> https://codereview.chromium.org/2439013002/diff/160001/
> third_party/WebKit/Source/bindings/scripts/v8_interface.py#newcode420
> third_party/WebKit/Source/bindings/scripts/v8_interface.py:420:
> has_cross_origin_indexed_getter = False
> On 2016/11/02 08:26:39, Yuki wrote:
> > On 2016/11/02 07:45:32, dcheng wrote:
> > > On 2016/11/02 04:30:32, haraken wrote:
> > > >
> > > > What about an indexed setter?
> > >
> > > There are no cross-origin indexed setters. We can add them if it
> becomes
> > > necessary later.
> > >
> > > This actually goes along with my question in
> interface_base.cpp.tmpl. There
> > are
> > > a lot of things we don't really support with CrossOrigin attributes,
> for
> > > example, per-world bindings. There is no real code that actually
> uses them.
> > >
> > > For these things, is it better to try to make the bindings generator
> raise an
> > > exception? Is it OK if we don't raise exceptions for anything? For
> example,
> > it's
> > > likely we'll never support cross-origin deleter, but do we need to
> raise an
> > > exception for that?
> >
> > I'm happy with raising an exception in the binding generator for
> unsupported
> > things (cross-origin deleter, etc). People should claim to the
> binding team.
>
> I updated the bindings generator to raise exceptions for explicitly
> unsupported use cases (which can be implemented later if we needed
> them).
>
> https://codereview.chromium.org/2439013002/diff/160001/
> third_party/WebKit/Source/bindings/tests/idls/core/
> TestInterfaceCheckSecurity.idl
> File
> third_party/WebKit/Source/bindings/tests/idls/core/
> TestInterfaceCheckSecurity.idl
> (right):
>
> https://codereview.chromium.org/2439013002/diff/160001/
> third_party/WebKit/Source/bindings/tests/idls/core/
> TestInterfaceCheckSecurity.idl#newcode43
> third_party/WebKit/Source/bindings/tests/idls/core/
> TestInterfaceCheckSecurity.idl:43:
> [CrossOrigin, PerWorldBindings] void
> doNotCheckSecurityPerWorldBindingsVoidMethod();
> On 2016/11/02 08:26:39, Yuki wrote:
> > On 2016/11/02 01:46:42, dcheng wrote:
> > > The IDL tests this case, but we never actually use it in practice.
> Should we
> > > just remove this test case, or is this something important to
> support?
> > >
> > > (Supporting it would mean additional complexity in the generated
> bindings for
> > > the cross-origin interceptor)
> >
> > I'm happy with removing this entirely.
>
> However, it's weird: the bindings generator isn't failing on this line,
> even though it should be. Any ideas?
>
> https://codereview.chromium.org/2439013002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-11-07 03:58:18]

Replied inline.


2016-11-03 16:44 GMT+09:00 <dcheng@chromium.org>:

> I haven't removed jochen's temporary hack in ScriptState yet, because I'm
> trying
> to figure out how we should handle this. I think we should just change v8
> so the
> global proxy's internal fields are configured based on the object template
> (if
> any). Otherwise, this leads to awkward code where we have to explicitly
> clear
> the internal fields on code that wouldn't otherwise care. WDYT?
>

Sounds good to me.



> There are also some weird binding generator issues that I don't quite
> understand, so if anyone has ideas, that would be helpful.
>
>
> On 2016/11/02 09:43:49, Yuki wrote:
> >
> https://codereview.chromium.org/2439013002/diff/200001/
> third_party/WebKit/Source/bindings/IDLExtendedAttributes.md
> > File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md
> (right):
> >
> >
> https://codereview.chromium.org/2439013002/diff/200001/
> third_party/WebKit/Source/bindings/IDLExtendedAttributes.md#newcode1414
> > third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:1414: all
> methods
> of
> > an interface. The security check verifies that the caller still
> > On 2016/11/02 09:07:41, dcheng wrote:
> > > I need to figure out what's going on here tomorrow. I noticed that this
> > security
> > > check doesn't seem to be emitted for any getter or setter, and I'm not
> sure
> > why.
> > >
> > > For example, Window.history's generated bindings don't have this
> security
> > check:
> > >
> > >
> >
> https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/
> V8Window.cpp?rcl=0&l=558
> > >
> > > But Window is CheckSecurity=Receiver and history is a normal access
> > attribute...
> >
> > Based on the old (i.e. current) implementation, Window.history is not
> > cross-origin accessible, so it's not specified with kAllCanRead. So,
> V8Window's
> > securityCheck() rejects the access to Window.history.
> >
> > For cross-origin accessible attributes/operations (i.e.
> [DoNotCheckSecurity]
> ==
> > kAllCanRead), you can access to the property even if it's cross origin.
> So
> > we're generating the call to BindingSecurity::
> shouldAllowAccessTo(receiver)
> > because of [CheckSecurity=Receiver].
> >
> > This is what I remember. Sorry if I remember wrong things.
>
> Right, I think my question is why we don't emit this check at all: if I
> change
> the comment on lines 62 and 285 so the comment has a unique string,
> rebuild, and
> search the generated code, this comment never appears. It looks like we
> have the
> code for this... so why isn't it getting generated at all? This is true
> with and
> without my change.
>

Oh, I'm sorry, line 62 and 285 on attributes.cpp.tmpl are currently dead
code because cross-origin accessible attributes on Window interface are
currently implemented as *data properties*.  They should be accessor
properties, but we're still in the middle of transition.

For data properties, securityCheck() in V8Window.cpp should handle the
cross-origin accessible data properties.



> However, in the test IDLs, the unique string appears, so it's very weird:
> what
> causes it to show up in the generated tests but not actual Blink bindings?
> I
> think something is broken either way.
>
> On 2016/11/02 12:15:50, haraken wrote:
> > I'm just curious but is this CL going to change some web-exposed
> behavior? In
> > particular, I'm interested in the impact of replacing
> OriginSafeGetter/Setter
> > with the cross-origin interceptor.
>
> This should not affect behavior. For same-origin, the behavior is the same
> as
> any other replaceable method: script can choose to override that attribute
> to
> hook the method.
>
> However, this hooking should not ever be visible cross-origin. If we're
> cross-origin, we fail the security check, so we always use the cross origin
> method getter, which will only ever return the original function.
>
>
> https://codereview.chromium.org/2439013002/diff/160001/
> third_party/WebKit/Source/bindings/scripts/v8_interface.py
> File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):
>
> https://codereview.chromium.org/2439013002/diff/160001/
> third_party/WebKit/Source/bindings/scripts/v8_interface.py#newcode420
> third_party/WebKit/Source/bindings/scripts/v8_interface.py:420:
> has_cross_origin_indexed_getter = False
> On 2016/11/02 08:26:39, Yuki wrote:
> > On 2016/11/02 07:45:32, dcheng wrote:
> > > On 2016/11/02 04:30:32, haraken wrote:
> > > >
> > > > What about an indexed setter?
> > >
> > > There are no cross-origin indexed setters. We can add them if it
> becomes
> > > necessary later.
> > >
> > > This actually goes along with my question in
> interface_base.cpp.tmpl. There
> > are
> > > a lot of things we don't really support with CrossOrigin attributes,
> for
> > > example, per-world bindings. There is no real code that actually
> uses them.
> > >
> > > For these things, is it better to try to make the bindings generator
> raise an
> > > exception? Is it OK if we don't raise exceptions for anything? For
> example,
> > it's
> > > likely we'll never support cross-origin deleter, but do we need to
> raise an
> > > exception for that?
> >
> > I'm happy with raising an exception in the binding generator for
> unsupported
> > things (cross-origin deleter, etc). People should claim to the
> binding team.
>
> I updated the bindings generator to raise exceptions for explicitly
> unsupported use cases (which can be implemented later if we needed
> them).
>
> https://codereview.chromium.org/2439013002/diff/160001/
> third_party/WebKit/Source/bindings/tests/idls/core/
> TestInterfaceCheckSecurity.idl
> File
> third_party/WebKit/Source/bindings/tests/idls/core/
> TestInterfaceCheckSecurity.idl
> (right):
>
> https://codereview.chromium.org/2439013002/diff/160001/
> third_party/WebKit/Source/bindings/tests/idls/core/
> TestInterfaceCheckSecurity.idl#newcode43
> third_party/WebKit/Source/bindings/tests/idls/core/
> TestInterfaceCheckSecurity.idl:43:
> [CrossOrigin, PerWorldBindings] void
> doNotCheckSecurityPerWorldBindingsVoidMethod();
> On 2016/11/02 08:26:39, Yuki wrote:
> > On 2016/11/02 01:46:42, dcheng wrote:
> > > The IDL tests this case, but we never actually use it in practice.
> Should we
> > > just remove this test case, or is this something important to
> support?
> > >
> > > (Supporting it would mean additional complexity in the generated
> bindings for
> > > the cross-origin interceptor)
> >
> > I'm happy with removing this entirely.
>
> However, it's weird: the bindings generator isn't failing on this line,
> even though it should be. Any ideas?
>
> https://codereview.chromium.org/2439013002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Yuki, 2016-11-07 05:02:42]

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceCheckSecurity.idl
(right):

https://codereview.chromium.org/2439013002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceCheckSecurity.idl:43:
[CrossOrigin, PerWorldBindings] void
doNotCheckSecurityPerWorldBindingsVoidMethod();
On 2016/11/03 07:44:45, dcheng wrote:
> On 2016/11/02 08:26:39, Yuki wrote:
> > On 2016/11/02 01:46:42, dcheng wrote:
> > > The IDL tests this case, but we never actually use it in practice. Should
we
> > > just remove this test case, or is this something important to support?
> > > 
> > > (Supporting it would mean additional complexity in the generated bindings
> for
> > > the cross-origin interceptor)
> > 
> > I'm happy with removing this entirely.
> 
> However, it's weird: the bindings generator isn't failing on this line, even
> though it should be. Any ideas?

Why do you think "it should be"?  The bindings generator doesn't handle error
cases in general, so usually the generator itself doesn't emit any error.

When I added [PerWorldBindings] to focus() in Window.idl:
    [DoNotCheckSecurity, CallWith=ExecutionContext, PerWorldBindings] void
focus();
then, it produced a compile error.

[jochen (gone - plz use gerrit), 2016-11-12 23:42:28]

https://codereview.chromium.org/2439013002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/Location.idl (right):

https://codereview.chromium.org/2439013002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/Location.idl:55:
[SetterCallWith=(CurrentWindow,EnteredWindow), RaisesException=Setter] attribute
DOMString protocol;
I think you'll need to invoke the failed access check callback for the non-cross
origin attributes, e.g., when someone tries to do
crossOriginWindow.location.protocol = 'ftp', this method would create the
exception:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

[jochen (gone - plz use gerrit), 2016-11-12 23:44:51]

Daniel, you mentioned some problem with calling vs current window, but hangout
ate my chat history :-/

I looked at some of the failing tests, and it seems that the failures are around
missing exceptions. I added a comment to why I think that is.

does that solve your problem? If not, can you explain the problem again please?

[dcheng, 2016-11-13 00:11:43]

On 2016/11/12 23:44:51, jochen (travelling til Nov 18) wrote:
> Daniel, you mentioned some problem with calling vs current window, but hangout
> ate my chat history :-/
> 
> I looked at some of the failing tests, and it seems that the failures are
around
> missing exceptions. I added a comment to why I think that is.
> 
> does that solve your problem? If not, can you explain the problem again
please?

So there's several problems.
1. As mentioned, some of the failing tests are failing because the cross-origin
interceptors forget to throw if the property isn't handled. That's easy to fix
(it's on my local TODO list, but I've been debugging other issues, so I haven't
uploaded a patchset with this change)

2. Tests like fast/dom/Window/window-postmessage-clone-frames.html never
complete. The main frame does window.frames[0].postMessage(...) and the child
frame does a event.source.postMessage(...) to reply. This test ends up in an
infinite loop in PS13, because event.source == window.top.frames[0] instead of
window.top. It's still failing in PS14, but I haven't re-investigated why: this
shouldn't hit the cross-origin interceptors, but I'm reasonably confident this
is something that can be resolved within Blink.

3. Tests like fast/frames/sandboxed-iframe-navigation-parent.html fail. This is
going through the cross-origin interceptors. Location::setLocation() ultimately
tries to enforce the security checks using currentDOMWindow. In this test,
there's a sandboxed iframe with a nested iframe. The nested iframe attempts to
navigate the sandboxed iframe, and expects it to fail. Previously,
currentDOMWindow == the nested iframe, but with the cross-origin interceptors, I
think currentDOMWindow == the sandboxed iframe--so the security check always
passes instead. I believe we'll need some help from v8 to get this case correct.

[jochen (gone - plz use gerrit), 2016-11-13 01:29:24]

so whatever disabled assigning to the origin before was not based on
cross-origin checks I think.

right now, the InternalV8Location::securityCheck grants access, as the sandbox
doesn't restrict same-origin access.

[jochen (gone - plz use gerrit), 2016-11-13 01:36:40]

I guess Frame::canNavigate is supposed to handle this. will debug more.

[dcheng, 2016-11-13 02:06:15]

On 2016/11/13 01:36:40, jochen (travelling til Nov 18) wrote:
> I guess Frame::canNavigate is supposed to handle this. will debug more.

Actually I analyzed case 3 incorrectly: the nested iframe and the sandboxed
iframe are same origin, so it's expected they pass the security check.

My suspicion is changing the property so it's not an ALL_CAN_READ property is
affecting the current context, but I'm not yet sure how

[jochen (gone - plz use gerrit), 2016-11-13 06:01:44]

I think the difference is that V8LocationMethods now contains "assign", but it
should still be an origin-safe getter which would return a new function that is
bound to the correct context.

[dcheng, 2016-12-06 00:40:01]

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

[commit-bot: I haz the power, 2016-12-06 00:40:28]

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-12-06 01:23:39]

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

[commit-bot: I haz the power, 2016-12-06 01:23:40]

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_...)

[dcheng, 2016-12-06 02:02:24]

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

[commit-bot: I haz the power, 2016-12-06 02:03:26]

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-12-06 02:49:19]

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

[commit-bot: I haz the power, 2016-12-06 02:49:22]

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

[dcheng, 2016-12-07 08:19:08]

PTAL. I had to implement enumerator support as well, but this should be pretty
close.

https://codereview.chromium.org/2439013002/diff/400001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/cross-frame-access-enumeration-expected.txt
(right):

https://codereview.chromium.org/2439013002/diff/400001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/cross-frame-access-enumeration-expected.txt:8:
FAIL: Cross frame access by getting the keys of the Location object returned
non-whitelisted key: href
Not sure what to do here. The w3c test (see below) and this test disagree. This
test expects assign and replace to be the only cross frame keys.

The w3c test expects replace and href...

https://codereview.chromium.org/2439013002/diff/400001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/cross-frame-access-put-expected.txt
(left):

https://codereview.chromium.org/2439013002/diff/400001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/cross-frame-access-put-expected.txt:317:
PASS: Unable to set property opener: SecurityError: Failed to set the 'opener'
property on 'Window': Blocked a frame with origin "http://127.0.0.1:8000" from
accessing a cross-origin frame.
Note: this should be an easy fix in a followup by plumbing the access type /
property information through to failedAccessCheckFor(). I don't want to include
the fix in this patch, since there already so many moving parts. Fixing this
generically in the cross-origin interceptors should make the messages generally
more helpful, not just for opener.

(Another easy fix I could have taken in this patch is pretend that opener is
cross-origin, but that doesn't seem nice either.)

I also tried removing the currently BindingSecurity check in the custom opener
setter, but there are a few things that depend on it, so cleaning up that custom
setter will be part of a followup as well (I added a TODO locally, haven't
uploaded that CL yet)

https://codereview.chromium.org/2439013002/diff/400001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/w3c/cross-origin-objects-expected.txt
(right):

https://codereview.chromium.org/2439013002/diff/400001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/w3c/cross-origin-objects-expected.txt:9:
FAIL [[GetOwnProperty]] - Property descriptors for cross-origin properties
should be set up correctly assert_equals: property descriptor for postMessage
should have writable: false expected false but got true
I haven't quite figured out how to fix this: I think this is getting derived
from the actual value returned by originSafePostMessageMethodGetter. I think
this is probably the biggest remaining issue, and I'm currently debugging it.

https://codereview.chromium.org/2439013002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/2439013002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:230: if (!target)
This is probably needed, but I'd like to confirm in a followup.

https://codereview.chromium.org/2439013002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
(right):

https://codereview.chromium.org/2439013002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:294:
toV8SequenceInternal(
This isn't very nice, but there's no easy way to convert a hash set to an array
right now.

Currently, the HashSet is used, because there might be duplicates between the
getter / setter table. This is a hack though; my plan is to combine this into
one list.

[Yuki, 2016-12-07 12:03:41]

To me, the CL looks good in general.

https://codereview.chromium.org/2439013002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
(right):

https://codereview.chromium.org/2439013002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:294:
toV8SequenceInternal(
On 2016/12/07 08:19:08, dcheng wrote:
> This isn't very nice, but there's no easy way to convert a hash set to an
array
> right now.
> 
> Currently, the HashSet is used, because there might be duplicates between the
> getter / setter table. This is a hack though; my plan is to combine this into
> one list.

nit: I think the code generator can pre-computes everything and generates a list
of names.  We don't need much runtime code.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:237:
ExceptionState exceptionState(ExceptionState::UnknownContext, 0, 0,
ExceptionState should be created in the call sites, where interface name, etc.
are clear.  It's better to be close as much as possible to the point that the
execution enters into Blink from V8.

It's also not good that we actually throw a v8::Exception here because V8 APIs
no longer work correctly having an exception.  It's better to *schedule* an
exception in the ExceptionState (with throwSecurityError) but not actually throw
it yet (the dtor of ExceptionState actually throws).

e.g.
    void foo() {
      BindingSecurity::failedAccessCheckFor(...);
    }
    void bar() { foo(); }
    void baz() {
      bar();
      do_something();  // do something with V8 APIs, but V8 APIs do nothing
because an exception is thrown.
    }
It's very hard to understand why do_something() doesn't work in a specific case
just seeing baz().

We'd like the following coding pattern.
    void foo(ExceptionState& exceptionState) {
      BindingSecurity::failedAccessCheckFor(...);
    }
    void bar(ExceptionState& exceptionState) {
      foo(exceptionState);
    }
    void baz(ExceptionState& exceptionState) {
      bar(exceptionState);
      if (exceptionState.hadException()) {
        return handle_error();
      }
      do_something();
    }
Then, it's a caller's duty to check exceptionState.hadException().

nit: Please use non-deprecated version of ExceptionState's ctors.  We no longer
need isolate->GetCurrentContext()->Global().

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:379:
'has_origin_safe_method_setter': any(method['is_cross_origin'] and not
method['is_unforgeable']
super nit: Could this be
    is_cross_origin and "method has setter"
?  (maybe, method.setter?)

Probably, "not method['is_unforgeable']" was meant that the method has setter,
but it's not a correct way.

Please ignore this comment if it's not easy.

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

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:492: v8 doesn't
support querying the incumbent context. For now, always
"incumbent realm" is an idea of HTML, hence it should be implemented in Blink, I
think.  It's not V8's fault.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:665: v8 doesn't
support querying the incumbent context. For now, always
ditto.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
(right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:241: for
(const auto& attribute: kCrossOriginAttributeTable) {
nit: attribute : kCrossOrigin...
(one space before the colon)

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:268: for
(const auto& attribute: kCrossOriginAttributeTable) {
ditto

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:524: v8 doesn't
support querying the incumbent context. For now, always
ditto

[jochen (gone - plz use gerrit), 2016-12-07 15:40:12]

I haven't found enough time today to wrap my head around the CL, sorry. Will try
tomorrow

[haraken, 2016-12-08 08:21:02]

Mostly LG.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:237:
ExceptionState exceptionState(ExceptionState::UnknownContext, 0, 0,
On 2016/12/07 12:03:40, Yuki wrote:
> ExceptionState should be created in the call sites, where interface name, etc.
> are clear.  It's better to be close as much as possible to the point that the
> execution enters into Blink from V8.
> 
> It's also not good that we actually throw a v8::Exception here because V8 APIs
> no longer work correctly having an exception.  It's better to *schedule* an
> exception in the ExceptionState (with throwSecurityError) but not actually
throw
> it yet (the dtor of ExceptionState actually throws).
> 
> e.g.
>     void foo() {
>       BindingSecurity::failedAccessCheckFor(...);
>     }
>     void bar() { foo(); }
>     void baz() {
>       bar();
>       do_something();  // do something with V8 APIs, but V8 APIs do nothing
> because an exception is thrown.
>     }
> It's very hard to understand why do_something() doesn't work in a specific
case
> just seeing baz().
> 
> We'd like the following coding pattern.
>     void foo(ExceptionState& exceptionState) {
>       BindingSecurity::failedAccessCheckFor(...);
>     }
>     void bar(ExceptionState& exceptionState) {
>       foo(exceptionState);
>     }
>     void baz(ExceptionState& exceptionState) {
>       bar(exceptionState);
>       if (exceptionState.hadException()) {
>         return handle_error();
>       }
>       do_something();
>     }
> Then, it's a caller's duty to check exceptionState.hadException().
> 
> nit: Please use non-deprecated version of ExceptionState's ctors.  We no
longer
> need isolate->GetCurrentContext()->Global().

What do you mean? Note that BindingSecurity::failedAccessCheckFor is called by
V8 (via failedAccessCheckCallbackInMainThread or window's interceptor), not by
some method in Blink. I think it's just fine to throw an exception here.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8CrossOriginSetterInfo.h
(right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8CrossOriginSetterInfo.h:16: //
normal setter interceptor takes a v8::PropertyCallbackInfo<void> argument.

Hmm, this looks nasty. Can we fix it in a follow-up CL?

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
(right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:304:
Document* doc = toLocalFrame(frame)->document();

What happens if the frame is a remote frame? Does
BindingSecurity::shouldAllowAccessTo return false for a remote frame?

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/v8_attributes.py:147:
'has_cross_origin_setter': has_extended_attribute_value(attribute,
'CrossOrigin', 'Setter'),

Do you want to make [CrossOrigin] equivalent to [CrossOrigin=Getter]? Personally
it seems straightforward to make [CrossOrigin] equivalent to
[CrossOrigin=Getter|Setter].

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:450: if
context['indexed_property_getter'] and
context['indexed_property_getter']['is_cross_origin']:

Is there any practical use case for cross-origin indexed getters?

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:418:
{{cpp_class_or_partial}}V8Internal::{{attribute.name}}AttributeSetter{{world_suffix}}(v8Value,
V8CrossOriginSetterInfo(info.GetIsolate(), info.Holder()));

You don't need to support {{world_suffix}} since [CrossOrigin] doesn't support
per-world bindings.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
(right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:199: return
false; // the frame is gone.

Not really. window.IsEmpty happens when the prototype chain is tweaked. Whether
the frame is gone is checked by the following
BindingSecurity::shouldAllowAccessTo.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:211: //
TODO(dcheng): Can we / should we use AtomicString here? That means using
DEFINE_STATIC_LOCAL here.

I'm fine with const char*.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:230:
{"{{method.name}}",
&{{cpp_class}}V8Internal::{{method.name}}OriginSafeMethodGetter, nullptr},

After this CL, OriginSafeMethodSetter is no longer used, right? If so, can we
remove code for OriginSafeMethodSetter?

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:236: void
crossOriginNamedGetter(v8::Local<v8::Name> name, const
v8::PropertyCallbackInfo<v8::Value>& info) {

Just to confirm: V8 calls back crossOriginNamedGetter only when securityCheck
returns false, right? I just want to check if the overhead of
crossOriginNamedGetter doesn't affect normal window.foo accesses.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:274: 

Why don't you need to call V8Internal::namedPropertySetter when
named_property_setter.is_cross_origin is set?

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:296:
{{v8_class}}::indexedPropertyGetterCustom(index, info);

Is there any real use case of indexedPropertyGetterCustom?

[dcheng, 2016-12-08 09:04:11]

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:237:
ExceptionState exceptionState(ExceptionState::UnknownContext, 0, 0,
On 2016/12/08 08:21:02, haraken wrote:
> On 2016/12/07 12:03:40, Yuki wrote:
> > ExceptionState should be created in the call sites, where interface name,
etc.
> > are clear.  It's better to be close as much as possible to the point that
the
> > execution enters into Blink from V8.
> > 
> > It's also not good that we actually throw a v8::Exception here because V8
APIs
> > no longer work correctly having an exception.  It's better to *schedule* an
> > exception in the ExceptionState (with throwSecurityError) but not actually
> throw
> > it yet (the dtor of ExceptionState actually throws).
> > 
> > e.g.
> >     void foo() {
> >       BindingSecurity::failedAccessCheckFor(...);
> >     }
> >     void bar() { foo(); }
> >     void baz() {
> >       bar();
> >       do_something();  // do something with V8 APIs, but V8 APIs do nothing
> > because an exception is thrown.
> >     }
> > It's very hard to understand why do_something() doesn't work in a specific
> case
> > just seeing baz().
> > 
> > We'd like the following coding pattern.
> >     void foo(ExceptionState& exceptionState) {
> >       BindingSecurity::failedAccessCheckFor(...);
> >     }
> >     void bar(ExceptionState& exceptionState) {
> >       foo(exceptionState);
> >     }
> >     void baz(ExceptionState& exceptionState) {
> >       bar(exceptionState);
> >       if (exceptionState.hadException()) {
> >         return handle_error();
> >       }
> >       do_something();
> >     }
> > Then, it's a caller's duty to check exceptionState.hadException().
> > 
> > nit: Please use non-deprecated version of ExceptionState's ctors.  We no
> longer
> > need isolate->GetCurrentContext()->Global().
> 
> What do you mean? Note that BindingSecurity::failedAccessCheckFor is called by
> V8 (via failedAccessCheckCallbackInMainThread or window's interceptor), not by
> some method in Blink. I think it's just fine to throw an exception here.

I moved this around to make it so the interceptors can invoke this helper as
well. Originally, I didn't expose this at all, but this leads to a problem:

A named getter interceptor can signal an access failure just by not setting a
return value. v8 will notice that there was no value returned, and call the
access check failed callback

However, a named setter interceptor can't easily signal failure. So it needs to
throw an exception. To make the implementation of the interceptors in Blink
consistent, I have them consistently throw exceptions on access check failures.

In a followup patch, we can actually improve the error messages further: since
we know the context in which it's being invoked, we can instantiate
ExceptionState with the interface name, property name, and the access type: the
net result should be that our SecurityErrors will be more useful afterwards.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8CrossOriginSetterInfo.h
(right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8CrossOriginSetterInfo.h:16: //
normal setter interceptor takes a v8::PropertyCallbackInfo<void> argument.
On 2016/12/08 08:21:02, haraken wrote:
> 
> Hmm, this looks nasty. Can we fix it in a follow-up CL?

I don't see an easy way to fix it without changing v8. I'm not actually sure why
the access check interceptor has a v8::Value argument: 99.9% of the time, it
seems like the return value is just going to be the input value, so v8 can do
that itself. Maybe +jochen has some history on this.

(I've fixed this comment to fix some incorrect terminology as well)

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
(right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:304:
Document* doc = toLocalFrame(frame)->document();
On 2016/12/08 08:21:02, haraken wrote:
> 
> What happens if the frame is a remote frame? Does
> BindingSecurity::shouldAllowAccessTo return false for a remote frame?

Yes, BindingSecurity will always return false in that case. I combined the two
cases to avoid having to call failedAccessCheckFor() twice.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/v8_attributes.py:147:
'has_cross_origin_setter': has_extended_attribute_value(attribute,
'CrossOrigin', 'Setter'),
On 2016/12/08 08:21:02, haraken wrote:
> 
> Do you want to make [CrossOrigin] equivalent to [CrossOrigin=Getter]?
Personally
> it seems straightforward to make [CrossOrigin] equivalent to
> [CrossOrigin=Getter|Setter].

I guess my goal was to try to make the scoping of cross-origin attributes more
clear: CrossOrigin=Getter is the common case we need to support, so we support
that as the shorthand version.

However, we also need to support CrossOrigin=Setter for Location.href and
CrossOrigin=(Getter,Setter) for Window.location: since these are just individual
cases, it makes sense (to me) that the IDL file would have to explicitly declare
this. Does that make sense?

It does make this part slightly more complicated, but I think it's not too bad.
It already revealed some interesting behavior: since window.opener is
DoNotCheckSecurity (which is implicity Getter+Setter), the setter needs to be
explicitly check for cross-origin accesses. I think the increased clarity in the
IDL file is worth the complexity tradeoff.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:379:
'has_origin_safe_method_setter': any(method['is_cross_origin'] and not
method['is_unforgeable']
On 2016/12/07 12:03:40, Yuki wrote:
> super nit: Could this be
>     is_cross_origin and "method has setter"
> ?  (maybe, method.setter?)
> 
> Probably, "not method['is_unforgeable']" was meant that the method has setter,
> but it's not a correct way.
> 
> Please ignore this comment if it's not easy.

I'm not sure of a better way to determine this: I looked at the jinja context
for methods, and I don't think there's another signal we can use atm.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:450: if
context['indexed_property_getter'] and
context['indexed_property_getter']['is_cross_origin']:
On 2016/12/08 08:21:02, haraken wrote:
> 
> Is there any practical use case for cross-origin indexed getters?

window[0], window[1], window[2], ... window[window.length] uses the indexed
getter.

(We could not implement this, and I think it would just go through the named
getter interceptor, but that makes the named getter interceptor more
complicated)

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:418:
{{cpp_class_or_partial}}V8Internal::{{attribute.name}}AttributeSetter{{world_suffix}}(v8Value,
V8CrossOriginSetterInfo(info.GetIsolate(), info.Holder()));
On 2016/12/08 08:21:02, haraken wrote:
> 
> You don't need to support {{world_suffix}} since [CrossOrigin] doesn't support
> per-world bindings.

Done.

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

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:492: v8 doesn't
support querying the incumbent context. For now, always
On 2016/12/07 12:03:40, Yuki wrote:
> "incumbent realm" is an idea of HTML, hence it should be implemented in Blink,
I
> think.  It's not V8's fault.

It's awkward to implement in Blink though: according to domenic@, we should not
be creating per-realm function objects except for cross-origin accesses.

If we're OK with keeping this behavior in the long-term, I will just change the
comments to explain the implementation strategy. WDYT?

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
(right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:199: return
false; // the frame is gone.
On 2016/12/08 08:21:02, haraken wrote:
> 
> Not really. window.IsEmpty happens when the prototype chain is tweaked.
Whether
> the frame is gone is checked by the following
> BindingSecurity::shouldAllowAccessTo.
> 

I'm just copying and pasting code here =)

What would you suggest as a more accurate comment?

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:211: //
TODO(dcheng): Can we / should we use AtomicString here? That means using
DEFINE_STATIC_LOCAL here.
On 2016/12/08 08:21:02, haraken wrote:
> 
> I'm fine with const char*.

Done. I removed the TODO... I think it might still be an interesting
optimization, but maybe it's not a big deal.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:230:
{"{{method.name}}",
&{{cpp_class}}V8Internal::{{method.name}}OriginSafeMethodGetter, nullptr},
On 2016/12/08 08:21:02, haraken wrote:
> 
> After this CL, OriginSafeMethodSetter is no longer used, right? If so, can we
> remove code for OriginSafeMethodSetter?

Unfortunately, it is still needed. Originally, I tried removing this, and this
broke things like postMessage: event.source would be set incorrectly.

postMessage and the location setters need the calling context (incumbent realm
in spec terminology): they use this to determine if the calling context has
access to the target context. Since v8 is trying to get rid of
GetCallingContext(), we need to support this another way: instead, we create
function objects for each realm, so that when we call GetCurrentContext(), it
ends up pointing to the context which we created the function object for.

With this hack, if document A post messages document B, GetCurrentContext() ==
document A, and we enforce security checks / set event.source correctly, etc.

However, if we do not create a per-realm function object, then
GetCurrentContext() == the context we are calling *into*: that means that when
document A post messages document B, GetCurrentContext() == document B. This
breaks security checks for Location, as well as event.source (since event.source
doesn't point back at the caller).

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:236: void
crossOriginNamedGetter(v8::Local<v8::Name> name, const
v8::PropertyCallbackInfo<v8::Value>& info) {
On 2016/12/08 08:21:02, haraken wrote:
> 
> Just to confirm: V8 calls back crossOriginNamedGetter only when securityCheck
> returns false, right? I just want to check if the overhead of
> crossOriginNamedGetter doesn't affect normal window.foo accesses.

That's correct: this interceptor is only invoked when the access check fails.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:241: for
(const auto& attribute: kCrossOriginAttributeTable) {
On 2016/12/07 12:03:40, Yuki wrote:
> nit: attribute : kCrossOrigin...
> (one space before the colon)

Done.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:268: for
(const auto& attribute: kCrossOriginAttributeTable) {
On 2016/12/07 12:03:40, Yuki wrote:
> ditto

Done.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:274: 
On 2016/12/08 08:21:02, haraken wrote:
> 
> Why don't you need to call V8Internal::namedPropertySetter when
> named_property_setter.is_cross_origin is set?

I could add this for consistency... but there is no IDL that would use it, and
it would not be tested at all. So I just omitted it.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:296:
{{v8_class}}::indexedPropertyGetterCustom(index, info);
On 2016/12/08 08:21:02, haraken wrote:
> 
> Is there any real use case of indexedPropertyGetterCustom?

See my previous comment about window[0], window[1], etc.

[dcheng, 2016-12-08 16:23:41]

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

[commit-bot: I haz the power, 2016-12-08 16:23:58]

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-12-08 16:27:08]

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

[commit-bot: I haz the power, 2016-12-08 16:27:09]

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

[dcheng, 2016-12-08 20:25:35]

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

[commit-bot: I haz the power, 2016-12-08 20:26:17]

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-12-08 23:06:09]

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

[commit-bot: I haz the power, 2016-12-08 23:06:10]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-12-09 02:26:56]

LGTM

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
(right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:199: return
false; // the frame is gone.
On 2016/12/08 09:04:11, dcheng wrote:
> On 2016/12/08 08:21:02, haraken wrote:
> > 
> > Not really. window.IsEmpty happens when the prototype chain is tweaked.
> Whether
> > the frame is gone is checked by the following
> > BindingSecurity::shouldAllowAccessTo.
> > 
> 
> I'm just copying and pasting code here =)
> 
> What would you suggest as a more accurate comment?

"It's not a window wrapper" :)

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:230:
{"{{method.name}}",
&{{cpp_class}}V8Internal::{{method.name}}OriginSafeMethodGetter, nullptr},
On 2016/12/08 09:04:11, dcheng wrote:
> On 2016/12/08 08:21:02, haraken wrote:
> > 
> > After this CL, OriginSafeMethodSetter is no longer used, right? If so, can
we
> > remove code for OriginSafeMethodSetter?
> 
> Unfortunately, it is still needed. Originally, I tried removing this, and this
> broke things like postMessage: event.source would be set incorrectly.
> 
> postMessage and the location setters need the calling context (incumbent realm
> in spec terminology): they use this to determine if the calling context has
> access to the target context. Since v8 is trying to get rid of
> GetCallingContext(), we need to support this another way: instead, we create
> function objects for each realm, so that when we call GetCurrentContext(), it
> ends up pointing to the context which we created the function object for.
> 
> With this hack, if document A post messages document B, GetCurrentContext() ==
> document A, and we enforce security checks / set event.source correctly, etc.
> 
> However, if we do not create a per-realm function object, then
> GetCurrentContext() == the context we are calling *into*: that means that when
> document A post messages document B, GetCurrentContext() == document B. This
> breaks security checks for Location, as well as event.source (since
event.source
> doesn't point back at the caller).

Yes, I understand why we still need to keep OriginSafeMethodGetter. My question
is if we still need OriginSafeMethodSetter.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:274: 
On 2016/12/08 09:04:11, dcheng wrote:
> On 2016/12/08 08:21:02, haraken wrote:
> > 
> > Why don't you need to call V8Internal::namedPropertySetter when
> > named_property_setter.is_cross_origin is set?
> 
> I could add this for consistency... but there is no IDL that would use it, and
> it would not be tested at all. So I just omitted it.

Okay, let's add a comment.

[dcheng, 2016-12-09 05:06:14]

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
(right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:199: return
false; // the frame is gone.
On 2016/12/09 02:26:55, haraken wrote:
> On 2016/12/08 09:04:11, dcheng wrote:
> > On 2016/12/08 08:21:02, haraken wrote:
> > > 
> > > Not really. window.IsEmpty happens when the prototype chain is tweaked.
> > Whether
> > > the frame is gone is checked by the following
> > > BindingSecurity::shouldAllowAccessTo.
> > > 
> > 
> > I'm just copying and pasting code here =)
> > 
> > What would you suggest as a more accurate comment?
> 
> "It's not a window wrapper" :)

Strictly speaking, is that true though? We only set access check callbacks for
Window and Location, and this code is specific for handling Window. So it must
be a window wrapper, but somehow got disassociated by changing the prototype
chain.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:230:
{"{{method.name}}",
&{{cpp_class}}V8Internal::{{method.name}}OriginSafeMethodGetter, nullptr},
On 2016/12/09 02:26:56, haraken wrote:
> On 2016/12/08 09:04:11, dcheng wrote:
> > On 2016/12/08 08:21:02, haraken wrote:
> > > 
> > > After this CL, OriginSafeMethodSetter is no longer used, right? If so, can
> we
> > > remove code for OriginSafeMethodSetter?
> > 
> > Unfortunately, it is still needed. Originally, I tried removing this, and
this
> > broke things like postMessage: event.source would be set incorrectly.
> > 
> > postMessage and the location setters need the calling context (incumbent
realm
> > in spec terminology): they use this to determine if the calling context has
> > access to the target context. Since v8 is trying to get rid of
> > GetCallingContext(), we need to support this another way: instead, we create
> > function objects for each realm, so that when we call GetCurrentContext(),
it
> > ends up pointing to the context which we created the function object for.
> > 
> > With this hack, if document A post messages document B, GetCurrentContext()
==
> > document A, and we enforce security checks / set event.source correctly,
etc.
> > 
> > However, if we do not create a per-realm function object, then
> > GetCurrentContext() == the context we are calling *into*: that means that
when
> > document A post messages document B, GetCurrentContext() == document B. This
> > breaks security checks for Location, as well as event.source (since
> event.source
> > doesn't point back at the caller).
> 
> Yes, I understand why we still need to keep OriginSafeMethodGetter. My
question
> is if we still need OriginSafeMethodSetter.

I thought that this would be needed so that if JS overwrites the property, we
can return the user-supplied value rather than the function object. I might be
missing something, but I don't see how I'd be able to make this work without a
setter.

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:274: 
On 2016/12/09 02:26:56, haraken wrote:
> On 2016/12/08 09:04:11, dcheng wrote:
> > On 2016/12/08 08:21:02, haraken wrote:
> > > 
> > > Why don't you need to call V8Internal::namedPropertySetter when
> > > named_property_setter.is_cross_origin is set?
> > 
> > I could add this for consistency... but there is no IDL that would use it,
and
> > it would not be tested at all. So I just omitted it.
> 
> Okay, let's add a comment.

Done.

[haraken, 2016-12-09 05:48:18]

Still LGTM

[Yuki, 2016-12-09 07:02:49]

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:237:
ExceptionState exceptionState(ExceptionState::UnknownContext, 0, 0,
On 2016/12/08 09:04:10, dcheng wrote:
> On 2016/12/08 08:21:02, haraken wrote:
> > On 2016/12/07 12:03:40, Yuki wrote:
> > > ExceptionState should be created in the call sites, where interface name,
> etc.
> > > are clear.  It's better to be close as much as possible to the point that
> the
> > > execution enters into Blink from V8.
> > > 
> > > It's also not good that we actually throw a v8::Exception here because V8
> APIs
> > > no longer work correctly having an exception.  It's better to *schedule*
an
> > > exception in the ExceptionState (with throwSecurityError) but not actually
> > throw
> > > it yet (the dtor of ExceptionState actually throws).
> > > 
> > > e.g.
> > >     void foo() {
> > >       BindingSecurity::failedAccessCheckFor(...);
> > >     }
> > >     void bar() { foo(); }
> > >     void baz() {
> > >       bar();
> > >       do_something();  // do something with V8 APIs, but V8 APIs do
nothing
> > > because an exception is thrown.
> > >     }
> > > It's very hard to understand why do_something() doesn't work in a specific
> > case
> > > just seeing baz().
> > > 
> > > We'd like the following coding pattern.
> > >     void foo(ExceptionState& exceptionState) {
> > >       BindingSecurity::failedAccessCheckFor(...);
> > >     }
> > >     void bar(ExceptionState& exceptionState) {
> > >       foo(exceptionState);
> > >     }
> > >     void baz(ExceptionState& exceptionState) {
> > >       bar(exceptionState);
> > >       if (exceptionState.hadException()) {
> > >         return handle_error();
> > >       }
> > >       do_something();
> > >     }
> > > Then, it's a caller's duty to check exceptionState.hadException().
> > > 
> > > nit: Please use non-deprecated version of ExceptionState's ctors.  We no
> > longer
> > > need isolate->GetCurrentContext()->Global().
> > 
> > What do you mean? Note that BindingSecurity::failedAccessCheckFor is called
by
> > V8 (via failedAccessCheckCallbackInMainThread or window's interceptor), not
by
> > some method in Blink. I think it's just fine to throw an exception here.

Both of failedAccessCheckCallbackInMainThread and window's interceptor are
Blink's functions, I think?  And it's not easy to guarantee that the call sites
return to V8 right after this function call.

In general, I think it's hard to track a *hidden* exception thrown in Blink
because most of code in Blink doesn't care about a possible exception unlike V8
(we don't have many returnIfAbruptCompletion).  If it does care about a possible
exception, then it should use an ExceptionState, and the ExceptionState should
be instantiated right after the entry from V8 to Blink.

As same as BindingSecurity::shouldAllowAccessTo, the call sites should
instantiate an ExceptionState with appropriate interface name, method/attribute
name, context, etc.


> I moved this around to make it so the interceptors can invoke this helper as
> well. Originally, I didn't expose this at all, but this leads to a problem:
> 
> A named getter interceptor can signal an access failure just by not setting a
> return value. v8 will notice that there was no value returned, and call the
> access check failed callback
> 
> However, a named setter interceptor can't easily signal failure. So it needs
to
> throw an exception. To make the implementation of the interceptors in Blink
> consistent, I have them consistently throw exceptions on access check
failures.
> 
> In a followup patch, we can actually improve the error messages further: since
> we know the context in which it's being invoked, we can instantiate
> ExceptionState with the interface name, property name, and the access type:
the
> net result should be that our SecurityErrors will be more useful afterwards.

dcheng@, could you add a TODO comment at least for this point if you cannot make
it in this CL?  I'd recommend to do it in this CL since it shouldn't be so hard,
though.

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

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:492: v8 doesn't
support querying the incumbent context. For now, always
On 2016/12/08 09:04:10, dcheng wrote:
> On 2016/12/07 12:03:40, Yuki wrote:
> > "incumbent realm" is an idea of HTML, hence it should be implemented in
Blink,
> I
> > think.  It's not V8's fault.
> 
> It's awkward to implement in Blink though: according to domenic@, we should
not
> be creating per-realm function objects except for cross-origin accesses.
> 
> If we're OK with keeping this behavior in the long-term, I will just change
the
> comments to explain the implementation strategy. WDYT?

I'm not saying that it's okay to create per-realm function objects
(originSafeMethodGetter), but I'm saying that Blink may/should be responsible to
support a way to control "incumbent realm" because it's out of scope of
ECMAScript.  "incumbent realm" is defined in HTML spec.

I'm saying that Blink should support "incumbent realm" on Blink side and remove
the ugly hack of originSafeMethodGetter.  It's possible that Blink needs some
help from V8, but in general, HTML things should be handled in Blink, not in V8,
I think.

I think/suppose that, once Blink supports "incumbent realm", we can implement
postMessage in a correct way using incumbent realm and we no longer need
originSafeMethodGetter.

[dcheng, 2016-12-09 07:35:20]

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:237:
ExceptionState exceptionState(ExceptionState::UnknownContext, 0, 0,
On 2016/12/09 07:02:49, Yuki wrote:
> On 2016/12/08 09:04:10, dcheng wrote:
> > On 2016/12/08 08:21:02, haraken wrote:
> > > On 2016/12/07 12:03:40, Yuki wrote:
> > > > ExceptionState should be created in the call sites, where interface
name,
> > etc.
> > > > are clear.  It's better to be close as much as possible to the point
that
> > the
> > > > execution enters into Blink from V8.
> > > > 
> > > > It's also not good that we actually throw a v8::Exception here because
V8
> > APIs
> > > > no longer work correctly having an exception.  It's better to *schedule*
> an
> > > > exception in the ExceptionState (with throwSecurityError) but not
actually
> > > throw
> > > > it yet (the dtor of ExceptionState actually throws).
> > > > 
> > > > e.g.
> > > >     void foo() {
> > > >       BindingSecurity::failedAccessCheckFor(...);
> > > >     }
> > > >     void bar() { foo(); }
> > > >     void baz() {
> > > >       bar();
> > > >       do_something();  // do something with V8 APIs, but V8 APIs do
> nothing
> > > > because an exception is thrown.
> > > >     }
> > > > It's very hard to understand why do_something() doesn't work in a
specific
> > > case
> > > > just seeing baz().
> > > > 
> > > > We'd like the following coding pattern.
> > > >     void foo(ExceptionState& exceptionState) {
> > > >       BindingSecurity::failedAccessCheckFor(...);
> > > >     }
> > > >     void bar(ExceptionState& exceptionState) {
> > > >       foo(exceptionState);
> > > >     }
> > > >     void baz(ExceptionState& exceptionState) {
> > > >       bar(exceptionState);
> > > >       if (exceptionState.hadException()) {
> > > >         return handle_error();
> > > >       }
> > > >       do_something();
> > > >     }
> > > > Then, it's a caller's duty to check exceptionState.hadException().
> > > > 
> > > > nit: Please use non-deprecated version of ExceptionState's ctors.  We no
> > > longer
> > > > need isolate->GetCurrentContext()->Global().
> > > 
> > > What do you mean? Note that BindingSecurity::failedAccessCheckFor is
called
> by
> > > V8 (via failedAccessCheckCallbackInMainThread or window's interceptor),
not
> by
> > > some method in Blink. I think it's just fine to throw an exception here.
> 
> Both of failedAccessCheckCallbackInMainThread and window's interceptor are
> Blink's functions, I think?  And it's not easy to guarantee that the call
sites
> return to V8 right after this function call.

That's correct. However, if an interceptor is installed, v8 will not call the
failedAccessCheckCallbackInMainThread unless:
- That type (getter, setter, enumerator, etc) of interceptor is not installed.
- If that interceptor is installed *and* appeared to do nothing (which is
impossible to determine for setters) *and* threw no exceptions.

> 
> In general, I think it's hard to track a *hidden* exception thrown in Blink
> because most of code in Blink doesn't care about a possible exception unlike
V8
> (we don't have many returnIfAbruptCompletion).  If it does care about a
possible
> exception, then it should use an ExceptionState, and the ExceptionState should
> be instantiated right after the entry from V8 to Blink.
> 
> As same as BindingSecurity::shouldAllowAccessTo, the call sites should
> instantiate an ExceptionState with appropriate interface name,
method/attribute
> name, context, etc.
> 

I agree with all this. But the reason this is in a shared helper is so that the
SecurityError exception can be generated consistently for the cross-origin
interceptor path: this is only used to generate errors for that specific case. I
think it is better than duplicating it into the generated bindings. Otherwise,
things like
https://codereview.chromium.org/2439013002/diff/480001/third_party/WebKit/Sou...
would have to build the exception manually. I'm happy to discuss this more
offline, if that would help clarify things.

> 
> > I moved this around to make it so the interceptors can invoke this helper as
> > well. Originally, I didn't expose this at all, but this leads to a problem:
> > 
> > A named getter interceptor can signal an access failure just by not setting
a
> > return value. v8 will notice that there was no value returned, and call the
> > access check failed callback
> > 
> > However, a named setter interceptor can't easily signal failure. So it needs
> to
> > throw an exception. To make the implementation of the interceptors in Blink
> > consistent, I have them consistently throw exceptions on access check
> failures.
> > 
> > In a followup patch, we can actually improve the error messages further:
since
> > we know the context in which it's being invoked, we can instantiate
> > ExceptionState with the interface name, property name, and the access type:
> the
> > net result should be that our SecurityErrors will be more useful afterwards.
> 
> dcheng@, could you add a TODO comment at least for this point if you cannot
make
> it in this CL?  I'd recommend to do it in this CL since it shouldn't be so
hard,
> though.

I will add a TODO: it's not hard, but I'd like to minimize the number of
behavior changes in this CL, since it aims to reimplement our bindings for
cross-origin things.

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

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:492: v8 doesn't
support querying the incumbent context. For now, always
On 2016/12/09 07:02:49, Yuki wrote:
> On 2016/12/08 09:04:10, dcheng wrote:
> > On 2016/12/07 12:03:40, Yuki wrote:
> > > "incumbent realm" is an idea of HTML, hence it should be implemented in
> Blink,
> > I
> > > think.  It's not V8's fault.
> > 
> > It's awkward to implement in Blink though: according to domenic@, we should
> not
> > be creating per-realm function objects except for cross-origin accesses.
> > 
> > If we're OK with keeping this behavior in the long-term, I will just change
> the
> > comments to explain the implementation strategy. WDYT?
> 
> I'm not saying that it's okay to create per-realm function objects
> (originSafeMethodGetter), but I'm saying that Blink may/should be responsible
to
> support a way to control "incumbent realm" because it's out of scope of
> ECMAScript.  "incumbent realm" is defined in HTML spec.
> 
> I'm saying that Blink should support "incumbent realm" on Blink side and
remove
> the ugly hack of originSafeMethodGetter.  It's possible that Blink needs some
> help from V8, but in general, HTML things should be handled in Blink, not in
V8,
> I think.
> 
> I think/suppose that, once Blink supports "incumbent realm", we can implement
> postMessage in a correct way using incumbent realm and we no longer need
> originSafeMethodGetter.

Ah, I understand. I've updated the comment to talk specifically about supporting
the incumbent realm. WDYT?

[Yuki, 2016-12-09 08:40:23]

LGTM.

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

https://codereview.chromium.org/2439013002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:492: v8 doesn't
support querying the incumbent context. For now, always
On 2016/12/09 07:35:20, dcheng wrote:
> On 2016/12/09 07:02:49, Yuki wrote:
> > On 2016/12/08 09:04:10, dcheng wrote:
> > > On 2016/12/07 12:03:40, Yuki wrote:
> > > > "incumbent realm" is an idea of HTML, hence it should be implemented in
> > Blink,
> > > I
> > > > think.  It's not V8's fault.
> > > 
> > > It's awkward to implement in Blink though: according to domenic@, we
should
> > not
> > > be creating per-realm function objects except for cross-origin accesses.
> > > 
> > > If we're OK with keeping this behavior in the long-term, I will just
change
> > the
> > > comments to explain the implementation strategy. WDYT?
> > 
> > I'm not saying that it's okay to create per-realm function objects
> > (originSafeMethodGetter), but I'm saying that Blink may/should be
responsible
> to
> > support a way to control "incumbent realm" because it's out of scope of
> > ECMAScript.  "incumbent realm" is defined in HTML spec.
> > 
> > I'm saying that Blink should support "incumbent realm" on Blink side and
> remove
> > the ugly hack of originSafeMethodGetter.  It's possible that Blink needs
some
> > help from V8, but in general, HTML things should be handled in Blink, not in
> V8,
> > I think.
> > 
> > I think/suppose that, once Blink supports "incumbent realm", we can
implement
> > postMessage in a correct way using incumbent realm and we no longer need
> > originSafeMethodGetter.
> 
> Ah, I understand. I've updated the comment to talk specifically about
supporting
> the incumbent realm. WDYT?

I like the new comment.  Thanks.  :)

[jochen (gone - plz use gerrit), 2016-12-09 08:41:30]

rubberstamp lgtm

[dcheng, 2016-12-09 08:44:07]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-12-09 08:44:07]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2439013002/#ps500001
(title: ".")

[commit-bot: I haz the power, 2016-12-09 08:44:22]

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-12-09 08:52:11]

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

[commit-bot: I haz the power, 2016-12-09 08:52:12]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dcheng, 2016-12-09 08:56:00]

On 2016/12/09 08:52:12, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Any idea why run-binding-tests is failing? The error output is not useful:

** Presubmit ERRORS **
run-bindings-tests (0.70s) failed
No tests to run.

And I can't repro locally either.

[dcheng, 2016-12-09 09:30:01]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-12-09 09:30:19]

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-12-09 09:37:41]

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

[commit-bot: I haz the power, 2016-12-09 09:37:42]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jochen (gone - plz use gerrit), 2016-12-09 15:12:42]

does git cl presubmit work locally?

[Yuki, 2016-12-09 15:16:29]

On 2016/12/09 15:12:42, jochen wrote:
> does git cl presubmit work locally?

I think we're hitting the following issue.
https://bugs.chromium.org/p/chromium/issues/detail?id=672738

[Yuki, 2016-12-09 15:20:33]

On 2016/12/09 15:16:29, Yuki wrote:
> On 2016/12/09 15:12:42, jochen wrote:
> > does git cl presubmit work locally?
> 
> I think we're hitting the following issue.
> https://bugs.chromium.org/p/chromium/issues/detail?id=672738

Could you do bypass-presubmit for the time being if possible?
Otherwise, revert crrev.com/2554503004 .
(I'm sorry that I need to run into my last train and I cannot fix it for myself
right now.)

[dcheng, 2016-12-09 16:16:44]

On 2016/12/09 15:20:33, Yuki wrote:
> On 2016/12/09 15:16:29, Yuki wrote:
> > On 2016/12/09 15:12:42, jochen wrote:
> > > does git cl presubmit work locally?
> > 
> > I think we're hitting the following issue.
> > https://bugs.chromium.org/p/chromium/issues/detail?id=672738
> 
> Could you do bypass-presubmit for the time being if possible?
> Otherwise, revert crrev.com/2554503004 .
> (I'm sorry that I need to run into my last train and I cannot fix it for
myself
> right now.)

No problem! I will just git cl land, since this has passed the CQ bots (other
than presubmit) anyway.

[dcheng, 2016-12-09 16:18:52]

On 2016/12/09 15:12:42, jochen wrote:
> does git cl presubmit work locally?

It does, but I guess I didn't have the CL that causes this to break locally.

[commit-bot: I haz the power, 2016-12-09 16:22:22]

Description was changed from

==========
Implement cross-origin attributes using access check interceptors.

BUG=618305
==========

to

==========
Implement cross-origin attributes using access check interceptors.

BUG=618305
R=haraken@chromium.org, jochen@chromium.org, yukishiino@chromium.org

Committed: https://crrev.com/3387c1777b355f2da7a02ad1fc833745f0589915
Cr-Commit-Position: refs/heads/master@{#437554}
==========

[commit-bot: I haz the power, 2016-12-09 16:22:24]

Patchset 25 (id:??) landed as
https://crrev.com/3387c1777b355f2da7a02ad1fc833745f0589915
Cr-Commit-Position: refs/heads/master@{#437554}

[dcheng, 2016-12-09 16:24:08]

Description was changed from

==========
Implement cross-origin attributes using access check interceptors.

BUG=618305
R=haraken@chromium.org, jochen@chromium.org, yukishiino@chromium.org

Committed: https://crrev.com/3387c1777b355f2da7a02ad1fc833745f0589915
Cr-Commit-Position: refs/heads/master@{#437554}
==========

to

==========
Implement cross-origin attributes using access check interceptors.

BUG=618305
R=haraken@chromium.org, jochen@chromium.org, yukishiino@chromium.org

Committed:
https://chromium.googlesource.com/chromium/src/+/3387c1777b355f2da7a02ad1fc83...
==========

[dcheng, 2016-12-09 16:24:10]

Committed patchset #25 (id:500001) manually as
3387c1777b355f2da7a02ad1fc833745f0589915 (presubmit successful).