Use v8::Context::NewRemoteContext in RemoteWindowProxy.

Use v8::Context::NewRemoteContext in RemoteWindowProxy.

A RemoteWindowProxy manages the state for the WindowProxy of a
RemoteFrame. By definition, a RemoteFrame is always cross-origin.
Today, RemoteWindowProxy uses v8::Context; however, a full v8 Context
is fairly heavyweight and includes prototypes for many objects that
will never be used (since the context is always considered cross
origin).

v8::Context::NewRemoteContext is intended to help reduce the memory
overhead of remote frames; it simply instantiates a JSGlobalProxy and
a shim global object that are very lightweight, since it skips all the
work to set up prototypes for String, Number, Boolean, etc that will
never be used.

BUG=527190
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2760793002
Cr-Commit-Position: refs/heads/master@{#459151}
Committed: https://chromium.googlesource.com/chromium/src/+/a94ee9eb38e2003712667dfe217a5cb1a1763dd0

[dcheng, 2017-03-20 04:28:47]

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

[commit-bot: I haz the power, 2017-03-20 04:28:59]

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

[dcheng, 2017-03-20 04:43:13]

Description was changed from

==========
Use v8::Context::NewRemoteContext in RemoteWindowProxy.

BUG=527190
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Use v8::Context::NewRemoteContext in RemoteWindowProxy.

A RemoteWindowProxy manages the state for the WindowProxy of a
RemoteFrame. By definition, a RemoteFrame is always cross-origin.
Today, RemoteWindowProxy uses v8::Context; however, a full v8 Context
is fairly heavyweight and includes prototypes for many objects that
will never be used (since the context is always considered cross
origin).

v8::Context::NewRemoteContext is intended to help reduce the memory
overhead of remote frames; it simply instantiates a JSGlobalProxy and
a shim global object that are very lightweight, since it skips all the
work to set up prototypes for String, Number, Boolean, etc that will
never be used.

BUG=527190
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dcheng, 2017-03-20 04:43:13]

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

[dcheng, 2017-03-20 04:43:37]

Hopefully all the bugs are fixed...

[haraken, 2017-03-20 04:47:04]

LGTM to try :)

[commit-bot: I haz the power, 2017-03-20 05:18:12]

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

[commit-bot: I haz the power, 2017-03-20 05:18:13]

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

[dcheng, 2017-03-20 06:03:09]

Hmm... looks like ScriptState::from is getting called from
associateObjectWithWrapper now? Investigating...

[dcheng, 2017-03-20 06:50:13]

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

[commit-bot: I haz the power, 2017-03-20 06:50:27]

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

[dcheng, 2017-03-20 06:51:30]

https://codereview.chromium.org/2760793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right):

https://codereview.chromium.org/2760793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:126:
V8DOMWrapper::setNativeInfo(isolate(), windowWrapper, wrapperTypeInfo,
PTAL... I had to change this to setNativeInf instead of
associateObjectWithWrapper, since associateObjectWithWrapper caches the wrapper
in the DOMDataStore.

One question I had though... why should we be using associateObjectWithWrapper?
The global object is different for different Documents, so it seems like the
internal cached wrapper will be the wrong global object after a navigation?

[haraken, 2017-03-20 07:36:49]

https://codereview.chromium.org/2760793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right):

https://codereview.chromium.org/2760793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:126:
V8DOMWrapper::setNativeInfo(isolate(), windowWrapper, wrapperTypeInfo,
On 2017/03/20 06:51:30, dcheng wrote:
> PTAL... I had to change this to setNativeInf instead of
> associateObjectWithWrapper, since associateObjectWithWrapper caches the
wrapper
> in the DOMDataStore.
> 
> One question I had though... why should we be using
associateObjectWithWrapper?
> The global object is different for different Documents, so it seems like the
> internal cached wrapper will be the wrong global object after a navigation?

After the navigation, the C++ window object will change as well, so it's just
fine, right?

We shouldn't store the global proxy on the DOMDataStore because the
corresponding C++ window object may change over time.

[dcheng, 2017-03-20 08:06:53]

https://codereview.chromium.org/2760793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right):

https://codereview.chromium.org/2760793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:126:
V8DOMWrapper::setNativeInfo(isolate(), windowWrapper, wrapperTypeInfo,
On 2017/03/20 07:36:49, haraken wrote:
> On 2017/03/20 06:51:30, dcheng wrote:
> > PTAL... I had to change this to setNativeInf instead of
> > associateObjectWithWrapper, since associateObjectWithWrapper caches the
> wrapper
> > in the DOMDataStore.
> > 
> > One question I had though... why should we be using
> associateObjectWithWrapper?
> > The global object is different for different Documents, so it seems like the
> > internal cached wrapper will be the wrong global object after a navigation?
> 
> After the navigation, the C++ window object will change as well, so it's just
> fine, right?
> 
> We shouldn't store the global proxy on the DOMDataStore because the
> corresponding C++ window object may change over time.

Ah... I see. Why is it important to store the global object on the room data
store though?

I see two alternatives:
1. Use setNativeInfo (this patch)
2. Move DOMDataStore to WindowProxy. I'm not sure how complicated this will be,
and it seems like a lot of work just to hold one wrapper per world for remote
frames.

[jochen (gone - plz use gerrit), 2017-03-20 08:08:22]

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

[jochen (gone - plz use gerrit), 2017-03-20 08:08:23]

should we have a layout test that tests remoteWindow.toString?

[commit-bot: I haz the power, 2017-03-20 09:40:48]

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

[commit-bot: I haz the power, 2017-03-20 09:40:49]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-03-20 11:42:36]

On 2017/03/20 08:06:53, dcheng wrote:
>
https://codereview.chromium.org/2760793002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right):
> 
>
https://codereview.chromium.org/2760793002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:126:
> V8DOMWrapper::setNativeInfo(isolate(), windowWrapper, wrapperTypeInfo,
> On 2017/03/20 07:36:49, haraken wrote:
> > On 2017/03/20 06:51:30, dcheng wrote:
> > > PTAL... I had to change this to setNativeInf instead of
> > > associateObjectWithWrapper, since associateObjectWithWrapper caches the
> > wrapper
> > > in the DOMDataStore.
> > > 
> > > One question I had though... why should we be using
> > associateObjectWithWrapper?
> > > The global object is different for different Documents, so it seems like
the
> > > internal cached wrapper will be the wrong global object after a
navigation?
> > 
> > After the navigation, the C++ window object will change as well, so it's
just
> > fine, right?
> > 
> > We shouldn't store the global proxy on the DOMDataStore because the
> > corresponding C++ window object may change over time.
> 
> Ah... I see. Why is it important to store the global object on the room data
> store though?
> 
> I see two alternatives:
> 1. Use setNativeInfo (this patch)
> 2. Move DOMDataStore to WindowProxy. I'm not sure how complicated this will
be,
> and it seems like a lot of work just to hold one wrapper per world for remote
> frames.

Once yukishiino@ lands https://codereview.chromium.org/2617733004/, I think it
becomes important that a global object is stored in the DOMDataStore. After the
CL is landed, ToV8(DOMWindow*) starts looking up the DOMDataStore to find a
global object rather than using windowProxy()->globalObject().

[dcheng, 2017-03-21 02:47:06]

On 2017/03/20 11:42:36, haraken wrote:
> On 2017/03/20 08:06:53, dcheng wrote:
> >
>
https://codereview.chromium.org/2760793002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/2760793002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:126:
> > V8DOMWrapper::setNativeInfo(isolate(), windowWrapper, wrapperTypeInfo,
> > On 2017/03/20 07:36:49, haraken wrote:
> > > On 2017/03/20 06:51:30, dcheng wrote:
> > > > PTAL... I had to change this to setNativeInf instead of
> > > > associateObjectWithWrapper, since associateObjectWithWrapper caches the
> > > wrapper
> > > > in the DOMDataStore.
> > > > 
> > > > One question I had though... why should we be using
> > > associateObjectWithWrapper?
> > > > The global object is different for different Documents, so it seems like
> the
> > > > internal cached wrapper will be the wrong global object after a
> navigation?
> > > 
> > > After the navigation, the C++ window object will change as well, so it's
> just
> > > fine, right?
> > > 
> > > We shouldn't store the global proxy on the DOMDataStore because the
> > > corresponding C++ window object may change over time.
> > 
> > Ah... I see. Why is it important to store the global object on the room data
> > store though?
> > 
> > I see two alternatives:
> > 1. Use setNativeInfo (this patch)
> > 2. Move DOMDataStore to WindowProxy. I'm not sure how complicated this will
> be,
> > and it seems like a lot of work just to hold one wrapper per world for
remote
> > frames.
> 
> Once yukishiino@ lands https://codereview.chromium.org/2617733004/, I think it
> becomes important that a global object is stored in the DOMDataStore. After
the
> CL is landed, ToV8(DOMWindow*) starts looking up the DOMDataStore to find a
> global object rather than using windowProxy()->globalObject().

Hmm... I looked into moving DOMDataStore, and I want to understand more before
figuring out how to proceed:

associateObjectWithWrapper() stores the wrapper on the DOMDataStore. It uses the
DOMDataStore of the current context:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

However, a remote context can never be the current context (since it cannot be
entered). One way to address this is to say that it's OK to use the current
context still; however, this means we have a tricky mapping of wrappers to C++
object:
- There will only be one main world wrapper, since this is stored in the
ScriptWrappable itself.
- However, any isolated world wrappers will be stored in the DOMDataStore of the
*accessing* context.
- Since multiple contexts can access a RemoteWindowProxy, this means there can
be multiple wrappers per isolated world pointing to the same C++ object.
- In addition, when swapping frames, we have no current context at all. To make
sure we updated all the wrappers correctly, we'd have to iterate through all
DOMDataStores in all contexts and make sure that any wrappers to WindowProxy are
updated with the new internal fields.

In particular, the last part sounds problematic. So my question is: what is the
benefit to using WindowProxy for DOMDataStore?

[haraken, 2017-03-21 02:55:20]

On 2017/03/21 02:47:06, dcheng wrote:
> On 2017/03/20 11:42:36, haraken wrote:
> > On 2017/03/20 08:06:53, dcheng wrote:
> > >
> >
>
https://codereview.chromium.org/2760793002/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2760793002/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:126:
> > > V8DOMWrapper::setNativeInfo(isolate(), windowWrapper, wrapperTypeInfo,
> > > On 2017/03/20 07:36:49, haraken wrote:
> > > > On 2017/03/20 06:51:30, dcheng wrote:
> > > > > PTAL... I had to change this to setNativeInf instead of
> > > > > associateObjectWithWrapper, since associateObjectWithWrapper caches
the
> > > > wrapper
> > > > > in the DOMDataStore.
> > > > > 
> > > > > One question I had though... why should we be using
> > > > associateObjectWithWrapper?
> > > > > The global object is different for different Documents, so it seems
like
> > the
> > > > > internal cached wrapper will be the wrong global object after a
> > navigation?
> > > > 
> > > > After the navigation, the C++ window object will change as well, so it's
> > just
> > > > fine, right?
> > > > 
> > > > We shouldn't store the global proxy on the DOMDataStore because the
> > > > corresponding C++ window object may change over time.
> > > 
> > > Ah... I see. Why is it important to store the global object on the room
data
> > > store though?
> > > 
> > > I see two alternatives:
> > > 1. Use setNativeInfo (this patch)
> > > 2. Move DOMDataStore to WindowProxy. I'm not sure how complicated this
will
> > be,
> > > and it seems like a lot of work just to hold one wrapper per world for
> remote
> > > frames.
> > 
> > Once yukishiino@ lands https://codereview.chromium.org/2617733004/, I think
it
> > becomes important that a global object is stored in the DOMDataStore. After
> the
> > CL is landed, ToV8(DOMWindow*) starts looking up the DOMDataStore to find a
> > global object rather than using windowProxy()->globalObject().
> 
> Hmm... I looked into moving DOMDataStore, and I want to understand more before
> figuring out how to proceed:
> 
> associateObjectWithWrapper() stores the wrapper on the DOMDataStore. It uses
the
> DOMDataStore of the current context:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
> 
> However, a remote context can never be the current context (since it cannot be
> entered). One way to address this is to say that it's OK to use the current
> context still; however, this means we have a tricky mapping of wrappers to C++
> object:
> - There will only be one main world wrapper, since this is stored in the
> ScriptWrappable itself.
> - However, any isolated world wrappers will be stored in the DOMDataStore of
the
> *accessing* context.
> - Since multiple contexts can access a RemoteWindowProxy, this means there can
> be multiple wrappers per isolated world pointing to the same C++ object.
> - In addition, when swapping frames, we have no current context at all. To
make
> sure we updated all the wrappers correctly, we'd have to iterate through all
> DOMDataStores in all contexts and make sure that any wrappers to WindowProxy
are
> updated with the new internal fields.
> 
> In particular, the last part sounds problematic. So my question is: what is
the
> benefit to using WindowProxy for DOMDataStore?

How does ToV8(RemoteDOMWindow*) work? Are you planning to override it and make
it return remoteWindowProxy()->context()? (In that case, I think it's fine.)

Regarding LocalWindowProxy, I'd prefer storing it in DOMDataStore so that we
don't need to override ToV8(LocalDOMWindow*) though.

[dcheng, 2017-03-21 04:09:02]

On 2017/03/21 02:55:20, haraken wrote:
> On 2017/03/21 02:47:06, dcheng wrote:
> > On 2017/03/20 11:42:36, haraken wrote:
> > > On 2017/03/20 08:06:53, dcheng wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2760793002/diff/40001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp
> > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2760793002/diff/40001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:126:
> > > > V8DOMWrapper::setNativeInfo(isolate(), windowWrapper, wrapperTypeInfo,
> > > > On 2017/03/20 07:36:49, haraken wrote:
> > > > > On 2017/03/20 06:51:30, dcheng wrote:
> > > > > > PTAL... I had to change this to setNativeInf instead of
> > > > > > associateObjectWithWrapper, since associateObjectWithWrapper caches
> the
> > > > > wrapper
> > > > > > in the DOMDataStore.
> > > > > > 
> > > > > > One question I had though... why should we be using
> > > > > associateObjectWithWrapper?
> > > > > > The global object is different for different Documents, so it seems
> like
> > > the
> > > > > > internal cached wrapper will be the wrong global object after a
> > > navigation?
> > > > > 
> > > > > After the navigation, the C++ window object will change as well, so
it's
> > > just
> > > > > fine, right?
> > > > > 
> > > > > We shouldn't store the global proxy on the DOMDataStore because the
> > > > > corresponding C++ window object may change over time.
> > > > 
> > > > Ah... I see. Why is it important to store the global object on the room
> data
> > > > store though?
> > > > 
> > > > I see two alternatives:
> > > > 1. Use setNativeInfo (this patch)
> > > > 2. Move DOMDataStore to WindowProxy. I'm not sure how complicated this
> will
> > > be,
> > > > and it seems like a lot of work just to hold one wrapper per world for
> > remote
> > > > frames.
> > > 
> > > Once yukishiino@ lands https://codereview.chromium.org/2617733004/, I
think
> it
> > > becomes important that a global object is stored in the DOMDataStore.
After
> > the
> > > CL is landed, ToV8(DOMWindow*) starts looking up the DOMDataStore to find
a
> > > global object rather than using windowProxy()->globalObject().
> > 
> > Hmm... I looked into moving DOMDataStore, and I want to understand more
before
> > figuring out how to proceed:
> > 
> > associateObjectWithWrapper() stores the wrapper on the DOMDataStore. It uses
> the
> > DOMDataStore of the current context:
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
> > 
> > However, a remote context can never be the current context (since it cannot
be
> > entered). One way to address this is to say that it's OK to use the current
> > context still; however, this means we have a tricky mapping of wrappers to
C++
> > object:
> > - There will only be one main world wrapper, since this is stored in the
> > ScriptWrappable itself.
> > - However, any isolated world wrappers will be stored in the DOMDataStore of
> the
> > *accessing* context.
> > - Since multiple contexts can access a RemoteWindowProxy, this means there
can
> > be multiple wrappers per isolated world pointing to the same C++ object.
> > - In addition, when swapping frames, we have no current context at all. To
> make
> > sure we updated all the wrappers correctly, we'd have to iterate through all
> > DOMDataStores in all contexts and make sure that any wrappers to WindowProxy
> are
> > updated with the new internal fields.
> > 
> > In particular, the last part sounds problematic. So my question is: what is
> the
> > benefit to using WindowProxy for DOMDataStore?
> 
> How does ToV8(RemoteDOMWindow*) work? Are you planning to override it and make
> it return remoteWindowProxy()->context()? (In that case, I think it's fine.)

Unfortunately, it's not easy to do that without complicating the bindings: we
return a generic DOMWindow pointer through things like DOMWindow::parent(), and
so we'd need to write custom code to downcast (with custom bindings or some
other DOMWindow-specific magic).

> 
> Regarding LocalWindowProxy, I'd prefer storing it in DOMDataStore so that we
> don't need to override ToV8(LocalDOMWindow*) though.

[haraken, 2017-03-21 05:55:01]

On 2017/03/21 04:09:02, dcheng wrote:
> On 2017/03/21 02:55:20, haraken wrote:
> > On 2017/03/21 02:47:06, dcheng wrote:
> > > On 2017/03/20 11:42:36, haraken wrote:
> > > > On 2017/03/20 08:06:53, dcheng wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2760793002/diff/40001/third_party/WebKit/Sour...
> > > > > File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp
> > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2760793002/diff/40001/third_party/WebKit/Sour...
> > > > > third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:126:
> > > > > V8DOMWrapper::setNativeInfo(isolate(), windowWrapper, wrapperTypeInfo,
> > > > > On 2017/03/20 07:36:49, haraken wrote:
> > > > > > On 2017/03/20 06:51:30, dcheng wrote:
> > > > > > > PTAL... I had to change this to setNativeInf instead of
> > > > > > > associateObjectWithWrapper, since associateObjectWithWrapper
caches
> > the
> > > > > > wrapper
> > > > > > > in the DOMDataStore.
> > > > > > > 
> > > > > > > One question I had though... why should we be using
> > > > > > associateObjectWithWrapper?
> > > > > > > The global object is different for different Documents, so it
seems
> > like
> > > > the
> > > > > > > internal cached wrapper will be the wrong global object after a
> > > > navigation?
> > > > > > 
> > > > > > After the navigation, the C++ window object will change as well, so
> it's
> > > > just
> > > > > > fine, right?
> > > > > > 
> > > > > > We shouldn't store the global proxy on the DOMDataStore because the
> > > > > > corresponding C++ window object may change over time.
> > > > > 
> > > > > Ah... I see. Why is it important to store the global object on the
room
> > data
> > > > > store though?
> > > > > 
> > > > > I see two alternatives:
> > > > > 1. Use setNativeInfo (this patch)
> > > > > 2. Move DOMDataStore to WindowProxy. I'm not sure how complicated this
> > will
> > > > be,
> > > > > and it seems like a lot of work just to hold one wrapper per world for
> > > remote
> > > > > frames.
> > > > 
> > > > Once yukishiino@ lands https://codereview.chromium.org/2617733004/, I
> think
> > it
> > > > becomes important that a global object is stored in the DOMDataStore.
> After
> > > the
> > > > CL is landed, ToV8(DOMWindow*) starts looking up the DOMDataStore to
find
> a
> > > > global object rather than using windowProxy()->globalObject().
> > > 
> > > Hmm... I looked into moving DOMDataStore, and I want to understand more
> before
> > > figuring out how to proceed:
> > > 
> > > associateObjectWithWrapper() stores the wrapper on the DOMDataStore. It
uses
> > the
> > > DOMDataStore of the current context:
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
> > > 
> > > However, a remote context can never be the current context (since it
cannot
> be
> > > entered). One way to address this is to say that it's OK to use the
current
> > > context still; however, this means we have a tricky mapping of wrappers to
> C++
> > > object:
> > > - There will only be one main world wrapper, since this is stored in the
> > > ScriptWrappable itself.
> > > - However, any isolated world wrappers will be stored in the DOMDataStore
of
> > the
> > > *accessing* context.
> > > - Since multiple contexts can access a RemoteWindowProxy, this means there
> can
> > > be multiple wrappers per isolated world pointing to the same C++ object.
> > > - In addition, when swapping frames, we have no current context at all. To
> > make
> > > sure we updated all the wrappers correctly, we'd have to iterate through
all
> > > DOMDataStores in all contexts and make sure that any wrappers to
WindowProxy
> > are
> > > updated with the new internal fields.
> > > 
> > > In particular, the last part sounds problematic. So my question is: what
is
> > the
> > > benefit to using WindowProxy for DOMDataStore?
> > 
> > How does ToV8(RemoteDOMWindow*) work? Are you planning to override it and
make
> > it return remoteWindowProxy()->context()? (In that case, I think it's fine.)
> 
> Unfortunately, it's not easy to do that without complicating the bindings: we
> return a generic DOMWindow pointer through things like DOMWindow::parent(),
and
> so we'd need to write custom code to downcast (with custom bindings or some
> other DOMWindow-specific magic).
> 
> > 
> > Regarding LocalWindowProxy, I'd prefer storing it in DOMDataStore so that we
> > don't need to override ToV8(LocalDOMWindow*) though.

So, your suggestion is to use setNativeInfo for both LocalDOMWindow* and
RemoteDOMWindow* and override ToV8(DOMWindow*) (i.e., close yukishiino's CL)?

[dcheng, 2017-03-21 06:38:13]

On 2017/03/21 05:55:01, haraken wrote:
> On 2017/03/21 04:09:02, dcheng wrote:
> > On 2017/03/21 02:55:20, haraken wrote:
> > > On 2017/03/21 02:47:06, dcheng wrote:
> > > > On 2017/03/20 11:42:36, haraken wrote:
> > > > > On 2017/03/20 08:06:53, dcheng wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2760793002/diff/40001/third_party/WebKit/Sour...
> > > > > > File
third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp
> > > > (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2760793002/diff/40001/third_party/WebKit/Sour...
> > > > > >
third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:126:
> > > > > > V8DOMWrapper::setNativeInfo(isolate(), windowWrapper,
wrapperTypeInfo,
> > > > > > On 2017/03/20 07:36:49, haraken wrote:
> > > > > > > On 2017/03/20 06:51:30, dcheng wrote:
> > > > > > > > PTAL... I had to change this to setNativeInf instead of
> > > > > > > > associateObjectWithWrapper, since associateObjectWithWrapper
> caches
> > > the
> > > > > > > wrapper
> > > > > > > > in the DOMDataStore.
> > > > > > > > 
> > > > > > > > One question I had though... why should we be using
> > > > > > > associateObjectWithWrapper?
> > > > > > > > The global object is different for different Documents, so it
> seems
> > > like
> > > > > the
> > > > > > > > internal cached wrapper will be the wrong global object after a
> > > > > navigation?
> > > > > > > 
> > > > > > > After the navigation, the C++ window object will change as well,
so
> > it's
> > > > > just
> > > > > > > fine, right?
> > > > > > > 
> > > > > > > We shouldn't store the global proxy on the DOMDataStore because
the
> > > > > > > corresponding C++ window object may change over time.
> > > > > > 
> > > > > > Ah... I see. Why is it important to store the global object on the
> room
> > > data
> > > > > > store though?
> > > > > > 
> > > > > > I see two alternatives:
> > > > > > 1. Use setNativeInfo (this patch)
> > > > > > 2. Move DOMDataStore to WindowProxy. I'm not sure how complicated
this
> > > will
> > > > > be,
> > > > > > and it seems like a lot of work just to hold one wrapper per world
for
> > > > remote
> > > > > > frames.
> > > > > 
> > > > > Once yukishiino@ lands https://codereview.chromium.org/2617733004/, I
> > think
> > > it
> > > > > becomes important that a global object is stored in the DOMDataStore.
> > After
> > > > the
> > > > > CL is landed, ToV8(DOMWindow*) starts looking up the DOMDataStore to
> find
> > a
> > > > > global object rather than using windowProxy()->globalObject().
> > > > 
> > > > Hmm... I looked into moving DOMDataStore, and I want to understand more
> > before
> > > > figuring out how to proceed:
> > > > 
> > > > associateObjectWithWrapper() stores the wrapper on the DOMDataStore. It
> uses
> > > the
> > > > DOMDataStore of the current context:
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
> > > > 
> > > > However, a remote context can never be the current context (since it
> cannot
> > be
> > > > entered). One way to address this is to say that it's OK to use the
> current
> > > > context still; however, this means we have a tricky mapping of wrappers
to
> > C++
> > > > object:
> > > > - There will only be one main world wrapper, since this is stored in the
> > > > ScriptWrappable itself.
> > > > - However, any isolated world wrappers will be stored in the
DOMDataStore
> of
> > > the
> > > > *accessing* context.
> > > > - Since multiple contexts can access a RemoteWindowProxy, this means
there
> > can
> > > > be multiple wrappers per isolated world pointing to the same C++ object.
> > > > - In addition, when swapping frames, we have no current context at all.
To
> > > make
> > > > sure we updated all the wrappers correctly, we'd have to iterate through
> all
> > > > DOMDataStores in all contexts and make sure that any wrappers to
> WindowProxy
> > > are
> > > > updated with the new internal fields.
> > > > 
> > > > In particular, the last part sounds problematic. So my question is: what
> is
> > > the
> > > > benefit to using WindowProxy for DOMDataStore?
> > > 
> > > How does ToV8(RemoteDOMWindow*) work? Are you planning to override it and
> make
> > > it return remoteWindowProxy()->context()? (In that case, I think it's
fine.)
> > 
> > Unfortunately, it's not easy to do that without complicating the bindings:
we
> > return a generic DOMWindow pointer through things like DOMWindow::parent(),
> and
> > so we'd need to write custom code to downcast (with custom bindings or some
> > other DOMWindow-specific magic).
> > 
> > > 
> > > Regarding LocalWindowProxy, I'd prefer storing it in DOMDataStore so that
we
> > > don't need to override ToV8(LocalDOMWindow*) though.
> 
> So, your suggestion is to use setNativeInfo for both LocalDOMWindow* and
> RemoteDOMWindow* and override ToV8(DOMWindow*) (i.e., close yukishiino's CL)?

That would be my recommendation, but I'm interested in if yukishiino has any
alternatives: it is unfortunate that we need to specialize ToV8 for DOMWindow,
but as it stands, I'm not sure I see a way around it.

[Yuki, 2017-03-21 09:24:26]

LGTM for this CL, and I support jochen's suggestion to test toString(), etc.
against RemoteDOMWindow.

About the wrapper association, I don't understand what problems you're seeing.

We have wrapper objects per-world, not per-accessing-context.  My plan is to
store the global proxy in DOMDataStore (or ScriptWrappable if it's main world),
and it should be okay because a remote window has its own global proxy object
although it doesn't have a global object.  We can save a global proxy for a
remote window in a DOMDataStore with mapping it to a RemoteDOMWindow.  When
navigating, we need to update the internal fields (we're already doing this).

It seems I'm missing some important points, but I don't understand you guys'
points.  Could you elaborate again what matters?

[dcheng, 2017-03-21 17:45:32]

On 2017/03/21 09:24:26, Yuki wrote:
> LGTM for this CL, and I support jochen's suggestion to test toString(), etc.
> against RemoteDOMWindow.

Yeah, I'll upload a CL for that later today (I've been trying to figure out the
DOMDataStore issues).

> 
> About the wrapper association, I don't understand what problems you're seeing.
> 
> We have wrapper objects per-world, not per-accessing-context. 

Right, but these wrapper objects are in a DOMDataStore. DOMDataStore is
associated with a ScriptState (which is per-WindowProxy).

> My plan is to
> store the global proxy in DOMDataStore (or ScriptWrappable if it's main
world),
> and it should be okay because a remote window has its own global proxy object
> although it doesn't have a global object.

Actually, a remote global proxy does have a stub global object =)
It originally didn't, but it caused lots of other issues: I had to add lots of
special-cases throughout v8 to account for this case. So instead of doing that,
now we just create a inner global object for remote proxies too.

> We can save a global proxy for a
> remote window in a DOMDataStore with mapping it to a RemoteDOMWindow.  When
> navigating, we need to update the internal fields (we're already doing this).
> 
> It seems I'm missing some important points, but I don't understand you guys'
> points.  Could you elaborate again what matters?

The problem comes down to this: what DOMDataStore is the right one to store a
remote global object association in? For consistency, we should be storing it on
the DOMDataStore associated with the RemoteWindowProxy. However, since a remote
context is never entered, DOMDataStore::current() will not be able to lookup the
right DOMDataStore--so what happens instead is the global object association
will be cached on the DOMDataStore of whatever the current context happened to
be at the time: this will typically be the accessing context. There are also
times when we won't even have a current context (such as during frame swap), and
so we need a workaround for that as well. For these reasons, it's non-trivial to
store this association in DOMDataStore at all.

[haraken, 2017-03-22 04:32:00]

On 2017/03/21 17:45:32, dcheng wrote:
> On 2017/03/21 09:24:26, Yuki wrote:
> > LGTM for this CL, and I support jochen's suggestion to test toString(), etc.
> > against RemoteDOMWindow.
> 
> Yeah, I'll upload a CL for that later today (I've been trying to figure out
the
> DOMDataStore issues).
> 
> > 
> > About the wrapper association, I don't understand what problems you're
seeing.
> > 
> > We have wrapper objects per-world, not per-accessing-context. 
> 
> Right, but these wrapper objects are in a DOMDataStore. DOMDataStore is
> associated with a ScriptState (which is per-WindowProxy).
> 
> > My plan is to
> > store the global proxy in DOMDataStore (or ScriptWrappable if it's main
> world),
> > and it should be okay because a remote window has its own global proxy
object
> > although it doesn't have a global object.
> 
> Actually, a remote global proxy does have a stub global object =)
> It originally didn't, but it caused lots of other issues: I had to add lots of
> special-cases throughout v8 to account for this case. So instead of doing
that,
> now we just create a inner global object for remote proxies too.
> 
> > We can save a global proxy for a
> > remote window in a DOMDataStore with mapping it to a RemoteDOMWindow.  When
> > navigating, we need to update the internal fields (we're already doing
this).
> > 
> > It seems I'm missing some important points, but I don't understand you guys'
> > points.  Could you elaborate again what matters?
> 
> The problem comes down to this: what DOMDataStore is the right one to store a
> remote global object association in? For consistency, we should be storing it
on
> the DOMDataStore associated with the RemoteWindowProxy. However, since a
remote
> context is never entered, DOMDataStore::current() will not be able to lookup
the
> right DOMDataStore--so what happens instead is the global object association
> will be cached on the DOMDataStore of whatever the current context happened to
> be at the time: this will typically be the accessing context. There are also
> times when we won't even have a current context (such as during frame swap),
and
> so we need a workaround for that as well. For these reasons, it's non-trivial
to
> store this association in DOMDataStore at all.

Not using DOMDataStore sounds good to me. Would you add a comment about it?

LGTM.

[Yuki, 2017-03-22 07:19:09]

On 2017/03/21 17:45:32, dcheng wrote:
> The problem comes down to this: what DOMDataStore is the right one to store a
> remote global object association in? For consistency, we should be storing it
on
> the DOMDataStore associated with the RemoteWindowProxy. However, since a
remote
> context is never entered, DOMDataStore::current() will not be able to lookup
the
> right DOMDataStore--so what happens instead is the global object association
> will be cached on the DOMDataStore of whatever the current context happened to
> be at the time: this will typically be the accessing context.

My understanding is that we're retrieving a DOMDataStore via v8::Context, via
ScriptState, but the DOMDataStore exists per world, not per context.  So, it
doesn't matter through which context you retrieve a DOMDataStore.  It matters in
which world, you retrieve a DOMDataStore.  Because the accessing context must be
in the same world, it's okay to retrieve a DOMDataStore through the accessing
context instead of the remote context.


> There are also
> times when we won't even have a current context (such as during frame swap),
and
> so we need a workaround for that as well. For these reasons, it's non-trivial
to
> store this association in DOMDataStore at all.

If you don't have a current context, it's strange to access to any v8::Object
(including DOM wrapper objects).  When ToV8() is called, there must exist the
current context.  V8 APIs may throw an exception and we need a v8::Context.  If
you don't have a current context, you shouldn't use V8 APIs nor access to
v8::Object in general (except for a few special APIs).

Plus, without remote frames, we're already swapping frames, right?  This problem
doesn't seem remote-frame-specific.


Probably, DOMWrapperWorld / DOMDataStore should be retrievable without
ScriptState / v8::Context.  But it doesn't mean that we cannot store wrapper
objects in DOMDataStore.

This is my understanding.  Please correct me if something is wrong.

[dcheng, 2017-03-22 08:10:28]

On 2017/03/22 07:19:09, Yuki wrote:
> On 2017/03/21 17:45:32, dcheng wrote:
> > The problem comes down to this: what DOMDataStore is the right one to store
a
> > remote global object association in? For consistency, we should be storing
it
> on
> > the DOMDataStore associated with the RemoteWindowProxy. However, since a
> remote
> > context is never entered, DOMDataStore::current() will not be able to lookup
> the
> > right DOMDataStore--so what happens instead is the global object association
> > will be cached on the DOMDataStore of whatever the current context happened
to
> > be at the time: this will typically be the accessing context.
> 
> My understanding is that we're retrieving a DOMDataStore via v8::Context, via
> ScriptState, but the DOMDataStore exists per world, not per context.  So, it
> doesn't matter through which context you retrieve a DOMDataStore.  It matters
in
> which world, you retrieve a DOMDataStore.  Because the accessing context must
be
> in the same world, it's okay to retrieve a DOMDataStore through the accessing
> context instead of the remote context.

Ah, you're right: this is my mistake.

> 
> 
> > There are also
> > times when we won't even have a current context (such as during frame swap),
> and
> > so we need a workaround for that as well. For these reasons, it's
non-trivial
> to
> > store this association in DOMDataStore at all.
> 
> If you don't have a current context, it's strange to access to any v8::Object
> (including DOM wrapper objects).  When ToV8() is called, there must exist the
> current context.  V8 APIs may throw an exception and we need a v8::Context. 
If
> you don't have a current context, you shouldn't use V8 APIs nor access to
> v8::Object in general (except for a few special APIs).

Right, but initializing a context is one of those APIs.

> 
> Plus, without remote frames, we're already swapping frames, right?  This
problem
> doesn't seem remote-frame-specific.

The problem is hidden right now, because RemoteWindowProxy (and
LocalWindowProxy) both enter the context after creating it, but before setting
up the window prototype chain. So when we call associateObjectWithWrapper, we'll
have a current context.

> 
> 
> Probably, DOMWrapperWorld / DOMDataStore should be retrievable without
> ScriptState / v8::Context.  But it doesn't mean that we cannot store wrapper
> objects in DOMDataStore.
> 
> This is my understanding.  Please correct me if something is wrong.

OK, so I was fundamentally misunderstanding how DOMDataStore works. However, we
still have a problem: associateObjectWithWrapper depends on the current context
to find the current world. For WindowProxy, this is unnecssary, because a
WindowProxy already knows what world it's associated with.

However, I think we probably don't want to expose a general
associateObjectWithWrapper() method that accepts a world argument: would you be
OK with duplicating the wrapper association logic into WindowProxy? Then
WindowProxy can directly update the wrapper on the DOMDataStore for its
associated world.

[Yuki, 2017-03-22 08:21:00]

On 2017/03/22 08:10:28, dcheng wrote:
> On 2017/03/22 07:19:09, Yuki wrote:
> > On 2017/03/21 17:45:32, dcheng wrote:
> > > The problem comes down to this: what DOMDataStore is the right one to
store
> a
> > > remote global object association in? For consistency, we should be storing
> it
> > on
> > > the DOMDataStore associated with the RemoteWindowProxy. However, since a
> > remote
> > > context is never entered, DOMDataStore::current() will not be able to
lookup
> > the
> > > right DOMDataStore--so what happens instead is the global object
association
> > > will be cached on the DOMDataStore of whatever the current context
happened
> to
> > > be at the time: this will typically be the accessing context.
> > 
> > My understanding is that we're retrieving a DOMDataStore via v8::Context,
via
> > ScriptState, but the DOMDataStore exists per world, not per context.  So, it
> > doesn't matter through which context you retrieve a DOMDataStore.  It
matters
> in
> > which world, you retrieve a DOMDataStore.  Because the accessing context
must
> be
> > in the same world, it's okay to retrieve a DOMDataStore through the
accessing
> > context instead of the remote context.
> 
> Ah, you're right: this is my mistake.
> 
> > 
> > 
> > > There are also
> > > times when we won't even have a current context (such as during frame
swap),
> > and
> > > so we need a workaround for that as well. For these reasons, it's
> non-trivial
> > to
> > > store this association in DOMDataStore at all.
> > 
> > If you don't have a current context, it's strange to access to any
v8::Object
> > (including DOM wrapper objects).  When ToV8() is called, there must exist
the
> > current context.  V8 APIs may throw an exception and we need a v8::Context. 
> If
> > you don't have a current context, you shouldn't use V8 APIs nor access to
> > v8::Object in general (except for a few special APIs).
> 
> Right, but initializing a context is one of those APIs.
> 
> > 
> > Plus, without remote frames, we're already swapping frames, right?  This
> problem
> > doesn't seem remote-frame-specific.
> 
> The problem is hidden right now, because RemoteWindowProxy (and
> LocalWindowProxy) both enter the context after creating it, but before setting
> up the window prototype chain. So when we call associateObjectWithWrapper,
we'll
> have a current context.
> 
> > 
> > 
> > Probably, DOMWrapperWorld / DOMDataStore should be retrievable without
> > ScriptState / v8::Context.  But it doesn't mean that we cannot store wrapper
> > objects in DOMDataStore.
> > 
> > This is my understanding.  Please correct me if something is wrong.
> 
> OK, so I was fundamentally misunderstanding how DOMDataStore works. However,
we
> still have a problem: associateObjectWithWrapper depends on the current
context
> to find the current world. For WindowProxy, this is unnecssary, because a
> WindowProxy already knows what world it's associated with.
> 
> However, I think we probably don't want to expose a general
> associateObjectWithWrapper() method that accepts a world argument: would you
be
> OK with duplicating the wrapper association logic into WindowProxy? Then
> WindowProxy can directly update the wrapper on the DOMDataStore for its
> associated world.

I'm totally fine with that WindowProxy associates the wrapper in a special
manner (without using associateObejctWithWrapper) as long as the wrapper is
stored in DOMDataStore so that ToV8() doesn't need a special hack for DOMWindow.
 :)

[dcheng, 2017-03-23 01:42:48]

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

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

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

[dcheng, 2017-03-23 01:47:09]

PTAL. I updated it to use the DOMDataStore even for the RemoteWindowProxy. It's
not easily testable AFAIK, since we still have the ToV8 overload for
DOMWindow...

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

https://codereview.chromium.org/2760793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:128:
windowWrapper = associateWithWrapper(window, wrapperTypeInfo, windowWrapper);
I'm kind of uncertain what the best thing to do here is; I guess we can create a
temporary and DCHECK that the values are the same? WDYT?

https://codereview.chromium.org/2760793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:131:
v8::Local<v8::Object> RemoteWindowProxy::associateWithWrapper(
An alternative is to make this a general function for WindowProxy and use it for
both LocalWindowProxy and RemoteWindowProxy. WDYT?

https://codereview.chromium.org/2760793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2760793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:63: #include
"core/frame/RemoteDOMWindow.h"
Needed, because a few things in here now are dereferencing a RemoteDOMWindow.

[dcheng, 2017-03-23 02:02:38]

On 2017/03/23 01:47:09, dcheng wrote:
> PTAL. I updated it to use the DOMDataStore even for the RemoteWindowProxy.
It's
> not easily testable AFAIK, since we still have the ToV8 overload for
> DOMWindow...
> 
>
https://codereview.chromium.org/2760793002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right):
> 
>
https://codereview.chromium.org/2760793002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:128:
> windowWrapper = associateWithWrapper(window, wrapperTypeInfo, windowWrapper);
> I'm kind of uncertain what the best thing to do here is; I guess we can create
a
> temporary and DCHECK that the values are the same? WDYT?
> 
>
https://codereview.chromium.org/2760793002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:131:
> v8::Local<v8::Object> RemoteWindowProxy::associateWithWrapper(
> An alternative is to make this a general function for WindowProxy and use it
for
> both LocalWindowProxy and RemoteWindowProxy. WDYT?
> 
>
https://codereview.chromium.org/2760793002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):
> 
>
https://codereview.chromium.org/2760793002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/tests/WebFrameTest.cpp:63: #include
> "core/frame/RemoteDOMWindow.h"
> Needed, because a few things in here now are dereferencing a RemoteDOMWindow.

Actually, I think I'm just going to move the helper function for associating the
wrapper into the WindowProxy base class... I'll upload another patchset.

[Yuki, 2017-03-23 02:58:30]

https://codereview.chromium.org/2760793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/DOMDataStore.h (right):

https://codereview.chromium.org/2760793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/DOMDataStore.h:141: friend class
RemoteWindowProxy;
I think that we don't have a strong reason to make |set| private:.  |get| is
already public:.

I'd prefer making |set| public: rather than making WindowProxy a friend class.

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

https://codereview.chromium.org/2760793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:128:
windowWrapper = associateWithWrapper(window, wrapperTypeInfo, windowWrapper);
On 2017/03/23 01:47:09, dcheng wrote:
> I'm kind of uncertain what the best thing to do here is; I guess we can create
a
> temporary and DCHECK that the values are the same? WDYT?

Yes, I think it's good.  I did the same thing in my CL which was reverted.  I
used CHECK at that time because the return value changes only if nested script
ran (this must not happen in case of Window).

https://codereview.chromium.org/2617733004/diff/270001/third_party/WebKit/Sou...

https://codereview.chromium.org/2760793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:131:
v8::Local<v8::Object> RemoteWindowProxy::associateWithWrapper(
On 2017/03/23 01:47:09, dcheng wrote:
> An alternative is to make this a general function for WindowProxy and use it
for
> both LocalWindowProxy and RemoteWindowProxy. WDYT?

Sounds good to me.

[commit-bot: I haz the power, 2017-03-23 03:29:34]

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

[commit-bot: I haz the power, 2017-03-23 03:29:36]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-03-23 06:02:09]

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

[commit-bot: I haz the power, 2017-03-23 06:02:19]

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

[dcheng, 2017-03-23 06:03:06]

PTAL

https://codereview.chromium.org/2760793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/DOMDataStore.h (right):

https://codereview.chromium.org/2760793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/DOMDataStore.h:141: friend class
RemoteWindowProxy;
On 2017/03/23 02:58:29, Yuki wrote:
> I think that we don't have a strong reason to make |set| private:.  |get| is
> already public:.
> 
> I'd prefer making |set| public: rather than making WindowProxy a friend class.

OK, I was thinking we didn't want to expose this more widely, but if you're OK
with exposing it publicly, I guess that's fine.

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

https://codereview.chromium.org/2760793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:128:
windowWrapper = associateWithWrapper(window, wrapperTypeInfo, windowWrapper);
On 2017/03/23 02:58:29, Yuki wrote:
> On 2017/03/23 01:47:09, dcheng wrote:
> > I'm kind of uncertain what the best thing to do here is; I guess we can
create
> a
> > temporary and DCHECK that the values are the same? WDYT?
> 
> Yes, I think it's good.  I did the same thing in my CL which was reverted.  I
> used CHECK at that time because the return value changes only if nested script
> ran (this must not happen in case of Window).
> 
>
https://codereview.chromium.org/2617733004/diff/270001/third_party/WebKit/Sou...

Done.

[commit-bot: I haz the power, 2017-03-23 06:19:03]

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

[commit-bot: I haz the power, 2017-03-23 06:19:04]

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

[Yuki, 2017-03-23 07:36:47]

LGTM.

[dcheng, 2017-03-23 07:37:24]

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

[commit-bot: I haz the power, 2017-03-23 07:37:42]

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-03-23 08:00:40]

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

[commit-bot: I haz the power, 2017-03-23 08:00:41]

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

[dcheng, 2017-03-23 17:59:53]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2017-03-23 17:59:53]

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/2760793002/#ps100001
(title: "Clean up association logic")

[commit-bot: I haz the power, 2017-03-23 18:00:39]

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-03-23 18:22:06]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490291993443770,
"parent_rev": "f70d58032706670f094ed7ae73c2bb13259fc0b6", "commit_rev":
"a94ee9eb38e2003712667dfe217a5cb1a1763dd0"}

[commit-bot: I haz the power, 2017-03-23 18:22:51]

Description was changed from

==========
Use v8::Context::NewRemoteContext in RemoteWindowProxy.

A RemoteWindowProxy manages the state for the WindowProxy of a
RemoteFrame. By definition, a RemoteFrame is always cross-origin.
Today, RemoteWindowProxy uses v8::Context; however, a full v8 Context
is fairly heavyweight and includes prototypes for many objects that
will never be used (since the context is always considered cross
origin).

v8::Context::NewRemoteContext is intended to help reduce the memory
overhead of remote frames; it simply instantiates a JSGlobalProxy and
a shim global object that are very lightweight, since it skips all the
work to set up prototypes for String, Number, Boolean, etc that will
never be used.

BUG=527190
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Use v8::Context::NewRemoteContext in RemoteWindowProxy.

A RemoteWindowProxy manages the state for the WindowProxy of a
RemoteFrame. By definition, a RemoteFrame is always cross-origin.
Today, RemoteWindowProxy uses v8::Context; however, a full v8 Context
is fairly heavyweight and includes prototypes for many objects that
will never be used (since the context is always considered cross
origin).

v8::Context::NewRemoteContext is intended to help reduce the memory
overhead of remote frames; it simply instantiates a JSGlobalProxy and
a shim global object that are very lightweight, since it skips all the
work to set up prototypes for String, Number, Boolean, etc that will
never be used.

BUG=527190
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2760793002
Cr-Commit-Position: refs/heads/master@{#459151}
Committed:
https://chromium.googlesource.com/chromium/src/+/a94ee9eb38e2003712667dfe217a...
==========

[commit-bot: I haz the power, 2017-03-23 18:22:52]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/a94ee9eb38e2003712667dfe217a...

[jochen (gone - plz use gerrit), 2017-03-23 18:27:48]

https://codereview.chromium.org/2760793002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2760793002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:8961: WebScriptSource("try
{ '' + window[2]; } catch (e) { e; }"));
Object.prototype.toString.call(window[2]) should work, no?

[dcheng, 2017-03-23 18:31:51]

https://codereview.chromium.org/2760793002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2760793002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:8961: WebScriptSource("try
{ '' + window[2]; } catch (e) { e; }"));
On 2017/03/23 18:27:48, jochen wrote:
> Object.prototype.toString.call(window[2]) should work, no?

Hmm... that's one version I forgot to try. I'll upload a followup CL if this
makes a difference.