[Binding] Do not install document into hidden Window object

Do not install document into hidden Window object, i.e. inner global Window.
Now we keep document in LocalDOMWindow, and do not have to keep it alive in hidden window object.

BUG=None

Committed: https://crrev.com/150b7e565c1821218cc3d29a1a9c9edb8dc0d08a
Cr-Commit-Position: refs/heads/master@{#383912}

[peria, 2016-03-29 07:41:52]

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

[commit-bot: I haz the power, 2016-03-29 07:42:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1836193002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1836193002/1

[peria, 2016-03-29 08:24:37]

Description was changed from

==========
Do not install document as a hidden propertyy

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

to

==========
Do not install document into hidden Window object, i.e.
inner global Window.

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

[commit-bot: I haz the power, 2016-03-29 09:11:31]

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

[commit-bot: I haz the power, 2016-03-29 09:11:31]

Dry run: This issue passed the CQ dry run.

[peria, 2016-03-29 09:33:26]

Description was changed from

==========
Do not install document into hidden Window object, i.e.
inner global Window.

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

to

==========
Do not install document into hidden Window object,
i.e. inner global Window.

BUG=None
==========

[peria, 2016-03-29 09:33:55]

Description was changed from

==========
Do not install document into hidden Window object,
i.e. inner global Window.

BUG=None
==========

to

==========
Do not install document into hidden Window object,
i.e. inner global Window.
We do not need to do it.

BUG=None
==========

[peria, 2016-03-29 09:46:55]

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

[peria, 2016-03-29 09:46:56]

PTL

[haraken, 2016-03-29 09:49:37]

I'm supportive to this change, but would you git-blame a CL that added the lines
to see why they were added?

[peria, 2016-03-29 10:05:53]

On 2016/03/29 09:49:37, haraken wrote:
> I'm supportive to this change, but would you git-blame a CL that added the
lines
> to see why they were added?

https://chromium.googlesource.com/chromium/src/+/ab103068b25fc507121f368e3aaa...
The CL ensures that DOMWindows that we obtain from V8 always have non-null
Document objects.
(It say that V8 GC has been able to collect Documents before DOMWindows.)

I'm not sure the issue still happens, although this CL passes the test.

[Yuki, 2016-03-29 10:20:52]

On 2016/03/29 10:05:53, peria wrote:
> On 2016/03/29 09:49:37, haraken wrote:
> > I'm supportive to this change, but would you git-blame a CL that added the
> lines
> > to see why they were added?
> 
>
https://chromium.googlesource.com/chromium/src/+/ab103068b25fc507121f368e3aaa...
> The CL ensures that DOMWindows that we obtain from V8 always have non-null
> Document objects.
> (It say that V8 GC has been able to collect Documents before DOMWindows.)
> 
> I'm not sure the issue still happens, although this CL passes the test.

As long as we're doing ForceSet(window, 'document'), the document shouldn't be
collected before the window gets being collected, right?

Plus, I don't understand why we need to keep the document *wrapper* alive.  Is
there any reason?

By the way, you should update V8_HIDDEN_VALUES in
bindings/core/v8/V8HiddenValue.h, too.

[Yuki, 2016-03-29 10:20:55]

On 2016/03/29 10:05:53, peria wrote:
> On 2016/03/29 09:49:37, haraken wrote:
> > I'm supportive to this change, but would you git-blame a CL that added the
> lines
> > to see why they were added?
> 
>
https://chromium.googlesource.com/chromium/src/+/ab103068b25fc507121f368e3aaa...
> The CL ensures that DOMWindows that we obtain from V8 always have non-null
> Document objects.
> (It say that V8 GC has been able to collect Documents before DOMWindows.)
> 
> I'm not sure the issue still happens, although this CL passes the test.

As long as we're doing ForceSet(window, 'document'), the document shouldn't be
collected before the window gets being collected, right?

Plus, I don't understand why we need to keep the document *wrapper* alive.  Is
there any reason?

By the way, you should update V8_HIDDEN_VALUES in
bindings/core/v8/V8HiddenValue.h, too.

[haraken, 2016-03-29 10:50:45]

On 2016/03/29 10:20:55, Yuki wrote:
> On 2016/03/29 10:05:53, peria wrote:
> > On 2016/03/29 09:49:37, haraken wrote:
> > > I'm supportive to this change, but would you git-blame a CL that added the
> > lines
> > > to see why they were added?
> > 
> >
>
https://chromium.googlesource.com/chromium/src/+/ab103068b25fc507121f368e3aaa...
> > The CL ensures that DOMWindows that we obtain from V8 always have non-null
> > Document objects.
> > (It say that V8 GC has been able to collect Documents before DOMWindows.)
> > 
> > I'm not sure the issue still happens, although this CL passes the test.
> 
> As long as we're doing ForceSet(window, 'document'), the document shouldn't be
> collected before the window gets being collected, right?
> 
> Plus, I don't understand why we need to keep the document *wrapper* alive.  Is
> there any reason?

Yeah, I don't see any reason we have to keep the wrapper alive. I think it's
enough as long as the C++ Document object is kept alive. (This is already
guaranteed by LocalDOMWindow::m_document.)

> 
> By the way, you should update V8_HIDDEN_VALUES in
> bindings/core/v8/V8HiddenValue.h, too.

^^^ Once this is fixed, LGTM.

[haraken, 2016-03-29 10:51:02]

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

[haraken, 2016-03-29 10:51:02]

+jochen FYI

[peria, 2016-03-30 03:16:19]

On 2016/03/29 10:50:45, haraken wrote:
> On 2016/03/29 10:20:55, Yuki wrote:
> > On 2016/03/29 10:05:53, peria wrote:
> > > On 2016/03/29 09:49:37, haraken wrote:
> > > > I'm supportive to this change, but would you git-blame a CL that added
the
> > > lines
> > > > to see why they were added?
> > > 
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/ab103068b25fc507121f368e3aaa...
> > > The CL ensures that DOMWindows that we obtain from V8 always have non-null
> > > Document objects.
> > > (It say that V8 GC has been able to collect Documents before DOMWindows.)
> > > 
> > > I'm not sure the issue still happens, although this CL passes the test.
> > 
> > As long as we're doing ForceSet(window, 'document'), the document shouldn't
be
> > collected before the window gets being collected, right?
> > 
> > Plus, I don't understand why we need to keep the document *wrapper* alive. 
Is
> > there any reason?
> 
> Yeah, I don't see any reason we have to keep the wrapper alive. I think it's
> enough as long as the C++ Document object is kept alive. (This is already
> guaranteed by LocalDOMWindow::m_document.)
> 

LocalDOMWindow::m_document was added in
https://chromium.googlesource.com/chromium/src/+/f8ac2889f778268837b206b59440...,
which was committed after the CL in #11.

I agree that we don't have to keep the document alive in the inner global
window.

> > 
> > By the way, you should update V8_HIDDEN_VALUES in
> > bindings/core/v8/V8HiddenValue.h, too.
> 
> ^^^ Once this is fixed, LGTM.
done.

[peria, 2016-03-30 03:18:00]

Description was changed from

==========
Do not install document into hidden Window object,
i.e. inner global Window.
We do not need to do it.

BUG=None
==========

to

==========
Do not install document into hidden Window object, i.e. inner global Window.
Now we keep document in LocalDOMWindow, and do not have to keep it alive in
hidden window object.

BUG=None
==========

[Yuki, 2016-03-30 03:24:26]

Thanks for the investigation.  LGTM.

[peria, 2016-03-30 03:58:34]

The CQ bit was checked by peria@chromium.org

[peria, 2016-03-30 03:58:34]

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/1836193002/#ps20001
(title: "Drop document from a hidden value list")

[commit-bot: I haz the power, 2016-03-30 03:58:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1836193002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1836193002/20001

[commit-bot: I haz the power, 2016-03-30 05:33:54]

Description was changed from

==========
Do not install document into hidden Window object, i.e. inner global Window.
Now we keep document in LocalDOMWindow, and do not have to keep it alive in
hidden window object.

BUG=None
==========

to

==========
Do not install document into hidden Window object, i.e. inner global Window.
Now we keep document in LocalDOMWindow, and do not have to keep it alive in
hidden window object.

BUG=None
==========

[commit-bot: I haz the power, 2016-03-30 05:33:55]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-30 05:35:18]

Description was changed from

==========
Do not install document into hidden Window object, i.e. inner global Window.
Now we keep document in LocalDOMWindow, and do not have to keep it alive in
hidden window object.

BUG=None
==========

to

==========
Do not install document into hidden Window object, i.e. inner global Window.
Now we keep document in LocalDOMWindow, and do not have to keep it alive in
hidden window object.

BUG=None

Committed: https://crrev.com/150b7e565c1821218cc3d29a1a9c9edb8dc0d08a
Cr-Commit-Position: refs/heads/master@{#383912}
==========

[commit-bot: I haz the power, 2016-03-30 05:35:20]

Patchset 2 (id:??) landed as
https://crrev.com/150b7e565c1821218cc3d29a1a9c9edb8dc0d08a
Cr-Commit-Position: refs/heads/master@{#383912}