Use the current context as the creation context for cross-origin objects.

Use NewRemoteInstance() to create cross-origin objects.

Also use the current context as the creation context when
returning the cross-origin named enumerator's results.

BUG=682822
Review-Url: https://codereview.chromium.org/2640123006
Cr-Commit-Position: refs/heads/master@{#446635}
Committed: https://chromium.googlesource.com/chromium/src/+/9ead0def6f12d7152260f2eb21e78c7e09cd47ee

[dcheng, 2017-01-19 23:26:45]

Description was changed from

==========
Use the current context as the creation context for cross-origin objects.

BUG=682822
==========

to

==========
Use the current context as the creation context for cross-origin objects.

Do not land, as this currently allows leaks between cross-origin contexts.

BUG=682822
==========

[dcheng, 2017-01-19 23:26:45]

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

[dcheng, 2017-01-19 23:34:07]

https://codereview.chromium.org/2640123006/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ToV8.cpp (right):

https://codereview.chromium.org/2640123006/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ToV8.cpp:59: }
Modifying ToV8 was my original approach, but it has the problem that we only
cache one wrapper per-world. This means if a cross-origin context touches this
property first, it will be created with creation context == cross-origin
context, which has surprising affects.

To get around this, I started modifying the bindings (the currently uploaded
PS). However, it requires a bunch of code duplication, and I don't really like
it.

Instead, I want to try to add an extra bool parameter called is_cross_origin,
and gate behavior based on that bool. However, this means that we need to pass
an extra parameter for attributes that are exposed cross-origin. When
is_cross_origin is true, we will use a different creation context, and cache the
wrappers differently. I am also not sure if it will affect performance too much,
but it seems like it'll be the easiest to understand... does this last approach
sound OK?

[Yuki, 2017-01-23 08:22:18]

I don't think that we need to support cross-origin representations with ToV8
because the use case is very limited.  I think there should be very limited call
sites and it should be okay to provide a dedicated function ToV8CrossOrigin for
example, without using ToV8.

I don't think we need to make |window.location| very efficient (especially
cross-origin case).  So, we don't need to support
|attribute.is_keep_alive_for_gc| for example.

Given that cross-origin representation is so special and |window.location| is
only the use case, I think it's okay to do hard-code everything necessary there.
 And, in this way, we can use ToV8CrossOrigin (or ToCrossOriginRepresentation)
without using ToV8.

[dcheng, 2017-01-23 08:22:58]

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

[commit-bot: I haz the power, 2017-01-23 08:23:12]

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

[dcheng, 2017-01-23 08:23:34]

Description was changed from

==========
Use the current context as the creation context for cross-origin objects.

Do not land, as this currently allows leaks between cross-origin contexts.

BUG=682822
==========

to

==========
Use NewRemoteInstance() to create cross-origin objects.

BUG=682822
==========

[dcheng, 2017-01-23 08:24:49]

Description was changed from

==========
Use NewRemoteInstance() to create cross-origin objects.

BUG=682822
==========

to

==========
Use NewRemoteInstance() to create cross-origin objects.

Also use the current context as the creation context when
returning the cross-origin named enumerator's results.

BUG=682822
==========

[dcheng, 2017-01-23 08:27:01]

On 2017/01/23 08:22:18, Yuki wrote:
> I don't think that we need to support cross-origin representations with ToV8
> because the use case is very limited.  I think there should be very limited
call
> sites and it should be okay to provide a dedicated function ToV8CrossOrigin
for
> example, without using ToV8.

I just finished tweaking a CL that adds a generic path for remote objects =P
Let me see what it looks like if I change it to hardcode for Location.

> 
> I don't think we need to make |window.location| very efficient (especially
> cross-origin case).  So, we don't need to support
> |attribute.is_keep_alive_for_gc| for example.
> 

My only concern about this is if it's important to support
crossOriginWindow.location === crossOriginWindow.location.

> Given that cross-origin representation is so special and |window.location| is
> only the use case, I think it's okay to do hard-code everything necessary
there.
>  And, in this way, we can use ToV8CrossOrigin (or ToCrossOriginRepresentation)
> without using ToV8.

My major concern about this is supporting wrapper tracing. I thought it would be
easier if I integrated into the existing systems, but as you can see, I have
some TODOs regarding the proper way to support setReference and markWrapper...
=(

[Yuki, 2017-01-23 08:35:10]

I'm sorry for my late response.

On 2017/01/23 08:27:01, dcheng wrote:
> My only concern about this is if it's important to support
> crossOriginWindow.location === crossOriginWindow.location.

This is really important.  I wrote my idea how to achieve this at
https://crbug.com/682822 .


> My major concern about this is supporting wrapper tracing. I thought it would
be
> easier if I integrated into the existing systems, but as you can see, I have
> some TODOs regarding the proper way to support setReference and markWrapper...
> =(

As I wrote in the bug, I think that the way that fooOriginSafeMethodGetter is
currently using would work just fine.  What we need to do is to associate the
wrapper object with a C++ instance in Blink using internal fields, and then, let
V8 GC do the rest things.


Having said that, I'm open to other options (hope they are simple).

[commit-bot: I haz the power, 2017-01-23 10:04:47]

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

[commit-bot: I haz the power, 2017-01-23 10:04:48]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-01-24 08:57:28]

On 2017/01/23 08:35:10, Yuki wrote:
> I'm sorry for my late response.
> 
> On 2017/01/23 08:27:01, dcheng wrote:
> > My only concern about this is if it's important to support
> > crossOriginWindow.location === crossOriginWindow.location.
> 
> This is really important.  I wrote my idea how to achieve this at
> https://crbug.com/682822 .
> 
> 
> > My major concern about this is supporting wrapper tracing. I thought it
would
> be
> > easier if I integrated into the existing systems, but as you can see, I have
> > some TODOs regarding the proper way to support setReference and
markWrapper...
> > =(
> 
> As I wrote in the bug, I think that the way that fooOriginSafeMethodGetter is
> currently using would work just fine.  What we need to do is to associate the
> wrapper object with a C++ instance in Blink using internal fields, and then,
let
> V8 GC do the rest things.

We still need to hold the persistent somewhere, so that we can mark it for
Oilpan to trace, right?

> 
> 
> Having said that, I'm open to other options (hope they are simple).

Just for documentation, I did some more local investigation of using
NewRemoteInstance(). The basic idea of PS3 is to generate one cross-origin
wrapper and share it among all frames. However, this fails the
cross-origin-objects.html layout test. What the test does is this:

var cachedLocation = parent.location;
document.domain = 'something';
assert_equals(cachedLocation, parent.location);

And the assert fails, since the cross-origin wrapper and the original wrapper
are obviously not the same object.

I will try implementing the GetFunction approach now. I'm still concerned that
this approach might fail the cross-origin-objects.html test: I'm actually not
100% sure how PS2 passes this test...

[dcheng, 2017-01-24 09:37:00]

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

[commit-bot: I haz the power, 2017-01-24 09:37:11]

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

[dcheng, 2017-01-24 09:40:46]

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

[dcheng, 2017-01-24 09:40:47]

Actually, PTAL. I did a small update (PS3 -> PS4) and I think this should
actually work fine.

https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
(right):

https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:81: if
(window->isRemoteDOMWindow()) {
Actually, I think with this small update, this will actually work fine. It's OK
for window.location != window.location if window is navigated, so this ensures
the wrapper is always consistent within the lifetime of the window itself (not
the window proxy).

https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Location.cpp (right):

https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Location.cpp:127: ->domTemplate(isolate,
DOMWrapperWorld::world(
However, this makes me nervous. Is there a way to get a dom template that's not
associated with a world?

https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Location.cpp:129: ->NewRemoteInstance()
+jochen

Sharing a remote instance like this should be safe, right? The access checks
should prevent access to anything 'interesting'. The main thing I'm worried
about is things in the prototype chain leaking between worlds / contexts, but I
think NewRemoteInstance() should be designed with that in mind, since it doesn't
use the current context?

[Yuki, 2017-01-24 09:41:17]

On 2017/01/24 08:57:28, dcheng wrote:
> We still need to hold the persistent somewhere, so that we can mark it for
> Oilpan to trace, right?

I think we don't need a persistent.  Oilpan asks V8 currently-living-wrappers at
the beginning, and Oilpan marks Blink objects pointed to by internal fields in
the wrappers.  IIUC, it makes the location object in Blink alive.  And there
should be no need to have a persistent.

[Yuki, 2017-01-24 10:31:05]

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

https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:300:
info.GetIsolate()).As<v8::Array>());
Why do we need .As<v8::Array>()?  I think we don't need it.

https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Location.cpp (right):

https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Location.cpp:123: v8::Local<v8::Object>
Location::crossOriginWrapper(v8::Isolate* isolate) {
I guess it's better to rename this function to remoteFrameWrapper?

https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Location.cpp:127: ->domTemplate(isolate,
DOMWrapperWorld::world(
On 2017/01/24 09:40:47, dcheng wrote:
> However, this makes me nervous. Is there a way to get a dom template that's
not
> associated with a world?

No, because we have main-world-specific optimization in v8::FunctionTempalte and
its prototype template.  And this seems incorrect that we're going to use the
same v8::Object between the main world and isolated worlds.  We're going to
assume "it's in the main world" even if it's in an isolated world.

Do we really need to re-use the existing Location::wrapperTypeInfo() here?  Is
it okay to define a new WrapperTypeInfo dedicated for remote frames?

https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Location.cpp:129: ->NewRemoteInstance()
On 2017/01/24 09:40:47, dcheng wrote:
> +jochen
> 
> Sharing a remote instance like this should be safe, right? The access checks
> should prevent access to anything 'interesting'. The main thing I'm worried
> about is things in the prototype chain leaking between worlds / contexts, but
I
> think NewRemoteInstance() should be designed with that in mind, since it
doesn't
> use the current context?

If it's okay, then I'd like to have a comment here to say that it's okay to
share the same v8::Object among contexts of remote frames.

[dcheng, 2017-01-24 10:41:59]

I'll upload another PS tomorrow using the GetFunction() per-context caching
approach.

https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Location.cpp (right):

https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Location.cpp:127: ->domTemplate(isolate,
DOMWrapperWorld::world(
On 2017/01/24 10:31:04, Yuki wrote:
> On 2017/01/24 09:40:47, dcheng wrote:
> > However, this makes me nervous. Is there a way to get a dom template that's
> not
> > associated with a world?
> 
> No, because we have main-world-specific optimization in v8::FunctionTempalte
and
> its prototype template.  And this seems incorrect that we're going to use the
> same v8::Object between the main world and isolated worlds.  We're going to
> assume "it's in the main world" even if it's in an isolated world.
> 
> Do we really need to re-use the existing Location::wrapperTypeInfo() here?  Is
> it okay to define a new WrapperTypeInfo dedicated for remote frames?

Can you point me at the main world optimizations? I'm curious to see if it will
affect the correctness of doing this. I saw several in things like DOMDataStore,
but I want to make sure I'm looking at the right ones.

[Yuki, 2017-01-24 10:53:00]

https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Location.cpp (right):

https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Location.cpp:127: ->domTemplate(isolate,
DOMWrapperWorld::world(
On 2017/01/24 10:41:59, dcheng wrote:
> On 2017/01/24 10:31:04, Yuki wrote:
> > On 2017/01/24 09:40:47, dcheng wrote:
> > > However, this makes me nervous. Is there a way to get a dom template
that's
> > not
> > > associated with a world?
> > 
> > No, because we have main-world-specific optimization in v8::FunctionTempalte
> and
> > its prototype template.  And this seems incorrect that we're going to use
the
> > same v8::Object between the main world and isolated worlds.  We're going to
> > assume "it's in the main world" even if it's in an isolated world.
> > 
> > Do we really need to re-use the existing Location::wrapperTypeInfo() here? 
Is
> > it okay to define a new WrapperTypeInfo dedicated for remote frames?
> 
> Can you point me at the main world optimizations? I'm curious to see if it
will
> affect the correctness of doing this. I saw several in things like
DOMDataStore,
> but I want to make sure I'm looking at the right ones.

I meant [PerWorldBindings] extended attribute, but Location.idl seems not having
it.  So probably it actually works.  But it's unfortunate to ignore worlds.

If you support things in handlers, I guess you wouldn't need much contents in
v8::FunctionTemplate?

If this approach works fine, I'm not insisting to
v8::FunctionTemplate::GetFunction.

[commit-bot: I haz the power, 2017-01-24 11:50:28]

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

[commit-bot: I haz the power, 2017-01-24 11:50:28]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-01-24 19:15:40]

On 2017/01/24 10:53:00, Yuki wrote:
>
https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/Location.cpp (right):
> 
>
https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/Location.cpp:127: ->domTemplate(isolate,
> DOMWrapperWorld::world(
> On 2017/01/24 10:41:59, dcheng wrote:
> > On 2017/01/24 10:31:04, Yuki wrote:
> > > On 2017/01/24 09:40:47, dcheng wrote:
> > > > However, this makes me nervous. Is there a way to get a dom template
> that's
> > > not
> > > > associated with a world?
> > > 
> > > No, because we have main-world-specific optimization in
v8::FunctionTempalte
> > and
> > > its prototype template.  And this seems incorrect that we're going to use
> the
> > > same v8::Object between the main world and isolated worlds.  We're going
to
> > > assume "it's in the main world" even if it's in an isolated world.
> > > 
> > > Do we really need to re-use the existing Location::wrapperTypeInfo() here?

> Is
> > > it okay to define a new WrapperTypeInfo dedicated for remote frames?
> > 
> > Can you point me at the main world optimizations? I'm curious to see if it
> will
> > affect the correctness of doing this. I saw several in things like
> DOMDataStore,
> > but I want to make sure I'm looking at the right ones.
> 
> I meant [PerWorldBindings] extended attribute, but Location.idl seems not
having
> it.  So probably it actually works.  But it's unfortunate to ignore worlds.
> 
> If you support things in handlers, I guess you wouldn't need much contents in
> v8::FunctionTemplate?
> 
> If this approach works fine, I'm not insisting to
> v8::FunctionTemplate::GetFunction.

OK, jochen points out something interesting: sharing the v8 Object between
worlds / contexts could lead to some info leaks by setting the location object
as a key of a WeakMap.
Thinking about this more, I think it's actually already an issue (multiple
contexts will reference the same wrapper Location object), but I'm going to
revert to having per-world instances to avoid having this leak cross-world as
well.

[haraken, 2017-01-24 21:32:52]

On 2017/01/24 19:15:40, dcheng wrote:
> On 2017/01/24 10:53:00, Yuki wrote:
> >
>
https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/frame/Location.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/frame/Location.cpp:127:
->domTemplate(isolate,
> > DOMWrapperWorld::world(
> > On 2017/01/24 10:41:59, dcheng wrote:
> > > On 2017/01/24 10:31:04, Yuki wrote:
> > > > On 2017/01/24 09:40:47, dcheng wrote:
> > > > > However, this makes me nervous. Is there a way to get a dom template
> > that's
> > > > not
> > > > > associated with a world?
> > > > 
> > > > No, because we have main-world-specific optimization in
> v8::FunctionTempalte
> > > and
> > > > its prototype template.  And this seems incorrect that we're going to
use
> > the
> > > > same v8::Object between the main world and isolated worlds.  We're going
> to
> > > > assume "it's in the main world" even if it's in an isolated world.
> > > > 
> > > > Do we really need to re-use the existing Location::wrapperTypeInfo()
here?
> 
> > Is
> > > > it okay to define a new WrapperTypeInfo dedicated for remote frames?
> > > 
> > > Can you point me at the main world optimizations? I'm curious to see if it
> > will
> > > affect the correctness of doing this. I saw several in things like
> > DOMDataStore,
> > > but I want to make sure I'm looking at the right ones.
> > 
> > I meant [PerWorldBindings] extended attribute, but Location.idl seems not
> having
> > it.  So probably it actually works.  But it's unfortunate to ignore worlds.
> > 
> > If you support things in handlers, I guess you wouldn't need much contents
in
> > v8::FunctionTemplate?
> > 
> > If this approach works fine, I'm not insisting to
> > v8::FunctionTemplate::GetFunction.
> 
> OK, jochen points out something interesting: sharing the v8 Object between
> worlds / contexts could lead to some info leaks by setting the location object
> as a key of a WeakMap.

Correct, any v8 object must not leak between worlds.

> Thinking about this more, I think it's actually already an issue (multiple
> contexts will reference the same wrapper Location object),

So if this is happening already, it should be a serious security issue. It means
that, for example, there is a way to exploit a window object of the main world
from a Chrome extension.

> but I'm going to
> revert to having per-world instances to avoid having this leak cross-world as
> well.

[dcheng, 2017-01-24 21:52:58]

On 2017/01/24 21:32:52, haraken wrote:
> On 2017/01/24 19:15:40, dcheng wrote:
> > On 2017/01/24 10:53:00, Yuki wrote:
> > >
> >
>
https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/frame/Location.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/frame/Location.cpp:127:
> ->domTemplate(isolate,
> > > DOMWrapperWorld::world(
> > > On 2017/01/24 10:41:59, dcheng wrote:
> > > > On 2017/01/24 10:31:04, Yuki wrote:
> > > > > On 2017/01/24 09:40:47, dcheng wrote:
> > > > > > However, this makes me nervous. Is there a way to get a dom template
> > > that's
> > > > > not
> > > > > > associated with a world?
> > > > > 
> > > > > No, because we have main-world-specific optimization in
> > v8::FunctionTempalte
> > > > and
> > > > > its prototype template.  And this seems incorrect that we're going to
> use
> > > the
> > > > > same v8::Object between the main world and isolated worlds.  We're
going
> > to
> > > > > assume "it's in the main world" even if it's in an isolated world.
> > > > > 
> > > > > Do we really need to re-use the existing Location::wrapperTypeInfo()
> here?
> > 
> > > Is
> > > > > it okay to define a new WrapperTypeInfo dedicated for remote frames?
> > > > 
> > > > Can you point me at the main world optimizations? I'm curious to see if
it
> > > will
> > > > affect the correctness of doing this. I saw several in things like
> > > DOMDataStore,
> > > > but I want to make sure I'm looking at the right ones.
> > > 
> > > I meant [PerWorldBindings] extended attribute, but Location.idl seems not
> > having
> > > it.  So probably it actually works.  But it's unfortunate to ignore
worlds.
> > > 
> > > If you support things in handlers, I guess you wouldn't need much contents
> in
> > > v8::FunctionTemplate?
> > > 
> > > If this approach works fine, I'm not insisting to
> > > v8::FunctionTemplate::GetFunction.
> > 
> > OK, jochen points out something interesting: sharing the v8 Object between
> > worlds / contexts could lead to some info leaks by setting the location
object
> > as a key of a WeakMap.
> 
> Correct, any v8 object must not leak between worlds.

I think it's actually safe for NewRemoteInstance(), but it's not much harder to
write a per-world version. I'm uploading a CL for that shortly.

> 
> > Thinking about this more, I think it's actually already an issue (multiple
> > contexts will reference the same wrapper Location object),
> 
> So if this is happening already, it should be a serious security issue. It
means
> that, for example, there is a way to exploit a window object of the main world
> from a Chrome extension.

Let me clarify what I mean. Consider Location. Any given Location object can
have only one main world wrapper associated it. However, Location is accessible
cross-origin, so origin B and origin C can get a reference to originA.location.

Let's see origin B and origin C use originA.location as a key in a WeakMap. Then
we navigate originA somewhere, so the original inner window / location are no
longer active. Once the hidden value is overwritten with the new wrapper for
Location, originA.location is eligible for garbage collection.

At this point, origin B and origin C can observe if the key in the WeakMap is
collected or not (and if it isn't, they know someone else must be holding a
reference to it and keeping it alive). Does that make sense? This is in an info
leak, but not necessarily exploitable (as the access checks on the Location
object should prevent changes).

> 
> > but I'm going to
> > revert to having per-world instances to avoid having this leak cross-world
as
> > well.

[haraken, 2017-01-24 21:57:25]

On 2017/01/24 21:52:58, dcheng wrote:
> On 2017/01/24 21:32:52, haraken wrote:
> > On 2017/01/24 19:15:40, dcheng wrote:
> > > On 2017/01/24 10:53:00, Yuki wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/core/frame/Location.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/core/frame/Location.cpp:127:
> > ->domTemplate(isolate,
> > > > DOMWrapperWorld::world(
> > > > On 2017/01/24 10:41:59, dcheng wrote:
> > > > > On 2017/01/24 10:31:04, Yuki wrote:
> > > > > > On 2017/01/24 09:40:47, dcheng wrote:
> > > > > > > However, this makes me nervous. Is there a way to get a dom
template
> > > > that's
> > > > > > not
> > > > > > > associated with a world?
> > > > > > 
> > > > > > No, because we have main-world-specific optimization in
> > > v8::FunctionTempalte
> > > > > and
> > > > > > its prototype template.  And this seems incorrect that we're going
to
> > use
> > > > the
> > > > > > same v8::Object between the main world and isolated worlds.  We're
> going
> > > to
> > > > > > assume "it's in the main world" even if it's in an isolated world.
> > > > > > 
> > > > > > Do we really need to re-use the existing Location::wrapperTypeInfo()
> > here?
> > > 
> > > > Is
> > > > > > it okay to define a new WrapperTypeInfo dedicated for remote frames?
> > > > > 
> > > > > Can you point me at the main world optimizations? I'm curious to see
if
> it
> > > > will
> > > > > affect the correctness of doing this. I saw several in things like
> > > > DOMDataStore,
> > > > > but I want to make sure I'm looking at the right ones.
> > > > 
> > > > I meant [PerWorldBindings] extended attribute, but Location.idl seems
not
> > > having
> > > > it.  So probably it actually works.  But it's unfortunate to ignore
> worlds.
> > > > 
> > > > If you support things in handlers, I guess you wouldn't need much
contents
> > in
> > > > v8::FunctionTemplate?
> > > > 
> > > > If this approach works fine, I'm not insisting to
> > > > v8::FunctionTemplate::GetFunction.
> > > 
> > > OK, jochen points out something interesting: sharing the v8 Object between
> > > worlds / contexts could lead to some info leaks by setting the location
> object
> > > as a key of a WeakMap.
> > 
> > Correct, any v8 object must not leak between worlds.
> 
> I think it's actually safe for NewRemoteInstance(), but it's not much harder
to
> write a per-world version. I'm uploading a CL for that shortly.
> 
> > 
> > > Thinking about this more, I think it's actually already an issue (multiple
> > > contexts will reference the same wrapper Location object),
> > 
> > So if this is happening already, it should be a serious security issue. It
> means
> > that, for example, there is a way to exploit a window object of the main
world
> > from a Chrome extension.
> 
> Let me clarify what I mean. Consider Location. Any given Location object can
> have only one main world wrapper associated it. However, Location is
accessible
> cross-origin, so origin B and origin C can get a reference to
originA.location.

Before landing this CL, a C++ Location object has a different wrapper per world,
doesn't it?

> 
> Let's see origin B and origin C use originA.location as a key in a WeakMap.
Then
> we navigate originA somewhere, so the original inner window / location are no
> longer active. Once the hidden value is overwritten with the new wrapper for
> Location, originA.location is eligible for garbage collection.
> 
> At this point, origin B and origin C can observe if the key in the WeakMap is
> collected or not (and if it isn't, they know someone else must be holding a
> reference to it and keeping it alive). Does that make sense? This is in an
info
> leak, but not necessarily exploitable (as the access checks on the Location
> object should prevent changes).

Do the access checks work for location.expando as well as Location's DOM
attributes?

> 
> > 
> > > but I'm going to
> > > revert to having per-world instances to avoid having this leak cross-world
> as
> > > well.

[dcheng, 2017-01-24 22:04:54]

On 2017/01/24 21:57:25, haraken wrote:
> On 2017/01/24 21:52:58, dcheng wrote:
> > On 2017/01/24 21:32:52, haraken wrote:
> > > On 2017/01/24 19:15:40, dcheng wrote:
> > > > On 2017/01/24 10:53:00, Yuki wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
> > > > > File third_party/WebKit/Source/core/frame/Location.cpp (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2640123006/diff/60001/third_party/WebKit/Sour...
> > > > > third_party/WebKit/Source/core/frame/Location.cpp:127:
> > > ->domTemplate(isolate,
> > > > > DOMWrapperWorld::world(
> > > > > On 2017/01/24 10:41:59, dcheng wrote:
> > > > > > On 2017/01/24 10:31:04, Yuki wrote:
> > > > > > > On 2017/01/24 09:40:47, dcheng wrote:
> > > > > > > > However, this makes me nervous. Is there a way to get a dom
> template
> > > > > that's
> > > > > > > not
> > > > > > > > associated with a world?
> > > > > > > 
> > > > > > > No, because we have main-world-specific optimization in
> > > > v8::FunctionTempalte
> > > > > > and
> > > > > > > its prototype template.  And this seems incorrect that we're going
> to
> > > use
> > > > > the
> > > > > > > same v8::Object between the main world and isolated worlds.  We're
> > going
> > > > to
> > > > > > > assume "it's in the main world" even if it's in an isolated world.
> > > > > > > 
> > > > > > > Do we really need to re-use the existing
Location::wrapperTypeInfo()
> > > here?
> > > > 
> > > > > Is
> > > > > > > it okay to define a new WrapperTypeInfo dedicated for remote
frames?
> > > > > > 
> > > > > > Can you point me at the main world optimizations? I'm curious to see
> if
> > it
> > > > > will
> > > > > > affect the correctness of doing this. I saw several in things like
> > > > > DOMDataStore,
> > > > > > but I want to make sure I'm looking at the right ones.
> > > > > 
> > > > > I meant [PerWorldBindings] extended attribute, but Location.idl seems
> not
> > > > having
> > > > > it.  So probably it actually works.  But it's unfortunate to ignore
> > worlds.
> > > > > 
> > > > > If you support things in handlers, I guess you wouldn't need much
> contents
> > > in
> > > > > v8::FunctionTemplate?
> > > > > 
> > > > > If this approach works fine, I'm not insisting to
> > > > > v8::FunctionTemplate::GetFunction.
> > > > 
> > > > OK, jochen points out something interesting: sharing the v8 Object
between
> > > > worlds / contexts could lead to some info leaks by setting the location
> > object
> > > > as a key of a WeakMap.
> > > 
> > > Correct, any v8 object must not leak between worlds.
> > 
> > I think it's actually safe for NewRemoteInstance(), but it's not much harder
> to
> > write a per-world version. I'm uploading a CL for that shortly.
> > 
> > > 
> > > > Thinking about this more, I think it's actually already an issue
(multiple
> > > > contexts will reference the same wrapper Location object),
> > > 
> > > So if this is happening already, it should be a serious security issue. It
> > means
> > > that, for example, there is a way to exploit a window object of the main
> world
> > > from a Chrome extension.
> > 
> > Let me clarify what I mean. Consider Location. Any given Location object can
> > have only one main world wrapper associated it. However, Location is
> accessible
> > cross-origin, so origin B and origin C can get a reference to
> originA.location.
> 
> Before landing this CL, a C++ Location object has a different wrapper per
world,
> doesn't it?

Correct, but I'm testing a CL that reverts this to per-world bindings.

> 
> > 
> > Let's see origin B and origin C use originA.location as a key in a WeakMap.
> Then
> > we navigate originA somewhere, so the original inner window / location are
no
> > longer active. Once the hidden value is overwritten with the new wrapper for
> > Location, originA.location is eligible for garbage collection.
> > 
> > At this point, origin B and origin C can observe if the key in the WeakMap
is
> > collected or not (and if it isn't, they know someone else must be holding a
> > reference to it and keeping it alive). Does that make sense? This is in an
> info
> > leak, but not necessarily exploitable (as the access checks on the Location
> > object should prevent changes).
> 
> Do the access checks work for location.expando as well as Location's DOM
> attributes?

The access checks on Location only allow read access for two properties (assign
and replace) and write-only access for href. So you can't set any additional
properties on Location.

I don't quite recall how the access checks on href work, but it doesn't appear
to be possible to set properties on window.location.href from a cross-origin
context.
For assign/replace, it is possible, but we use origin safe method getters for
CrossOrigin functions, so each context has its own copy of the function (and
can't affect other contexts).

> 
> > 
> > > 
> > > > but I'm going to
> > > > revert to having per-world instances to avoid having this leak
cross-world
> > as
> > > > well.

[dcheng, 2017-01-24 23:12:11]

New CL with per-world bindings.

https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
(right):

https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:79: if
(DOMDataStore::setReturnValue(info.GetReturnValue(), location))
Now that a Location object only exposes a consistent set of wrappers (either
NewRemoteInstance() wrappers, for a Location for a RemoteDOMWindow) or a
standard DOM wrapper, we can cache the wrappers using DOMDataStore.

https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:103: //
object is alive in order to save creation time of the wrapper object.
I'm not 100% sure about this, but we could try caching this for all windows. Any
opinions here?

[dcheng, 2017-01-24 23:12:14]

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

[dcheng, 2017-01-24 23:12:32]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2017-01-24 23:12:50]

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

[commit-bot: I haz the power, 2017-01-24 23:15:08]

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

[commit-bot: I haz the power, 2017-01-25 03:22:24]

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

[commit-bot: I haz the power, 2017-01-25 03:22:25]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-01-25 03:24:12]

https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
(right):

https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:79: if
(DOMDataStore::setReturnValue(info.GetReturnValue(), location))
On 2017/01/24 23:12:11, dcheng wrote:
> Now that a Location object only exposes a consistent set of wrappers (either
> NewRemoteInstance() wrappers, for a Location for a RemoteDOMWindow) or a
> standard DOM wrapper, we can cache the wrappers using DOMDataStore.

Can we add some dcheck to check that we're returning a wrapper of an intended
window?

https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:103: //
object is alive in order to save creation time of the wrapper object.
On 2017/01/24 23:12:11, dcheng wrote:
> I'm not 100% sure about this, but we could try caching this for all windows.
Any
> opinions here?

Actually this caching is already wrong in many ways:

- The caching is not a performance optimization to save the creation time. The
caching is needed for correctness to keep window.location alive while the window
object is alive.

- So we have to generate the caching code for all DOM attributes. However, we
generate it only for read-only attributes. That's a bug.

- So you should enable the caching for all windows.

A middle-term fix is to replace the caching with the wrapper tracing.

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

https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:297: // may
involve an object that does not have a creation context.

Can we add a dcheck to assert that this happens only on a Location object?

[dcheng, 2017-01-26 02:12:03]

https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
(right):

https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:79: if
(DOMDataStore::setReturnValue(info.GetReturnValue(), location))
On 2017/01/25 03:24:12, haraken wrote:
> On 2017/01/24 23:12:11, dcheng wrote:
> > Now that a Location object only exposes a consistent set of wrappers (either
> > NewRemoteInstance() wrappers, for a Location for a RemoteDOMWindow) or a
> > standard DOM wrapper, we can cache the wrappers using DOMDataStore.
> 
> Can we add some dcheck to check that we're returning a wrapper of an intended
> window?

Can you elaborate what you mean here? Do you mean I should DCHECK that the
location pointer stored in the internal field matches the location object here?

https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:103: //
object is alive in order to save creation time of the wrapper object.
On 2017/01/25 03:24:12, haraken wrote:
> On 2017/01/24 23:12:11, dcheng wrote:
> > I'm not 100% sure about this, but we could try caching this for all windows.
> Any
> > opinions here?
> 
> Actually this caching is already wrong in many ways:
> 
> - The caching is not a performance optimization to save the creation time. The
> caching is needed for correctness to keep window.location alive while the
window
> object is alive.

I'm trying to understand why this is important. If there is an object keeping
the Location wrapper live, then we will use the already-created wrapper. If
there is no reference keeping the Location wrapper live, the fact that the
wrapper changes is unobservable. Am I missing something?

> 
> - So we have to generate the caching code for all DOM attributes. However, we
> generate it only for read-only attributes. That's a bug.
> 
> - So you should enable the caching for all windows.
> 

This isn't specific to window.location then, right? Perhaps a separate CL would
be more appropriate? I'm especially nervous here, because setting the hidden
value depends on the current context, which means we're setting hidden values on
a window object from a cross-origin context.

> A middle-term fix is to replace the caching with the wrapper tracing.

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

https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:297: // may
involve an object that does not have a creation context.
On 2017/01/25 03:24:12, haraken wrote:
> 
> Can we add a dcheck to assert that this happens only on a Location object?

This is also used for Window though. Rather than add it here, I've added it to
the securityCheck() template -- cross-origin objects need a security check, and
we can static_assert if we have a new cross-origin object.

Note that this results in a static_assert showing up in the golden files for the
binding test results... alternatively we could explicitly hardcode the test
cross-origin interface name here as well.

[haraken, 2017-01-26 05:59:57]

https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
(right):

https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:79: if
(DOMDataStore::setReturnValue(info.GetReturnValue(), location))
On 2017/01/26 02:12:03, dcheng wrote:
> On 2017/01/25 03:24:12, haraken wrote:
> > On 2017/01/24 23:12:11, dcheng wrote:
> > > Now that a Location object only exposes a consistent set of wrappers
(either
> > > NewRemoteInstance() wrappers, for a Location for a RemoteDOMWindow) or a
> > > standard DOM wrapper, we can cache the wrappers using DOMDataStore.
> > 
> > Can we add some dcheck to check that we're returning a wrapper of an
intended
> > window?
> 
> Can you elaborate what you mean here? Do you mean I should DCHECK that the
> location pointer stored in the internal field matches the location object
here?

Ah, sorry. I realized my comment doesn't make sense -- please ignore :)

https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:103: //
object is alive in order to save creation time of the wrapper object.
On 2017/01/26 02:12:03, dcheng wrote:
> On 2017/01/25 03:24:12, haraken wrote:
> > On 2017/01/24 23:12:11, dcheng wrote:
> > > I'm not 100% sure about this, but we could try caching this for all
windows.
> > Any
> > > opinions here?
> > 
> > Actually this caching is already wrong in many ways:
> > 
> > - The caching is not a performance optimization to save the creation time.
The
> > caching is needed for correctness to keep window.location alive while the
> window
> > object is alive.
> 
> I'm trying to understand why this is important. If there is an object keeping
> the Location wrapper live, then we will use the already-created wrapper. If
> there is no reference keeping the Location wrapper live, the fact that the
> wrapper changes is unobservable. Am I missing something?

Imagine that a user sets window.location.foo="aaa".

If we don't have a hidden reference from window to window.location, the wrapper
may be collected in the next GC. Then the wrapper will be recreated when
window.location is accessed next. Then window.location.foo is lost (returns
undefined).


> 
> > 
> > - So we have to generate the caching code for all DOM attributes. However,
we
> > generate it only for read-only attributes. That's a bug.
> > 
> > - So you should enable the caching for all windows.
> > 
> 
> This isn't specific to window.location then, right? Perhaps a separate CL
would
> be more appropriate? I'm especially nervous here, because setting the hidden
> value depends on the current context, which means we're setting hidden values
on
> a window object from a cross-origin context.

Yeah, the hidden reference is already broken in many ways, so I think it's fine
to address it in a follow-up.

> 
> > A middle-term fix is to replace the caching with the wrapper tracing.

[dcheng, 2017-01-26 06:30:19]

https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
(right):

https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:103: //
object is alive in order to save creation time of the wrapper object.
On 2017/01/26 05:59:57, haraken wrote:
> On 2017/01/26 02:12:03, dcheng wrote:
> > On 2017/01/25 03:24:12, haraken wrote:
> > > On 2017/01/24 23:12:11, dcheng wrote:
> > > > I'm not 100% sure about this, but we could try caching this for all
> windows.
> > > Any
> > > > opinions here?
> > > 
> > > Actually this caching is already wrong in many ways:
> > > 
> > > - The caching is not a performance optimization to save the creation time.
> The
> > > caching is needed for correctness to keep window.location alive while the
> > window
> > > object is alive.
> > 
> > I'm trying to understand why this is important. If there is an object
keeping
> > the Location wrapper live, then we will use the already-created wrapper. If
> > there is no reference keeping the Location wrapper live, the fact that the
> > wrapper changes is unobservable. Am I missing something?
> 
> Imagine that a user sets window.location.foo="aaa".
> 
> If we don't have a hidden reference from window to window.location, the
wrapper
> may be collected in the next GC. Then the wrapper will be recreated when
> window.location is accessed next. Then window.location.foo is lost (returns
> undefined).

Ah-ha, I understand now.
So the interesting part here is Location cannot be reused, so a Location is tied
to either a RemoteDOMWindow or a LocalDOMWindow.

However, if the location is for a RemoteDOMWindow, we never need to cache the
wrapper, because it is not possible to set any JS properties on it. So the
effects are never observable.

Given that, I've added a comment to explain why it's OK to skip this for a
Location object for a RemoteFrame, with a TODO to address this and the hidden
value work in a followup.

> 
> 
> > 
> > > 
> > > - So we have to generate the caching code for all DOM attributes. However,
> we
> > > generate it only for read-only attributes. That's a bug.
> > > 
> > > - So you should enable the caching for all windows.
> > > 
> > 
> > This isn't specific to window.location then, right? Perhaps a separate CL
> would
> > be more appropriate? I'm especially nervous here, because setting the hidden
> > value depends on the current context, which means we're setting hidden
values
> on
> > a window object from a cross-origin context.
> 
> Yeah, the hidden reference is already broken in many ways, so I think it's
fine
> to address it in a follow-up.
> 
> > 
> > > A middle-term fix is to replace the caching with the wrapper tracing.
>

[dcheng, 2017-01-26 06:34:30]

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

[commit-bot: I haz the power, 2017-01-26 06:34:49]

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

[commit-bot: I haz the power, 2017-01-26 08:09:02]

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

[commit-bot: I haz the power, 2017-01-26 08:09:03]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-01-26 09:27:09]

https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
(right):

https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:103: //
object is alive in order to save creation time of the wrapper object.
On 2017/01/26 06:30:19, dcheng wrote:
> On 2017/01/26 05:59:57, haraken wrote:
> > On 2017/01/26 02:12:03, dcheng wrote:
> > > On 2017/01/25 03:24:12, haraken wrote:
> > > > On 2017/01/24 23:12:11, dcheng wrote:
> > > > > I'm not 100% sure about this, but we could try caching this for all
> > windows.
> > > > Any
> > > > > opinions here?
> > > > 
> > > > Actually this caching is already wrong in many ways:
> > > > 
> > > > - The caching is not a performance optimization to save the creation
time.
> > The
> > > > caching is needed for correctness to keep window.location alive while
the
> > > window
> > > > object is alive.
> > > 
> > > I'm trying to understand why this is important. If there is an object
> keeping
> > > the Location wrapper live, then we will use the already-created wrapper.
If
> > > there is no reference keeping the Location wrapper live, the fact that the
> > > wrapper changes is unobservable. Am I missing something?
> > 
> > Imagine that a user sets window.location.foo="aaa".
> > 
> > If we don't have a hidden reference from window to window.location, the
> wrapper
> > may be collected in the next GC. Then the wrapper will be recreated when
> > window.location is accessed next. Then window.location.foo is lost (returns
> > undefined).
> 
> Ah-ha, I understand now.
> So the interesting part here is Location cannot be reused, so a Location is
tied
> to either a RemoteDOMWindow or a LocalDOMWindow.
> 
> However, if the location is for a RemoteDOMWindow, we never need to cache the
> wrapper, because it is not possible to set any JS properties on it. So the
> effects are never observable.

It might still be observable when the remote Location wrapper is stored in a
WeakMap. If the wrapper is lost, it will be removed from the WeakMap.

Either way, this is already widely broken in the code base, so you don't need to
worry about it at the moment.

> 
> Given that, I've added a comment to explain why it's OK to skip this for a
> Location object for a RemoteFrame, with a TODO to address this and the hidden
> value work in a followup.
> 
> > 
> > 
> > > 
> > > > 
> > > > - So we have to generate the caching code for all DOM attributes.
However,
> > we
> > > > generate it only for read-only attributes. That's a bug.
> > > > 
> > > > - So you should enable the caching for all windows.
> > > > 
> > > 
> > > This isn't specific to window.location then, right? Perhaps a separate CL
> > would
> > > be more appropriate? I'm especially nervous here, because setting the
hidden
> > > value depends on the current context, which means we're setting hidden
> values
> > on
> > > a window object from a cross-origin context.
> > 
> > Yeah, the hidden reference is already broken in many ways, so I think it's
> fine
> > to address it in a follow-up.
> > 
> > > 
> > > > A middle-term fix is to replace the caching with the wrapper tracing.
> > 
>

[Yuki, 2017-01-26 10:41:54]

Looks good to me.

https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
(right):

https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:103: //
object is alive in order to save creation time of the wrapper object.
I'd argue on this point.  IIUC, the spec doesn't require to keep
|window.location.expando|.  I agree that web developers would be surprised,
though.

Unless [SameObject] is specified on a DOM attribute, it's not guaranteed that
the DOM attribute returns the same object.  So it's totally okay from a PoV of
the spec that we create a new wrapper and return it.  It's okay to return a
different object every time.

My point is "it's a nice-to-have".  The spec says it's okay.  Less surprising,
it's better, though.

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

https://codereview.chromium.org/2640123006/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:205:
static_assert(false, "Unexpected security check for interface
{{interface_name}}");
nit: #error "message" does the trick and it's more light-weight.

[Yuki, 2017-01-26 10:43:06]

LGTM.

[haraken, 2017-01-26 10:55:02]

On 2017/01/26 10:41:54, Yuki wrote:
> Looks good to me.
> 
>
https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
> (right):
> 
>
https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:103: //
> object is alive in order to save creation time of the wrapper object.
> I'd argue on this point.  IIUC, the spec doesn't require to keep
> |window.location.expando|.  I agree that web developers would be surprised,
> though.
> 
> Unless [SameObject] is specified on a DOM attribute, it's not guaranteed that
> the DOM attribute returns the same object.  So it's totally okay from a PoV of
> the spec that we create a new wrapper and return it.  It's okay to return a
> different object every time.
> 
> My point is "it's a nice-to-have".  The spec says it's okay.  Less surprising,
> it's better, though.

(I think we discussed this somewhere before but) the spec requires that
reachable objects should be kept alive, right? I agree that the spec is entirely
clear about the reachability but I think that the fact that window has a
reference to a location object means that there should be a strong reference
from the window to the location.

Regarding [SameObject], domenic@ was saying that the spec is missing
[SameObject] in many parts of the spec -- it's an implicitly assumption at least
currently.

> 
>
https://codereview.chromium.org/2640123006/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
> (right):
> 
>
https://codereview.chromium.org/2640123006/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:205:
> static_assert(false, "Unexpected security check for interface
> {{interface_name}}");
> nit: #error "message" does the trick and it's more light-weight.

[blink-reviews, 2017-01-26 13:07:18]

On Thu, Jan 26, 2017 at 5:41 AM <yukishiino@chromium.org> wrote:

> Looks good to me.
>
>
>
>
>
>
https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
> File
> third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
> (right):
>
>
>
https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:103:
> // object is alive in order to save creation time of the wrapper object.
> I'd argue on this point. IIUC, the spec doesn't require to keep
> |window.location.expando|. I agree that web developers would be
> surprised, though.
>
> Unless [SameObject] is specified on a DOM attribute, it's not guaranteed
> that the DOM attribute returns the same object. So it's totally okay
> from a PoV of the spec that we create a new wrapper and return it. It's
> okay to return a different object every time.
>

That's not true. [SameObject] is documentation-only, and it's for a very
specific case: when the value returned *never* changes. If it changes once,
e.g. during page navigation or something, you can't use [SameObject]. But
the normative algorithms still apply as to whether the same object keeps
being returned.

In particular https://html.spec.whatwg.org/#dom-document-location says

> The Document <https://html.spec.whatwg.org/#document> object's location
attribute's
getter must return this Document <https://html.spec.whatwg.org/#document>
object's relevant global object
<https://html.spec.whatwg.org/#concept-relevant-global>'s Location
<https://html.spec.whatwg.org/#location> object, if this Document
<https://html.spec.whatwg.org/#document> object is fully active
<https://html.spec.whatwg.org/#fully-active>, and null otherwise.


So this can change if and only if the Document's relevant global object
changes (document.open()), or the document's relevant global object's
Location object changes (never happens), or the Document transitions to or
from being fully active.


>
> My point is "it's a nice-to-have". The spec says it's okay. Less
> surprising, it's better, though.
>
>
>
https://codereview.chromium.org/2640123006/diff/120001/third_party/WebKit/Sou...
> File
> third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
> (right):
>
>
>
https://codereview.chromium.org/2640123006/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:205:
> static_assert(false, "Unexpected security check for interface
> {{interface_name}}");
> nit: #error "message" does the trick and it's more light-weight.
>
> https://codereview.chromium.org/2640123006/
>

-- 
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, 2017-01-26 13:07:18]

On Thu, Jan 26, 2017 at 5:41 AM <yukishiino@chromium.org> wrote:

> Looks good to me.
>
>
>
>
>
>
https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
> File
> third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
> (right):
>
>
>
https://codereview.chromium.org/2640123006/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:103:
> // object is alive in order to save creation time of the wrapper object.
> I'd argue on this point. IIUC, the spec doesn't require to keep
> |window.location.expando|. I agree that web developers would be
> surprised, though.
>
> Unless [SameObject] is specified on a DOM attribute, it's not guaranteed
> that the DOM attribute returns the same object. So it's totally okay
> from a PoV of the spec that we create a new wrapper and return it. It's
> okay to return a different object every time.
>

That's not true. [SameObject] is documentation-only, and it's for a very
specific case: when the value returned *never* changes. If it changes once,
e.g. during page navigation or something, you can't use [SameObject]. But
the normative algorithms still apply as to whether the same object keeps
being returned.

In particular https://html.spec.whatwg.org/#dom-document-location says

> The Document <https://html.spec.whatwg.org/#document> object's location
attribute's
getter must return this Document <https://html.spec.whatwg.org/#document>
object's relevant global object
<https://html.spec.whatwg.org/#concept-relevant-global>'s Location
<https://html.spec.whatwg.org/#location> object, if this Document
<https://html.spec.whatwg.org/#document> object is fully active
<https://html.spec.whatwg.org/#fully-active>, and null otherwise.


So this can change if and only if the Document's relevant global object
changes (document.open()), or the document's relevant global object's
Location object changes (never happens), or the Document transitions to or
from being fully active.


>
> My point is "it's a nice-to-have". The spec says it's okay. Less
> surprising, it's better, though.
>
>
>
https://codereview.chromium.org/2640123006/diff/120001/third_party/WebKit/Sou...
> File
> third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
> (right):
>
>
>
https://codereview.chromium.org/2640123006/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:205:
> static_assert(false, "Unexpected security check for interface
> {{interface_name}}");
> nit: #error "message" does the trick and it's more light-weight.
>
> https://codereview.chromium.org/2640123006/
>

-- 
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, 2017-01-26 13:12:14]

domenic@ and haraken@ are right.

I checked the spec again, and found that the spec assumes that there is 1:1
mappings between IDL interface type values and ECMAScript values.

So, as long as the IDL value is the same, we must not change the corresponding
ECMAScript value.

[dcheng, 2017-01-27 01:30:50]

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

[dcheng, 2017-01-27 01:31:42]

PTAL. I investigated the hidden value code more, and I think it's safe to set
the hidden value for both paths. I've updated this so we should be spec
compliant for window.location again.

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

https://codereview.chromium.org/2640123006/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:205:
static_assert(false, "Unexpected security check for interface
{{interface_name}}");
On 2017/01/26 10:41:54, Yuki wrote:
> nit: #error "message" does the trick and it's more light-weight.

Done.

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

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

[commit-bot: I haz the power, 2017-01-27 01:37:12]

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

[commit-bot: I haz the power, 2017-01-27 01:37:13]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  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_...)

[haraken, 2017-01-27 01:53:27]

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

https://codereview.chromium.org/2640123006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:115:
wrapper);

You have multiple location wrappers per holder, right? If you set those wrpapers
with the same key "KeepAlive#Window#location", won't we end up with overwriting
other location wrappers?

[dcheng, 2017-01-27 07:04:40]

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

https://codereview.chromium.org/2640123006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:115:
wrapper);
On 2017/01/27 01:53:26, haraken wrote:
> 
> You have multiple location wrappers per holder, right? If you set those
wrpapers
> with the same key "KeepAlive#Window#location", won't we end up with
overwriting
> other location wrappers?
> 

It will, but only if the frame is navigated. This was also the case before this
change.

It's also important to note that the distinction here is between local and
remote Windows, not same origin vs cross origin. So accessing a local window's
location property will always use the full Object wrapper, even if cross origin.

[Yuki, 2017-01-27 07:20:00]

LGTM

[haraken, 2017-01-27 08:03:19]

On 2017/01/27 07:04:40, dcheng wrote:
>
https://codereview.chromium.org/2640123006/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
> (right):
> 
>
https://codereview.chromium.org/2640123006/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:115:
> wrapper);
> On 2017/01/27 01:53:26, haraken wrote:
> > 
> > You have multiple location wrappers per holder, right? If you set those
> wrpapers
> > with the same key "KeepAlive#Window#location", won't we end up with
> overwriting
> > other location wrappers?
> > 
> 
> It will, but only if the frame is navigated. This was also the case before
this
> change.

ah...

Maybe I prefer not caching remote location objects until we implement the
correct behavior with the wrapper tracing. It would be confusing if
window.location starts returning a different object when the frame is
navigated...

> It's also important to note that the distinction here is between local and
> remote Windows, not same origin vs cross origin. So accessing a local window's
> location property will always use the full Object wrapper, even if cross
origin.

[Yuki, 2017-01-27 08:14:09]

On 2017/01/27 08:03:19, haraken wrote:
> Maybe I prefer not caching remote location objects until we implement the
> correct behavior with the wrapper tracing. It would be confusing if
> window.location starts returning a different object when the frame is
> navigated...

"returning a different object when the frame is navigated" happens regardless of
the wrapper caching, right?

If domWindow->location() changed, then the wrapper object changes accordingly,
and we return the new wrapper object associated with the new
domWindow->location().

[dcheng, 2017-01-27 08:34:24]

On 2017/01/27 08:14:09, Yuki wrote:
> On 2017/01/27 08:03:19, haraken wrote:
> > Maybe I prefer not caching remote location objects until we implement the
> > correct behavior with the wrapper tracing. It would be confusing if
> > window.location starts returning a different object when the frame is
> > navigated...
> 
> "returning a different object when the frame is navigated" happens regardless
of
> the wrapper caching, right?
> 
> If domWindow->location() changed, then the wrapper object changes accordingly,
> and we return the new wrapper object associated with the new
> domWindow->location().

Right, this already happens with or without wrapper tracing. The net effect of
the change is to keep all of our current behavior, so there won't be observable
differences from it, even with the WeakMap case.

[haraken, 2017-01-27 08:42:59]

On 2017/01/27 08:34:24, dcheng wrote:
> On 2017/01/27 08:14:09, Yuki wrote:
> > On 2017/01/27 08:03:19, haraken wrote:
> > > Maybe I prefer not caching remote location objects until we implement the
> > > correct behavior with the wrapper tracing. It would be confusing if
> > > window.location starts returning a different object when the frame is
> > > navigated...
> > 
> > "returning a different object when the frame is navigated" happens
regardless
> of
> > the wrapper caching, right?
> > 
> > If domWindow->location() changed, then the wrapper object changes
accordingly,
> > and we return the new wrapper object associated with the new
> > domWindow->location().
> 
> Right, this already happens with or without wrapper tracing. The net effect of
> the change is to keep all of our current behavior, so there won't be
observable
> differences from it, even with the WeakMap case.

Sorry, my comment was not making sense. LGTM!

I was thinking that we have a performance optimization to get window.location
using the private property (we have this optimization on readonly DOM
attributes) so I was worried about the following scenario:

1) window.location is accessed. window[KeepAlive#Window#location] is set to the
location object.
2) crossOriginWindow.location is accessed. window[KeepAlive#Window#location] is
overwritten to the cross-origin location object. window.location starts
returning a different location object.

[dcheng, 2017-01-27 08:56:08]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2017-01-27 08:56:08]

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

[commit-bot: I haz the power, 2017-01-27 08:56:18]

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

[commit-bot: I haz the power, 2017-01-27 10:31:11]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1485507368123440,
"parent_rev": "bb90def05df40c3aebf65c5a65c6376baf779f61", "commit_rev":
"9ead0def6f12d7152260f2eb21e78c7e09cd47ee"}

[commit-bot: I haz the power, 2017-01-27 10:32:04]

Description was changed from

==========
Use NewRemoteInstance() to create cross-origin objects.

Also use the current context as the creation context when
returning the cross-origin named enumerator's results.

BUG=682822
==========

to

==========
Use NewRemoteInstance() to create cross-origin objects.

Also use the current context as the creation context when
returning the cross-origin named enumerator's results.

BUG=682822

Review-Url: https://codereview.chromium.org/2640123006
Cr-Commit-Position: refs/heads/master@{#446635}
Committed:
https://chromium.googlesource.com/chromium/src/+/9ead0def6f12d7152260f2eb21e7...
==========

[commit-bot: I haz the power, 2017-01-27 10:32:05]

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