v8binding: Makes WindowProxy::global_proxy_ a weak reference when detached.

v8binding: Makes WindowProxy::global_proxy_ a weak reference when detached.

Once the context's frame is detached from the DOM, we shouldn't hold
a strong reference to v8::Context.  This CL makes WindowProxy::global_proxy_
a weak reference once the frame is detached.

This CL is a preparation of https://crrev.com/2713623002 .

BUG=

Review-Url: https://codereview.chromium.org/2811943003
Cr-Commit-Position: refs/heads/master@{#464304}
Committed: https://chromium.googlesource.com/chromium/src/+/6bfd2af0b9ab88cd377c6902033812556e1d4edd

[Yuki, 2017-04-11 09:19:31]

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

[commit-bot: I haz the power, 2017-04-11 09:19:59]

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-11 09:20:13]

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

[Yuki, 2017-04-11 09:20:14]

Could you guys review this CL?

[commit-bot: I haz the power, 2017-04-11 10:53:11]

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

[commit-bot: I haz the power, 2017-04-11 10:53:11]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-04-11 14:32:54]

I discussed this with yukishiino@ offline and am convinced that this will be a
reasonable solution. LGTM.

Please wait for dcheng's review.

https://codereview.chromium.org/2811943003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right):

https://codereview.chromium.org/2811943003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:257: // to be
destroyed.

// |global_proxy_| needs to be made a weak reference when the frame gets
detached

[dcheng, 2017-04-11 23:37:08]

Sorry I have another dumb question =)

Why does this need to be a strong reference before frame detach? If there are no
script references to the window proxy, could it be safe to collect anyway?

[Yuki, 2017-04-12 07:57:49]

On 2017/04/11 23:37:08, dcheng wrote:
> Sorry I have another dumb question =)
> 
> Why does this need to be a strong reference before frame detach? If there are
no
> script references to the window proxy, could it be safe to collect anyway?

When there is a context X and author script has been running in X, V8 GC doesn't
collect the entire context X because a script is currently running in X. 
However, once a script has stopped running (e.g. just completed onload event and
no script is currently running), who prevents V8 GC from collecting the entire
context X?

If no one outside of context X holds a strong reference to X, V8 GC collects the
entire context X that is isolated from any other thing.  Even when a script in X
has a reference to X, it doesn't help.  V8 GC collects isolated cycles.  This is
true even for a top-level window.  Someone needs to hold a strong reference to
the context in order to keep it alive.  At least, Blink must hold a strong
reference to the top-level window's context or its equivalent (e.g. a global
proxy).  Non-top-level windows can be made alive via a strong reference from the
parent window.

In case of Blink, WindowProxy::global_proxy_ is the strong reference to the
context in order to keep the context alive, regardless of whether it's a
top-level or not.

This is my understanding.

[dcheng, 2017-04-12 21:22:15]

On 2017/04/12 07:57:49, Yuki wrote:
> On 2017/04/11 23:37:08, dcheng wrote:
> > Sorry I have another dumb question =)
> > 
> > Why does this need to be a strong reference before frame detach? If there
are
> no
> > script references to the window proxy, could it be safe to collect anyway?
> 
> When there is a context X and author script has been running in X, V8 GC
doesn't
> collect the entire context X because a script is currently running in X. 
> However, once a script has stopped running (e.g. just completed onload event
and
> no script is currently running), who prevents V8 GC from collecting the entire
> context X?
> 
> If no one outside of context X holds a strong reference to X, V8 GC collects
the
> entire context X that is isolated from any other thing.  Even when a script in
X
> has a reference to X, it doesn't help.  V8 GC collects isolated cycles.  This
is
> true even for a top-level window.  Someone needs to hold a strong reference to
> the context in order to keep it alive.  At least, Blink must hold a strong
> reference to the top-level window's context or its equivalent (e.g. a global
> proxy).  Non-top-level windows can be made alive via a strong reference from
the
> parent window.
> 
> In case of Blink, WindowProxy::global_proxy_ is the strong reference to the
> context in order to keep the context alive, regardless of whether it's a
> top-level or not.
> 
> This is my understanding.

OK, after thinking about this, I agree. LGTM

[Yuki, 2017-04-13 03:26:24]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2017-04-13 03:26:55]

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-13 05:01:37]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1492053984609200, "parent_rev":
"f6d6371a3b819d5ad6a31f25cca18e952e28d621", "commit_rev":
"6bfd2af0b9ab88cd377c6902033812556e1d4edd"}

[commit-bot: I haz the power, 2017-04-13 05:09:06]

Description was changed from

==========
v8binding: Makes WindowProxy::global_proxy_ a weak reference when detached.

Once the context's frame is detached from the DOM, we shouldn't hold
a strong reference to v8::Context.  This CL makes WindowProxy::global_proxy_
a weak reference once the frame is detached.

This CL is a preparation of https://crrev.com/2713623002 .

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

to

==========
v8binding: Makes WindowProxy::global_proxy_ a weak reference when detached.

Once the context's frame is detached from the DOM, we shouldn't hold
a strong reference to v8::Context.  This CL makes WindowProxy::global_proxy_
a weak reference once the frame is detached.

This CL is a preparation of https://crrev.com/2713623002 .

BUG=

Review-Url: https://codereview.chromium.org/2811943003
Cr-Commit-Position: refs/heads/master@{#464304}
Committed:
https://chromium.googlesource.com/chromium/src/+/6bfd2af0b9ab88cd377c69020338...
==========

[commit-bot: I haz the power, 2017-04-13 05:09:08]

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