binding: Changes the association among global-proxy/global/window-instance.

binding: Changes the association among global-proxy/global/window-instance.

Since V8 now supports internal fields for the global proxy objects,
let's change the mappings among global proxy object / global object /
C++ instance in Blink.

With this CL, a global proxy object and C++ instance point to each
other, and a global object points to the C++ instance (without
reverse pointer).

Note that this CL makes one big difference.  For non-window objects,
Blink objects always live longer than V8 wrappers.  This CL
introduces an exception; DOMWindow may be collected before the V8
wrapper object gets collected because the V8 wrapper is the global
proxy object which remains the same while navigations.

BUG=675872
Review-Url: https://codereview.chromium.org/2617733004
Cr-Commit-Position: refs/heads/master@{#449616}
Committed: https://chromium.googlesource.com/chromium/src/+/76ec9e6ea20fb4a3c1f242e39abea0067afd235a

[Yuki, 2017-01-06 15:00:30]

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

[commit-bot: I haz the power, 2017-01-06 15:00:53]

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

[commit-bot: I haz the power, 2017-01-06 15:54:17]

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

[commit-bot: I haz the power, 2017-01-06 15:54:18]

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_...)

[Yuki, 2017-01-10 07:01:35]

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

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

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

[commit-bot: I haz the power, 2017-01-10 07:54:13]

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

[commit-bot: I haz the power, 2017-01-10 07:54:14]

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

[Yuki, 2017-01-11 08:10:05]

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

[commit-bot: I haz the power, 2017-01-11 08:10:17]

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

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

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

[commit-bot: I haz the power, 2017-01-11 09:07:25]

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

[Yuki, 2017-01-11 12:58:09]

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

[commit-bot: I haz the power, 2017-01-11 12:58:24]

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

[commit-bot: I haz the power, 2017-01-11 14:02:14]

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

[commit-bot: I haz the power, 2017-01-11 14:02:15]

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

[Yuki, 2017-01-11 15:06:27]

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

[commit-bot: I haz the power, 2017-01-11 15:07:02]

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

[commit-bot: I haz the power, 2017-01-11 15:46:20]

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

[commit-bot: I haz the power, 2017-01-11 15:46:21]

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

[Yuki, 2017-01-13 06:05:36]

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

[commit-bot: I haz the power, 2017-01-13 06:05:53]

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

[commit-bot: I haz the power, 2017-01-13 06:50:57]

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

[commit-bot: I haz the power, 2017-01-13 06:50:57]

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

[Yuki, 2017-01-13 08:33:31]

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

[commit-bot: I haz the power, 2017-01-13 08:33:44]

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

[commit-bot: I haz the power, 2017-01-13 09:09:52]

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

[commit-bot: I haz the power, 2017-01-13 09:09:53]

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

[Yuki, 2017-01-13 12:17:11]

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

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

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

[commit-bot: I haz the power, 2017-01-13 12:56:45]

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

[commit-bot: I haz the power, 2017-01-13 12:56:46]

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

[Yuki, 2017-01-24 13:32:39]

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

[commit-bot: I haz the power, 2017-01-24 13:32:49]

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

[Yuki, 2017-01-24 13:44:39]

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

[commit-bot: I haz the power, 2017-01-24 13:44:57]

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

[commit-bot: I haz the power, 2017-01-24 14:24:47]

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

[commit-bot: I haz the power, 2017-01-24 14:24:49]

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

[Yuki, 2017-01-31 14:59:15]

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

[commit-bot: I haz the power, 2017-01-31 14:59: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-01-31 15:16:21]

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

[commit-bot: I haz the power, 2017-01-31 15:16:22]

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-01-31 15:22:39]

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

[commit-bot: I haz the power, 2017-01-31 15:23:00]

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

[commit-bot: I haz the power, 2017-01-31 16:05:40]

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

[commit-bot: I haz the power, 2017-01-31 16:05:41]

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

[Yuki, 2017-02-01 05:57:12]

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

[commit-bot: I haz the power, 2017-02-01 05:57:34]

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

[Yuki, 2017-02-01 07:11:54]

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

[commit-bot: I haz the power, 2017-02-01 07:12:10]

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

[Yuki, 2017-02-01 07:20:05]

Description was changed from

==========
binding: Changes the association among global-proxy/global/window-instance.

BUG=675872
==========

to

==========
binding: Changes the association among global-proxy/global/window-instance.

Since V8 now supports internal fields for the global proxy objects,
let's change the mappings among global proxy object / global object /
C++ instance in Blink.

With this CL, a global proxy object and C++ instance point to each
other, and a global object points to the C++ instance (without
reverse pointer).

Note that this CL makes one big difference.  For non-window objects,
Blink objects always live longer than V8 wrappers.  This CL
introduces an exception; DOMWindow may be collected before the V8
wrapper object gets collected because the V8 wrapper is the global
proxy object which remains the same while navigations.

BUG=675872
==========

[Yuki, 2017-02-01 08:27:40]

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

[Yuki, 2017-02-01 08:27:42]

Could you review this CL?

[haraken, 2017-02-01 08:40:44]

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (right):

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:138: // wrapper
because the wrapper object associated with a DOMWindow is a global

I don't fully understand why it can happen.

A global object (i.e., a wrapper) is also still holding a C++ pointer to the
DOMWindow, right? Then why is it possible that the DOMWindow gets collected
before the global object gets collected?

I'm assuming that DOMWrapperTracer traces the global object and treats the
pointed-to DOMWindow object as a GC root (of a Blink GC).

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

[Yuki, 2017-02-01 08:47:59]

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (right):

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:138: // wrapper
because the wrapper object associated with a DOMWindow is a global
On 2017/02/01 08:40:43, haraken wrote:
> 
> I don't fully understand why it can happen.
> 
> A global object (i.e., a wrapper) is also still holding a C++ pointer to the
> DOMWindow, right? Then why is it possible that the DOMWindow gets collected
> before the global object gets collected?
> 
> I'm assuming that DOMWrapperTracer traces the global object and treats the
> pointed-to DOMWindow object as a GC root (of a Blink GC).
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

No, it's not "global object".  It's "global proxy object".

"global object" is collected by V8 GC, then the DOMWindow is collected by
Oilpan, but "global proxy object" is still alive.

Since DOMWindow inherits from ScriptWrappable, DOMWindow has a weak persistent
handle that points to "global proxy object" (with this CL applied).

The bad scenario here is:
1) DOMWindow is freed, and may be re-used.
2) global proxy object is gone, then reset its weak persistent handle
  => write zero into memory area that was previously used by DOMWindow.

[haraken, 2017-02-01 08:53:42]

On 2017/02/01 08:47:59, Yuki wrote:
>
https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (right):
> 
>
https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:138: // wrapper
> because the wrapper object associated with a DOMWindow is a global
> On 2017/02/01 08:40:43, haraken wrote:
> > 
> > I don't fully understand why it can happen.
> > 
> > A global object (i.e., a wrapper) is also still holding a C++ pointer to the
> > DOMWindow, right? Then why is it possible that the DOMWindow gets collected
> > before the global object gets collected?
> > 
> > I'm assuming that DOMWrapperTracer traces the global object and treats the
> > pointed-to DOMWindow object as a GC root (of a Blink GC).
> > 
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
> 
> No, it's not "global object".  It's "global proxy object".
> 
> "global object" is collected by V8 GC, then the DOMWindow is collected by
> Oilpan, but "global proxy object" is still alive.
> 
> Since DOMWindow inherits from ScriptWrappable, DOMWindow has a weak persistent
> handle that points to "global proxy object" (with this CL applied).
> 
> The bad scenario here is:
> 1) DOMWindow is freed, and may be re-used.
> 2) global proxy object is gone, then reset its weak persistent handle
>   => write zero into memory area that was previously used by DOMWindow.

Alternatively, would it be possible to trace a global proxy object and keep the
DOMWindow alive as long as the global proxy object has a reference to the
DOMWindow?

Your implementation looks correct but I think it's safer to just keep the
DOMWindow alive as long as any counterpart in the V8 side is alive.

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

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

[commit-bot: I haz the power, 2017-02-01 09:14:15]

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-02-01 12:36:16]

On 2017/02/01 08:53:42, haraken wrote:
> On 2017/02/01 08:47:59, Yuki wrote:
> >
>
https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (right):
> > 
> >
>
https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:138: // wrapper
> > because the wrapper object associated with a DOMWindow is a global
> > On 2017/02/01 08:40:43, haraken wrote:
> > > 
> > > I don't fully understand why it can happen.
> > > 
> > > A global object (i.e., a wrapper) is also still holding a C++ pointer to
the
> > > DOMWindow, right? Then why is it possible that the DOMWindow gets
collected
> > > before the global object gets collected?
> > > 
> > > I'm assuming that DOMWrapperTracer traces the global object and treats the
> > > pointed-to DOMWindow object as a GC root (of a Blink GC).
> > > 
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
> > 
> > No, it's not "global object".  It's "global proxy object".
> > 
> > "global object" is collected by V8 GC, then the DOMWindow is collected by
> > Oilpan, but "global proxy object" is still alive.
> > 
> > Since DOMWindow inherits from ScriptWrappable, DOMWindow has a weak
persistent
> > handle that points to "global proxy object" (with this CL applied).
> > 
> > The bad scenario here is:
> > 1) DOMWindow is freed, and may be re-used.
> > 2) global proxy object is gone, then reset its weak persistent handle
> >   => write zero into memory area that was previously used by DOMWindow.
> 
> Alternatively, would it be possible to trace a global proxy object and keep
the
> DOMWindow alive as long as the global proxy object has a reference to the
> DOMWindow?

We've already been doing so.  We're making a DOMWindow alive when the global
proxy object still has the internal field to the DOMWindow.

However, when a navigation happens, the global object is detached from the
global proxy object, and the global proxy object's internal field to the
DOMWindow gets cleared.  After that, we attach a new global object and new
DOMWindow to the same global proxy object.  While/after navigation, the global
proxy object loses the pointer to the old DOMWindow.

> Your implementation looks correct but I think it's safer to just keep the
> DOMWindow alive as long as any counterpart in the V8 side is alive.

Yes, and this CL is doing so.

a) at the moment that a navigation happens
The global proxy object detaches the old global object and old DOMWindow, but
the old global object and old DOMWindow are still alive.  Author script may have
references to the old v8::Context.  We still need ToV8(old_DOMWindow) to return
the global proxy object (otherwise, we leak the old window after navigation). 
So we cannot clear the weak persistent handle in the old DOMWindow.  This is
quite asymmetric that the global proxy object forgets the old DOMWindow, but the
old DOMWindow still needs to remember the global proxy object.  This is the root
of the problem, but the spec requires this asymmetry.

b) at the moment that the old global object dies (because no author script has a
reference)
V8 GC collects the old global object, Oilpan collects the old DOMWindow, but the
global proxy object is still alive.  At this point, we need to reset the weak
persistent handle in the old DOMWindow, otherwise the global proxy object resets
it later when it dies.  This timing is only known by the old global object and
old DOMWindow.  The global proxy object doesn't know when the old global object
dies.

[haraken, 2017-02-02 09:09:02]

(Maybe it's easier to chat offline -- not because I think your analysis is wrong
but because the situation is complicated.)

> > Alternatively, would it be possible to trace a global proxy object and keep
> the
> > DOMWindow alive as long as the global proxy object has a reference to the
> > DOMWindow?
> 
> We've already been doing so.  We're making a DOMWindow alive when the global
> proxy object still has the internal field to the DOMWindow.
> 
> However, when a navigation happens, the global object is detached from the
> global proxy object, and the global proxy object's internal field to the
> DOMWindow gets cleared.  After that, we attach a new global object and new
> DOMWindow to the same global proxy object.  While/after navigation, the global
> proxy object loses the pointer to the old DOMWindow.
> 
> > Your implementation looks correct but I think it's safer to just keep the
> > DOMWindow alive as long as any counterpart in the V8 side is alive.
> 
> Yes, and this CL is doing so.
> 
> a) at the moment that a navigation happens
> The global proxy object detaches the old global object and old DOMWindow, but
> the old global object and old DOMWindow are still alive.  Author script may
have
> references to the old v8::Context.  We still need ToV8(old_DOMWindow) to
return
> the global proxy object (otherwise, we leak the old window after navigation). 
> So we cannot clear the weak persistent handle in the old DOMWindow.  This is
> quite asymmetric that the global proxy object forgets the old DOMWindow, but
the
> old DOMWindow still needs to remember the global proxy object.  This is the
root
> of the problem, but the spec requires this asymmetry.

In the first place, how is it possible that a user script runs
ToV8(old_DOMWindow) while the global proxy object is not yet attached to any
global object? If that really happens, we will end up with returning the global
proxy object that is not yet attached to any global object. Won't it just crash?

> 
> b) at the moment that the old global object dies (because no author script has
a
> reference)
> V8 GC collects the old global object, Oilpan collects the old DOMWindow, but
the
> global proxy object is still alive.  At this point, we need to reset the weak
> persistent handle in the old DOMWindow, otherwise the global proxy object
resets
> it later when it dies.  This timing is only known by the old global object and
> old DOMWindow.  The global proxy object doesn't know when the old global
object
> dies.

If we're keeping alive the DOMWindow while the global proxy object is holding a
C++ pointer in its internal field, this should not happen, right?

[Yuki, 2017-02-02 09:19:42]

On 2017/02/02 09:09:02, haraken wrote:
> (Maybe it's easier to chat offline -- not because I think your analysis is
wrong
> but because the situation is complicated.)

Agreed.  Let's chat offline when you're back.


> In the first place, how is it possible that a user script runs
> ToV8(old_DOMWindow) while the global proxy object is not yet attached to any
> global object? If that really happens, we will end up with returning the
global
> proxy object that is not yet attached to any global object. Won't it just
crash?

It must not happen while the global proxy object is not yet attached to a new
global object.  But it happens after the global proxy object is attached to a
new global object (after navigation).  Then, ToV8(old_DOMWindow) needs to return
the global proxy object.


> > b) at the moment that the old global object dies (because no author script
has
> a
> > reference)
> > V8 GC collects the old global object, Oilpan collects the old DOMWindow, but
> the
> > global proxy object is still alive.  At this point, we need to reset the
weak
> > persistent handle in the old DOMWindow, otherwise the global proxy object
> resets
> > it later when it dies.  This timing is only known by the old global object
and
> > old DOMWindow.  The global proxy object doesn't know when the old global
> object
> > dies.
> 
> If we're keeping alive the DOMWindow while the global proxy object is holding
a
> C++ pointer in its internal field, this should not happen, right?

When navigation happens, the global proxy object is attached to a new global
object and new DOMWindow, then the internal field to DOMWindow* is overwritten
to point to the new DOMWindow.  The global proxy object no longer points to the
old DOMWindow, it points to the new DOMWindow.

[haraken, 2017-02-09 12:46:40]

LGTM with comments.

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMDataStore.h (right):

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMDataStore.h:186: // Dissociates a
wrapper, if any, from |scriptWrappable|.

Dissociate => Disconnect ?

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperMap.h (right):

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperMap.h:83: if
(containsKey(key))

Nit: Do you need this check?

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:322: void
DOMWrapperWorld::dissociateWrappersInAllWorlds(

dissociateWrappersInAllWorlds => disconnectDOMWindowWrapper

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:475: // which
points to nothing while a navigation.

Move this comment to toScriptWrappable.

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:5212: return m_frame ?
m_domWindow : nullptr;

Would you help me understand why we need to check m_frame?

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

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/DOMWindow.cpp:56: CHECK(frame);

Would you help me understand why frame is guaranteed to be non-null? In other
words, how is it guaranteed that the window is not yet detached?

[Yuki, 2017-02-10 07:22:34]

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

[commit-bot: I haz the power, 2017-02-10 07:22:53]

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-10 07:37:04]

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

[commit-bot: I haz the power, 2017-02-10 07:37:05]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Yuki, 2017-02-10 07:46:23]

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

[commit-bot: I haz the power, 2017-02-10 07:46:46]

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

[Yuki, 2017-02-10 07:47:21]

Could you take another look?

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMDataStore.h (right):

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMDataStore.h:186: // Dissociates a
wrapper, if any, from |scriptWrappable|.
On 2017/02/09 12:46:40, haraken wrote:
> 
> Dissociate => Disconnect ?

A dictionary says:
  connect <=> disconnect
  associate <=> dissociate

We say "associate a wrapper with a scriptwrappable", etc., in other places, so I
thought "dissociate a wrapper from a scriptwrappable" makes sense.  WDYT?

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperMap.h (right):

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperMap.h:83: if
(containsKey(key))
On 2017/02/09 12:46:40, haraken wrote:
> 
> Nit: Do you need this check?

Seems not necessary.  Removed.

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:475: // which
points to nothing while a navigation.
On 2017/02/09 12:46:40, haraken wrote:
> 
> Move this comment to toScriptWrappable.

Done.

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:5212: return m_frame ?
m_domWindow : nullptr;
On 2017/02/09 12:46:40, haraken wrote:
> 
> Would you help me understand why we need to check m_frame?

https://html.spec.whatwg.org/multipage/browsers.html#dom-document-defaultview
The defaultView IDL attribute of the Document interface, on getting, must return
this Document's browsing context's WindowProxy object, if this Document has an
associated browsing context, or null otherwise.

The last phrase "or null otherwise" is important here.  We need to return
nullptr if no |m_frame|.  FYI, we have a layout test, that fails if we ignore
m_frame.

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

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/DOMWindow.cpp:56: CHECK(frame);
On 2017/02/09 12:46:40, haraken wrote:
> 
> Would you help me understand why frame is guaranteed to be non-null? In other
> words, how is it guaranteed that the window is not yet detached?

If WindowProxy::initialize() was already called, this DOMWindow object must be
already associated with a global proxy object as a wrapper object.  So, you
don't hit this line.

If you hit this line, the WindowProxy is not yet initialized due to the lazy
initialization.  I don't think there is a way to detach a window *BEFORE* its
initialization.  So the window must be attached.

[haraken, 2017-02-10 07:59:40]

LGTM

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:5212: return m_frame ?
m_domWindow : nullptr;
On 2017/02/10 07:47:21, Yuki wrote:
> On 2017/02/09 12:46:40, haraken wrote:
> > 
> > Would you help me understand why we need to check m_frame?
> 
> https://html.spec.whatwg.org/multipage/browsers.html#dom-document-defaultview
> The defaultView IDL attribute of the Document interface, on getting, must
return
> this Document's browsing context's WindowProxy object, if this Document has an
> associated browsing context, or null otherwise.
> 
> The last phrase "or null otherwise" is important here.  We need to return
> nullptr if no |m_frame|.  FYI, we have a layout test, that fails if we ignore
> m_frame.

I'm wondering why this cannot be 'return m_domWindow'.

It looks like that there's a subtle difference between when m_domWindow is
cleared and when m_frame is cleared, but does it matter here?

https://codereview.chromium.org/2617733004/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (right):

https://codereview.chromium.org/2617733004/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:148: static void
dissociateWrappersInAllWorlds(ScriptWrappable*);

I'd prefer renaming this to dissociateDOMWindowWrappers to clarify that this
method is only for DOMWindow.

[Yuki, 2017-02-10 08:18:19]

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:5212: return m_frame ?
m_domWindow : nullptr;
On 2017/02/10 07:59:40, haraken wrote:
> On 2017/02/10 07:47:21, Yuki wrote:
> > On 2017/02/09 12:46:40, haraken wrote:
> > > 
> > > Would you help me understand why we need to check m_frame?
> > 
> >
https://html.spec.whatwg.org/multipage/browsers.html#dom-document-defaultview
> > The defaultView IDL attribute of the Document interface, on getting, must
> return
> > this Document's browsing context's WindowProxy object, if this Document has
an
> > associated browsing context, or null otherwise.
> > 
> > The last phrase "or null otherwise" is important here.  We need to return
> > nullptr if no |m_frame|.  FYI, we have a layout test, that fails if we
ignore
> > m_frame.
> 
> I'm wondering why this cannot be 'return m_domWindow'.
> 
> It looks like that there's a subtle difference between when m_domWindow is
> cleared and when m_frame is cleared, but does it matter here?

I read code a little, and found that
- m_domWindow is cleared by Document::clearDOMWindow
- clearDOMWindow is called only through
  LocalDOMWindow::installNewDocument
  => LocalDOMWindow::clearDocument
     => Document::clearDOMWindow
This doesn't help much to determine if the document is detached from a frame or
not.  Unless the new document is installed, m_domWindow never gets cleared.

https://codereview.chromium.org/2617733004/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (right):

https://codereview.chromium.org/2617733004/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:148: static void
dissociateWrappersInAllWorlds(ScriptWrappable*);
On 2017/02/10 07:59:40, haraken wrote:
> 
> I'd prefer renaming this to dissociateDOMWindowWrappers to clarify that this
> method is only for DOMWindow.

Good point.  I'll do rename it to dissociateDOMWindowWrappersInAllWorlds.  This
is a little long, but we don't often use this.  So it should be okay.

I'll also change the argument type to DOMWindow*.

[haraken, 2017-02-10 09:15:31]

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:5212: return m_frame ?
m_domWindow : nullptr;
On 2017/02/10 08:18:19, Yuki wrote:
> On 2017/02/10 07:59:40, haraken wrote:
> > On 2017/02/10 07:47:21, Yuki wrote:
> > > On 2017/02/09 12:46:40, haraken wrote:
> > > > 
> > > > Would you help me understand why we need to check m_frame?
> > > 
> > >
> https://html.spec.whatwg.org/multipage/browsers.html#dom-document-defaultview
> > > The defaultView IDL attribute of the Document interface, on getting, must
> > return
> > > this Document's browsing context's WindowProxy object, if this Document
has
> an
> > > associated browsing context, or null otherwise.
> > > 
> > > The last phrase "or null otherwise" is important here.  We need to return
> > > nullptr if no |m_frame|.  FYI, we have a layout test, that fails if we
> ignore
> > > m_frame.
> > 
> > I'm wondering why this cannot be 'return m_domWindow'.
> > 
> > It looks like that there's a subtle difference between when m_domWindow is
> > cleared and when m_frame is cleared, but does it matter here?
> 
> I read code a little, and found that
> - m_domWindow is cleared by Document::clearDOMWindow
> - clearDOMWindow is called only through
>   LocalDOMWindow::installNewDocument
>   => LocalDOMWindow::clearDocument
>      => Document::clearDOMWindow
> This doesn't help much to determine if the document is detached from a frame
or
> not.  Unless the new document is installed, m_domWindow never gets cleared.

Makes sense. Can you add a comment about it?

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

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

[commit-bot: I haz the power, 2017-02-10 09:45:11]

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

[Yuki, 2017-02-10 11:21:00]

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

[commit-bot: I haz the power, 2017-02-10 11:21:18]

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

[Yuki, 2017-02-10 11:22:47]

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2617733004/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:5212: return m_frame ?
m_domWindow : nullptr;
On 2017/02/10 09:15:31, haraken wrote:
> On 2017/02/10 08:18:19, Yuki wrote:
> > On 2017/02/10 07:59:40, haraken wrote:
> > > On 2017/02/10 07:47:21, Yuki wrote:
> > > > On 2017/02/09 12:46:40, haraken wrote:
> > > > > 
> > > > > Would you help me understand why we need to check m_frame?
> > > > 
> > > >
> >
https://html.spec.whatwg.org/multipage/browsers.html#dom-document-defaultview
> > > > The defaultView IDL attribute of the Document interface, on getting,
must
> > > return
> > > > this Document's browsing context's WindowProxy object, if this Document
> has
> > an
> > > > associated browsing context, or null otherwise.
> > > > 
> > > > The last phrase "or null otherwise" is important here.  We need to
return
> > > > nullptr if no |m_frame|.  FYI, we have a layout test, that fails if we
> > ignore
> > > > m_frame.
> > > 
> > > I'm wondering why this cannot be 'return m_domWindow'.
> > > 
> > > It looks like that there's a subtle difference between when m_domWindow is
> > > cleared and when m_frame is cleared, but does it matter here?
> > 
> > I read code a little, and found that
> > - m_domWindow is cleared by Document::clearDOMWindow
> > - clearDOMWindow is called only through
> >   LocalDOMWindow::installNewDocument
> >   => LocalDOMWindow::clearDocument
> >      => Document::clearDOMWindow
> > This doesn't help much to determine if the document is detached from a frame
> or
> > not.  Unless the new document is installed, m_domWindow never gets cleared.
> 
> Makes sense. Can you add a comment about it?

Done.

https://codereview.chromium.org/2617733004/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (right):

https://codereview.chromium.org/2617733004/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:148: static void
dissociateWrappersInAllWorlds(ScriptWrappable*);
On 2017/02/10 08:18:19, Yuki wrote:
> On 2017/02/10 07:59:40, haraken wrote:
> > 
> > I'd prefer renaming this to dissociateDOMWindowWrappers to clarify that this
> > method is only for DOMWindow.
> 
> Good point.  I'll do rename it to dissociateDOMWindowWrappersInAllWorlds. 
This
> is a little long, but we don't often use this.  So it should be okay.
> 
> I'll also change the argument type to DOMWindow*.

I'm sorry that I changed my mind.  I'm going to continue using ScriptWrappable*
because I don't want to introduce more dependencies from bindings/ to core/. 
I'd like to make DOMWrapperWorld independent from core/ as much as possible.

I did the rename.  No worry for this point.

[Yuki, 2017-02-10 13:07:31]

The CQ bit was unchecked by yukishiino@chromium.org

[Yuki, 2017-02-10 13:07:39]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2017-02-10 13:07:40]

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/2617733004/#ps270001
(title: "Addressed review comments.")

[commit-bot: I haz the power, 2017-02-10 13:08:03]

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-10 14:36:24]

CQ is committing da patch.

Bot data: {"patchset_id": 270001, "attempt_start_ts": 1486732059691970,
"parent_rev": "405e89e5b599025c4b0f7ed8b7b92da36ab1b69f", "commit_rev":
"76ec9e6ea20fb4a3c1f242e39abea0067afd235a"}

[commit-bot: I haz the power, 2017-02-10 14:36:57]

Description was changed from

==========
binding: Changes the association among global-proxy/global/window-instance.

Since V8 now supports internal fields for the global proxy objects,
let's change the mappings among global proxy object / global object /
C++ instance in Blink.

With this CL, a global proxy object and C++ instance point to each
other, and a global object points to the C++ instance (without
reverse pointer).

Note that this CL makes one big difference.  For non-window objects,
Blink objects always live longer than V8 wrappers.  This CL
introduces an exception; DOMWindow may be collected before the V8
wrapper object gets collected because the V8 wrapper is the global
proxy object which remains the same while navigations.

BUG=675872
==========

to

==========
binding: Changes the association among global-proxy/global/window-instance.

Since V8 now supports internal fields for the global proxy objects,
let's change the mappings among global proxy object / global object /
C++ instance in Blink.

With this CL, a global proxy object and C++ instance point to each
other, and a global object points to the C++ instance (without
reverse pointer).

Note that this CL makes one big difference.  For non-window objects,
Blink objects always live longer than V8 wrappers.  This CL
introduces an exception; DOMWindow may be collected before the V8
wrapper object gets collected because the V8 wrapper is the global
proxy object which remains the same while navigations.

BUG=675872

Review-Url: https://codereview.chromium.org/2617733004
Cr-Commit-Position: refs/heads/master@{#449616}
Committed:
https://chromium.googlesource.com/chromium/src/+/76ec9e6ea20fb4a3c1f242e39abe...
==========

[commit-bot: I haz the power, 2017-02-10 14:36:58]

Committed patchset #15 (id:270001) as
https://chromium.googlesource.com/chromium/src/+/76ec9e6ea20fb4a3c1f242e39abe...

[Yuki, 2017-02-13 06:53:09]

A revert of this CL (patchset #15 id:270001) has been created in
https://codereview.chromium.org/2690943002/ by yukishiino@chromium.org.

The reason for reverting is: This CL is suspicious to be causing crashes:
https://crbug.com/691269
.

[Yuki, 2017-02-15 12:53:58]

Patchset #16 (id:290001) has been deleted