v8binding: Makes ToV8(window) work without a frame.

v8binding: Makes ToV8(window) work without a frame.

Makes DOMWindow hold a WindowProxyManager and makes
it possible that ToV8(window) work without a frame.

With this CL, all tests in
external/wpt/html/webappapis/scripting/events/compile-event-handler-settings-objects.html
pass without a failure.

This CL is a preparation for
https://crrev.com/2693893007

BUG=

Review-Url: https://codereview.chromium.org/2713623002
Cr-Commit-Position: refs/heads/master@{#464907}
Committed: https://chromium.googlesource.com/chromium/src/+/58af3664facba6332528fd983988d51cf3636af4

[Yuki, 2017-02-22 10:27:43]

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

[commit-bot: I haz the power, 2017-02-22 10:28:09]

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-02-22 11:17:23]

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

[commit-bot: I haz the power, 2017-02-22 11:17:24]

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

[Yuki, 2017-03-22 14:00:15]

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

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

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

[commit-bot: I haz the power, 2017-03-22 15:34:19]

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

[commit-bot: I haz the power, 2017-03-22 15:34:19]

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

[Yuki, 2017-03-23 10:28:04]

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

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

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 11:21:24]

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

[commit-bot: I haz the power, 2017-03-23 11:21:25]

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

[Yuki, 2017-03-31 14:40:35]

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

[commit-bot: I haz the power, 2017-03-31 14:40:59]

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-31 16:07:38]

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

[commit-bot: I haz the power, 2017-03-31 16:07:39]

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

[Yuki, 2017-04-05 14:37:51]

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

[commit-bot: I haz the power, 2017-04-05 14:38:28]

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

[commit-bot: I haz the power, 2017-04-05 16:07:47]

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

[commit-bot: I haz the power, 2017-04-05 16:07:48]

Dry run: This issue passed the CQ dry run.

[Yuki, 2017-04-06 09:25:16]

Description was changed from

==========
binding: Make ToV8(window) work without a frame.

BUG=
==========

to

==========
v8binding: Makes ToV8(window) work without a frame.

Makes DOMWindow hold a WindowProxyManager and makes
it possible that ToV8(window) work without a frame.

With this CL, all tests in
external/wpt/html/webappapis/scripting/events/compile-event-handler-settings-objects.html
pass without a failure.

This CL is a preparation for
https://crrev.com/2693893007

BUG=
==========

[Yuki, 2017-04-06 09:26:51]

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

[commit-bot: I haz the power, 2017-04-06 09:27:08]

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

[Yuki, 2017-04-06 09:32:28]

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

[Yuki, 2017-04-06 09:32:29]

Could you guys review this CL?

[commit-bot: I haz the power, 2017-04-06 10:45:07]

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

[commit-bot: I haz the power, 2017-04-06 10:45:08]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-04-06 17:25:27]

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

https://codereview.chromium.org/2713623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/DOMWindow.h:129: // of this object.

I don't fully understand why DOMWindow needs to hold m_windowProxyManager. If
the window's frame is detached, DOMWindow::globalProxy() is expected to return
an empty handle. Then wouldn't it be enough to hold m_frame and return a valid
global proxy only when m_frame is not null?

https://codereview.chromium.org/2713623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/DOMWindow.h:143: // because WindowProxy has
|m_globalProxy| as a ScopedPersistent.  We need

Can we break the cycle by replacing the ScopedPersistent with
TraceWrapperV8Reference?

[dcheng, 2017-04-07 06:23:12]

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

https://codereview.chromium.org/2713623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/DOMWindow.h:129: // of this object.
On 2017/04/06 17:25:27, haraken wrote:
> 
> I don't fully understand why DOMWindow needs to hold m_windowProxyManager. If
> the window's frame is detached, DOMWindow::globalProxy() is expected to return
> an empty handle. Then wouldn't it be enough to hold m_frame and return a valid
> global proxy only when m_frame is not null?

Right now it does, but maybe it shouldn't. According to the spec in
https://html.spec.whatwg.org/multipage/browsers.html#dom-self: The window,
frames, and self IDL attributes, on getting, must all return this Window
object's browsing context's WindowProxy object.

The spec is not clear if a detached Window still has a browsing context.

(I guess you could argue that the window doesn't have a browsing context once
it's removed; however, this is inconsistent from the behavior of Firefox. Edge
seems to throw an error in this case)

[Yuki, 2017-04-07 08:40:20]

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

https://codereview.chromium.org/2713623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/DOMWindow.h:129: // of this object.
On 2017/04/07 06:23:12, dcheng wrote:
> On 2017/04/06 17:25:27, haraken wrote:
> > 
> > I don't fully understand why DOMWindow needs to hold m_windowProxyManager.
If
> > the window's frame is detached, DOMWindow::globalProxy() is expected to
return
> > an empty handle. Then wouldn't it be enough to hold m_frame and return a
valid
> > global proxy only when m_frame is not null?
> 
> Right now it does, but maybe it shouldn't. According to the spec in
> https://html.spec.whatwg.org/multipage/browsers.html#dom-self: The window,
> frames, and self IDL attributes, on getting, must all return this Window
> object's browsing context's WindowProxy object.

My understanding is that we agreed that we need to support a browsing context in
a detached iframe mainly because author script can have references to it and
there is no way to avoid it.

Having said that, we still have an option to return null for win.self where
|win| is a saved reference.  However, as dcheng@ wrote, the spec requires that
|self| always returns non-null WindowProxy (of the spec, not
blink::WindowProxy).

> The spec is not clear if a detached Window still has a browsing context.
> 
> (I guess you could argue that the window doesn't have a browsing context once
> it's removed; however, this is inconsistent from the behavior of Firefox. Edge
> seems to throw an error in this case)

I asked this point before:
https://github.com/whatwg/html/issues/1279

And at that time, IIUC, the answer is that we should maintain the browsing
context as long as author script has a reference to it.  Note that, in the spec,
"nested browsing context" is a different concept from "browsing context".  IIUC,
"nested browsing context" is a kind of slot to store a reference to a browsing
context or null.

Also there is WPT for detached cases.

In short, we need to support win.self, etc. in a detached iframe.

https://codereview.chromium.org/2713623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/DOMWindow.h:143: // because WindowProxy has
|m_globalProxy| as a ScopedPersistent.  We need
On 2017/04/06 17:25:27, haraken wrote:
> 
> Can we break the cycle by replacing the ScopedPersistent with
> TraceWrapperV8Reference?

I don't fully understand this point and I'm nore sure if it's possible or not at
this moment.

I thought that WindowProxy::m_globalProxy is the root reference.  If so,
TraceWrapperV8Reference wouldn't work.  IIUC, ScriptState::m_context is a weak
reference, so it cannot be the root reference.

I think that, if we don't have a root reference, we cannot keep a browsing
context alive.

[haraken, 2017-04-07 10:08:58]

https://codereview.chromium.org/2713623002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/2713623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/DOMWindow.cpp:424: return
m_windowProxyManager->windowProxy(world)->globalProxyIfNotDetached();

So, are you planning to change this in the future to return a valid global proxy
even after the frame gets detached?

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

https://codereview.chromium.org/2713623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/DOMWindow.h:143: // because WindowProxy has
|m_globalProxy| as a ScopedPersistent.  We need
On 2017/04/07 08:40:19, Yuki wrote:
> On 2017/04/06 17:25:27, haraken wrote:
> > 
> > Can we break the cycle by replacing the ScopedPersistent with
> > TraceWrapperV8Reference?
> 
> I don't fully understand this point and I'm nore sure if it's possible or not
at
> this moment.
> 
> I thought that WindowProxy::m_globalProxy is the root reference.  If so,
> TraceWrapperV8Reference wouldn't work.  IIUC, ScriptState::m_context is a weak
> reference, so it cannot be the root reference.
> 
> I think that, if we don't have a root reference, we cannot keep a browsing
> context alive.

Ah, makes sense.

However, I don't fully understand why using a Member causes a leak. When a frame
navigates to a new window, the reference from m_globalProxy to the old global
object will be broken. Then:

1) A V8 GC collects the old global object (because it is unreachable from
m_globalProxy).
2) An oilpan GC collects the corresponding C++ DOMWindow, WindowProxyManager etc
(even if we use a Member here).

[Yuki, 2017-04-07 10:30:44]

https://codereview.chromium.org/2713623002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/2713623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/DOMWindow.cpp:424: return
m_windowProxyManager->windowProxy(world)->globalProxyIfNotDetached();
On 2017/04/07 10:08:58, haraken wrote:
> 
> So, are you planning to change this in the future to return a valid global
proxy
> even after the frame gets detached?

Yes, I hope that I can make it someday.

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

https://codereview.chromium.org/2713623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/DOMWindow.h:143: // because WindowProxy has
|m_globalProxy| as a ScopedPersistent.  We need
On 2017/04/07 10:08:58, haraken wrote:
> On 2017/04/07 08:40:19, Yuki wrote:
> > On 2017/04/06 17:25:27, haraken wrote:
> > > 
> > > Can we break the cycle by replacing the ScopedPersistent with
> > > TraceWrapperV8Reference?
> > 
> > I don't fully understand this point and I'm nore sure if it's possible or
not
> at
> > this moment.
> > 
> > I thought that WindowProxy::m_globalProxy is the root reference.  If so,
> > TraceWrapperV8Reference wouldn't work.  IIUC, ScriptState::m_context is a
weak
> > reference, so it cannot be the root reference.
> > 
> > I think that, if we don't have a root reference, we cannot keep a browsing
> > context alive.
> 
> Ah, makes sense.
> 
> However, I don't fully understand why using a Member causes a leak. When a
frame
> navigates to a new window, the reference from m_globalProxy to the old global
> object will be broken. Then:
> 
> 1) A V8 GC collects the old global object (because it is unreachable from
> m_globalProxy).
> 2) An oilpan GC collects the corresponding C++ DOMWindow, WindowProxyManager
etc
> (even if we use a Member here).

We have another case: iframe is detacehd from the DOM.

In this case, m_globalProxy still points to the same global object.

[haraken, 2017-04-07 10:42:12]

On 2017/04/07 10:30:44, Yuki wrote:
>
https://codereview.chromium.org/2713623002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/DOMWindow.cpp (right):
> 
>
https://codereview.chromium.org/2713623002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/DOMWindow.cpp:424: return
> m_windowProxyManager->windowProxy(world)->globalProxyIfNotDetached();
> On 2017/04/07 10:08:58, haraken wrote:
> > 
> > So, are you planning to change this in the future to return a valid global
> proxy
> > even after the frame gets detached?
> 
> Yes, I hope that I can make it someday.
> 
>
https://codereview.chromium.org/2713623002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/DOMWindow.h (right):
> 
>
https://codereview.chromium.org/2713623002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/DOMWindow.h:143: // because WindowProxy
has
> |m_globalProxy| as a ScopedPersistent.  We need
> On 2017/04/07 10:08:58, haraken wrote:
> > On 2017/04/07 08:40:19, Yuki wrote:
> > > On 2017/04/06 17:25:27, haraken wrote:
> > > > 
> > > > Can we break the cycle by replacing the ScopedPersistent with
> > > > TraceWrapperV8Reference?
> > > 
> > > I don't fully understand this point and I'm nore sure if it's possible or
> not
> > at
> > > this moment.
> > > 
> > > I thought that WindowProxy::m_globalProxy is the root reference.  If so,
> > > TraceWrapperV8Reference wouldn't work.  IIUC, ScriptState::m_context is a
> weak
> > > reference, so it cannot be the root reference.
> > > 
> > > I think that, if we don't have a root reference, we cannot keep a browsing
> > > context alive.
> > 
> > Ah, makes sense.
> > 
> > However, I don't fully understand why using a Member causes a leak. When a
> frame
> > navigates to a new window, the reference from m_globalProxy to the old
global
> > object will be broken. Then:
> > 
> > 1) A V8 GC collects the old global object (because it is unreachable from
> > m_globalProxy).
> > 2) An oilpan GC collects the corresponding C++ DOMWindow, WindowProxyManager
> etc
> > (even if we use a Member here).
> 
> We have another case: iframe is detacehd from the DOM.
> 
> In this case, m_globalProxy still points to the same global object.

In that case, we should just keep alive the global object etc. So not collecting
the global object etc is just fine... right?

[Yuki, 2017-04-07 10:53:37]

On 2017/04/07 10:42:12, haraken wrote:
> > On 2017/04/07 10:08:58, haraken wrote:
> > > On 2017/04/07 08:40:19, Yuki wrote:
> > > > On 2017/04/06 17:25:27, haraken wrote:
> > > > > 
> > > > > Can we break the cycle by replacing the ScopedPersistent with
> > > > > TraceWrapperV8Reference?
> > > > 
> > > > I don't fully understand this point and I'm nore sure if it's possible
or
> > not
> > > at
> > > > this moment.
> > > > 
> > > > I thought that WindowProxy::m_globalProxy is the root reference.  If so,
> > > > TraceWrapperV8Reference wouldn't work.  IIUC, ScriptState::m_context is
a
> > weak
> > > > reference, so it cannot be the root reference.
> > > > 
> > > > I think that, if we don't have a root reference, we cannot keep a
browsing
> > > > context alive.
> > > 
> > > Ah, makes sense.
> > > 
> > > However, I don't fully understand why using a Member causes a leak. When a
> > frame
> > > navigates to a new window, the reference from m_globalProxy to the old
> global
> > > object will be broken. Then:
> > > 
> > > 1) A V8 GC collects the old global object (because it is unreachable from
> > > m_globalProxy).
> > > 2) An oilpan GC collects the corresponding C++ DOMWindow,
WindowProxyManager
> > etc
> > > (even if we use a Member here).
> > 
> > We have another case: iframe is detacehd from the DOM.
> > 
> > In this case, m_globalProxy still points to the same global object.
> 
> In that case, we should just keep alive the global object etc. So not
collecting
> the global object etc is just fine... right?

It's possible that author script do not have any reference to the window object
that was in the detached iframe.  When detached, iframe.contentWindow no longer
returns the window proxy, it returns null.  So, it's okay to collect the window
object and context as long as there is no reference to it.  We have such GC
tests.

[haraken, 2017-04-07 11:35:03]

On 2017/04/07 10:53:37, Yuki wrote:
> On 2017/04/07 10:42:12, haraken wrote:
> > > On 2017/04/07 10:08:58, haraken wrote:
> > > > On 2017/04/07 08:40:19, Yuki wrote:
> > > > > On 2017/04/06 17:25:27, haraken wrote:
> > > > > > 
> > > > > > Can we break the cycle by replacing the ScopedPersistent with
> > > > > > TraceWrapperV8Reference?
> > > > > 
> > > > > I don't fully understand this point and I'm nore sure if it's possible
> or
> > > not
> > > > at
> > > > > this moment.
> > > > > 
> > > > > I thought that WindowProxy::m_globalProxy is the root reference.  If
so,
> > > > > TraceWrapperV8Reference wouldn't work.  IIUC, ScriptState::m_context
is
> a
> > > weak
> > > > > reference, so it cannot be the root reference.
> > > > > 
> > > > > I think that, if we don't have a root reference, we cannot keep a
> browsing
> > > > > context alive.
> > > > 
> > > > Ah, makes sense.
> > > > 
> > > > However, I don't fully understand why using a Member causes a leak. When
a
> > > frame
> > > > navigates to a new window, the reference from m_globalProxy to the old
> > global
> > > > object will be broken. Then:
> > > > 
> > > > 1) A V8 GC collects the old global object (because it is unreachable
from
> > > > m_globalProxy).
> > > > 2) An oilpan GC collects the corresponding C++ DOMWindow,
> WindowProxyManager
> > > etc
> > > > (even if we use a Member here).
> > > 
> > > We have another case: iframe is detacehd from the DOM.
> > > 
> > > In this case, m_globalProxy still points to the same global object.
> > 
> > In that case, we should just keep alive the global object etc. So not
> collecting
> > the global object etc is just fine... right?
> 
> It's possible that author script do not have any reference to the window
object
> that was in the detached iframe.  When detached, iframe.contentWindow no
longer
> returns the window proxy, it returns null.  So, it's okay to collect the
window
> object and context as long as there is no reference to it.  We have such GC
> tests.

Discussed offline. I agree that there will be no cleaner solution than this.

Let's change the WeakMember with UntracedMember. With that change, LGTM.

[Yuki, 2017-04-07 12:48:37]

On 2017/04/07 11:35:03, haraken wrote:
> Let's change the WeakMember with UntracedMember. With that change, LGTM.

I thought this point again, and now I think WeakMember is safer.

If something went wrong, a WeakMember will be nullptr, so it crashes safely. 
But UntracedMember won't be nullptr, right?  Then, it's more dangerous to access
a dead object (freed memory area) than to access nullptr.  Null crashes are
easier to debug, too.

So, I'd prefer WeakMember to UntracedMember here.

[Yuki, 2017-04-07 12:48:52]

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

[commit-bot: I haz the power, 2017-04-07 12:49:01]

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

[haraken, 2017-04-07 14:01:50]

On 2017/04/07 12:48:37, Yuki wrote:
> On 2017/04/07 11:35:03, haraken wrote:
> > Let's change the WeakMember with UntracedMember. With that change, LGTM.
> 
> I thought this point again, and now I think WeakMember is safer.
> 
> If something went wrong, a WeakMember will be nullptr, so it crashes safely. 
> But UntracedMember won't be nullptr, right?  Then, it's more dangerous to
access
> a dead object (freed memory area) than to access nullptr.  Null crashes are
> easier to debug, too.
> 
> So, I'd prefer WeakMember to UntracedMember here.

OK, LGTM.

Maybe shall we add CHECK(!m_windowProxyManager) ?

[commit-bot: I haz the power, 2017-04-07 14:08:40]

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

[commit-bot: I haz the power, 2017-04-07 14:08:41]

Dry run: This issue passed the CQ dry run.

[Yuki, 2017-04-07 15:03:37]

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

[commit-bot: I haz the power, 2017-04-07 15:03:58]

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

[Yuki, 2017-04-07 15:04:35]

On 2017/04/07 14:01:50, haraken wrote:
> On 2017/04/07 12:48:37, Yuki wrote:
> > On 2017/04/07 11:35:03, haraken wrote:
> > > Let's change the WeakMember with UntracedMember. With that change, LGTM.
> > 
> > I thought this point again, and now I think WeakMember is safer.
> > 
> > If something went wrong, a WeakMember will be nullptr, so it crashes safely.

> > But UntracedMember won't be nullptr, right?  Then, it's more dangerous to
> access
> > a dead object (freed memory area) than to access nullptr.  Null crashes are
> > easier to debug, too.
> > 
> > So, I'd prefer WeakMember to UntracedMember here.
> 
> OK, LGTM.
> 
> Maybe shall we add CHECK(!m_windowProxyManager) ?

Added DCHECK.

dcheng@, could you take another look?  I'd like to have your review, too.

[commit-bot: I haz the power, 2017-04-07 15:51:20]

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

[commit-bot: I haz the power, 2017-04-07 15:51:21]

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

[dcheng, 2017-04-07 18:48:48]

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

https://codereview.chromium.org/2713623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/DOMWindow.h:140: // available.
Thinking about this more, I wonder if this is guaranteed.

If we have something like this:

parentWindow = window.parent;
parentWindow.savedClosure = function () { parentWindow.console.log(document); };

And then we remove the child iframe from the DOM and wait for GC, what would
keep WindowProxyManager live? The frame is gone in that case, right? In that
case, what stops WindowProxyManager from being collected?

https://codereview.chromium.org/2713623002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/2713623002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/DOMWindow.cpp:424:
DCHECK(!m_windowProxyManager);
I think this check is backwards (we want to assert that it is not null)

[Yuki, 2017-04-10 09:43:32]

dcheng@ is right.  Let me change the approach overall.
I'll ping you guys when this CL is ready for another review.

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

https://codereview.chromium.org/2713623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/DOMWindow.h:140: // available.
On 2017/04/07 18:48:48, dcheng wrote:
> Thinking about this more, I wonder if this is guaranteed.
> 
> If we have something like this:
> 
> parentWindow = window.parent;
> parentWindow.savedClosure = function () { parentWindow.console.log(document);
};
> 
> And then we remove the child iframe from the DOM and wait for GC, what would
> keep WindowProxyManager live? The frame is gone in that case, right? In that
> case, what stops WindowProxyManager from being collected?

You're right.  Let me change the approach overall.

https://codereview.chromium.org/2713623002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/2713623002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/DOMWindow.cpp:424:
DCHECK(!m_windowProxyManager);
On 2017/04/07 18:48:48, dcheng wrote:
> I think this check is backwards (we want to assert that it is not null)

I was silly.  X(

[Yuki, 2017-04-14 11:16:19]

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

[commit-bot: I haz the power, 2017-04-14 11:16:30]

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-04-14 12:39:50]

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

[commit-bot: I haz the power, 2017-04-14 12:39:50]

Dry run: This issue passed the CQ dry run.

[Yuki, 2017-04-14 12:43:39]

Could you guys take another look?

I've changed the approach.  Now this CL simply makes
DOMWindow::window_proxy_manager_ a strong reference.

Because https://crrev.com/2811943003 made WindowProxy::global_proxy_ a weak
reference once the frame gets detached from the DOM, we no longer need to make
DOMWindow::window_proxy_manager_ weak.  As DOMWindows need a way to look for its
wrapper object as long as it's alive, this CL makes window_proxy_manager_ a
strong reference.

[haraken, 2017-04-14 18:51:51]

LGTM

https://codereview.chromium.org/2713623002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/2713623002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/DOMWindow.cpp:430:
->GlobalProxyIfNotDetached();

Shall we add a TODO and mention that this should return a valid global proxy
even after the frame gets detached?

[dcheng, 2017-04-14 19:21:30]

LGTM

[Yuki, 2017-04-17 07:15:26]

https://codereview.chromium.org/2713623002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/2713623002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/DOMWindow.cpp:430:
->GlobalProxyIfNotDetached();
On 2017/04/14 18:51:50, haraken wrote:
> 
> Shall we add a TODO and mention that this should return a valid global proxy
> even after the frame gets detached?

Done.

[Yuki, 2017-04-17 07:15:44]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2017-04-17 07:15:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2713623002/#ps240001
(title: "Added a TODO comment.")

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

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-04-17 08:51:12]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1492413344332700,
"parent_rev": "90d6ed690cab69c72e9a8994ed209f7b3432e44c", "commit_rev":
"58af3664facba6332528fd983988d51cf3636af4"}

[commit-bot: I haz the power, 2017-04-17 08:51:46]

Description was changed from

==========
v8binding: Makes ToV8(window) work without a frame.

Makes DOMWindow hold a WindowProxyManager and makes
it possible that ToV8(window) work without a frame.

With this CL, all tests in
external/wpt/html/webappapis/scripting/events/compile-event-handler-settings-objects.html
pass without a failure.

This CL is a preparation for
https://crrev.com/2693893007

BUG=
==========

to

==========
v8binding: Makes ToV8(window) work without a frame.

Makes DOMWindow hold a WindowProxyManager and makes
it possible that ToV8(window) work without a frame.

With this CL, all tests in
external/wpt/html/webappapis/scripting/events/compile-event-handler-settings-objects.html
pass without a failure.

This CL is a preparation for
https://crrev.com/2693893007

BUG=

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

[commit-bot: I haz the power, 2017-04-17 08:51:51]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/58af3664facba6332528fd983988...