binding: Removes Document::wrap that must be equivalent to Node::wrap. (2nd try)

binding: Removes Document::wrap that must be equivalent to Node::wrap. (2nd try)

Removes unnecessary Document::wrap and associateWithWrapper.
The special code in these functions were made in order to keep
WindowProxy::m_document updated and consistent with toV8(document).
But we no longer rely on the hack, and we can remove the dead code.

WindowProxy::m_document is used to support named properties on the
document, and only used in WindowProxy::namedItem{Added,Removed}.
These functions are only called from ScriptController::namedItem
{Added,Removed} and ScriptController already guarantees that the
WindowProxy is initialized at once.  So, there is no need for
Document::wrap to call WindowProxy::updateDocumentProperty.

Plus, this CL removes WindowProxy::m_document.  Given that we knew
it's the main world, we should be able to retrieve the wrapper
object of the document in almost the same speed as before.  The
new code to retrieve the wrapper object should be almost equivalent
to
    ScriptWrappable::mainWorldWrapper(isolate)
and it's almost the same as
    m_document.newLocal(isolate)

The first attempt was: http://crrev.com/2525313004

BUG=

Committed: https://crrev.com/23d2ae42f228825c3edb45ca50c4cb5e2e1045c0
Cr-Commit-Position: refs/heads/master@{#436243}

[Yuki, 2016-12-01 12:42:03]

Description was changed from

==========
binding: Removes Document::wrap that must be equivalent to Node::wrap. (2nd try)

Removes unnecessary Document::wrap and associateWithWrapper.
The special code in these functions were made in order to keep
WindowProxy::m_document updated and consistent with toV8(document).
But we no longer rely on the hack, and we can remove the dead code.

The first attempt was: http://crrev.com/2525313004

patch from issue 2525313004 at patchset 100001
(http://crrev.com/2525313004#ps100001)

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

to

==========
binding: Removes Document::wrap that must be equivalent to Node::wrap. (2nd try)

Removes unnecessary Document::wrap and associateWithWrapper.
The special code in these functions were made in order to keep
WindowProxy::m_document updated and consistent with toV8(document).
But we no longer rely on the hack, and we can remove the dead code.

The first attempt was: http://crrev.com/2525313004

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

[Yuki, 2016-12-02 08:59:11]

Description was changed from

==========
binding: Removes Document::wrap that must be equivalent to Node::wrap. (2nd try)

Removes unnecessary Document::wrap and associateWithWrapper.
The special code in these functions were made in order to keep
WindowProxy::m_document updated and consistent with toV8(document).
But we no longer rely on the hack, and we can remove the dead code.

The first attempt was: http://crrev.com/2525313004

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

to

==========
binding: Removes Document::wrap that must be equivalent to Node::wrap. (2nd try)

Removes unnecessary Document::wrap and associateWithWrapper.
The special code in these functions were made in order to keep
WindowProxy::m_document updated and consistent with toV8(document).
But we no longer rely on the hack, and we can remove the dead code.

WindowProxy::m_document is used to support named properties on the
document, and only used in WindowProxy::namedItem{Added,Removed}.
These functions are only called from ScriptController::namedItem
{Added,Removed} and ScriptController already guarantees that the
WindowProxy is initialized at once.  So, there is no need for
Document::wrap to call WindowProxy::updateDocumentProperty.

Plus, this CL removes WindowProxy::m_document.  Given that we knew
it's the main world, we should be able to retrieve the wrapper
object of the document in almost the same speed as before.  The
new code to retrieve the wrapper object should be almost equivalent
to
    ScriptWrappable::mainWorldWrapper(isolate)
and it's almost the same as
    m_document.newLocal(isolate)

The first attempt was: http://crrev.com/2525313004

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

[Yuki, 2016-12-02 08:59:16]

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

[commit-bot: I haz the power, 2016-12-02 08:59:38]

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, 2016-12-02 09:06:32]

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

[commit-bot: I haz the power, 2016-12-02 09:06:33]

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

[Yuki, 2016-12-02 09:08:05]

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

[Yuki, 2016-12-02 09:08:06]

haraken@, what do you think of this approach to remove WindowProxy::m_document?

As I commented, the CHECK was wrong.  Simply removing the CHECK should work well
I believe.  However, in order to get rid of the concern about consistency
between toV8(document) and WindowProxy::m_document, I've removed
WindowProxy::m_document.

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

https://codereview.chromium.org/2542123002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:436:
CHECK(!frame->document()->containsWrapper());
This CHECK and comment are wrong.  We can create a new document and instantiate
its wrapper before initializing a WindowProxy.  For example,
    <image id="foo"/>
    <script>
    iframe = document.createElement('iframe');
    document.body.appendChild(iframe);
    d = iframe.contentDocument;  // document wrapper gets instantiated
    d.appendChild(window.foo);  // initialize the iframe's WindowProxy in order
to add an named object
    </script>

[Yuki, 2016-12-02 09:09:38]

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

[commit-bot: I haz the power, 2016-12-02 09:09:54]

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

[haraken, 2016-12-02 09:53:06]

LGTM

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

https://codereview.chromium.org/2542123002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:431:
toV8(frame->document(), context->Global(), m_isolate);

Can you add CHECK(m_world->domDataStore().get(document, m_isolate).IsEmpty()) ?

https://codereview.chromium.org/2542123002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:560: DCHECK(wrapper
==

Can you add DCHECK(!m_world->domDataStore().get(document, m_isolate).IsEmpty())
? Otherwise, this DCHECK will have a side effect of creating a wrapper.

[Yuki, 2016-12-02 09:55:56]

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

https://codereview.chromium.org/2542123002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:431:
toV8(frame->document(), context->Global(), m_isolate);
On 2016/12/02 09:53:05, haraken wrote:
> 
> Can you add CHECK(m_world->domDataStore().get(document, m_isolate).IsEmpty())
?

THAT CHECK CAUSED THE CRASH.

https://codereview.chromium.org/2542123002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:560: DCHECK(wrapper
==
On 2016/12/02 09:53:05, haraken wrote:
> 
> Can you add DCHECK(!m_world->domDataStore().get(document,
m_isolate).IsEmpty())
> ? Otherwise, this DCHECK will have a side effect of creating a wrapper.

Oops, I forgot to remove checkDocumentWrapper() entirely.
Since this CL removes m_document, we no longer need this function.

[Yuki, 2016-12-02 09:58:26]

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

[commit-bot: I haz the power, 2016-12-02 09:58:46]

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

[haraken, 2016-12-02 10:17:48]

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

https://codereview.chromium.org/2542123002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:431:
toV8(frame->document(), context->Global(), m_isolate);
On 2016/12/02 09:55:56, Yuki wrote:
> On 2016/12/02 09:53:05, haraken wrote:
> > 
> > Can you add CHECK(m_world->domDataStore().get(document,
m_isolate).IsEmpty())
> ?
> 
> THAT CHECK CAUSED THE CRASH.

I'm behind. Would you help me understand:

- How is it possible that a document wrapper is created before here?

- If that really happens, we will have some period where window.document (the
cached accessor) is not pointing to a valid document wrapper (because it is set
at line 434). Why is it okay?

[Yuki, 2016-12-02 10:39:50]

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

https://codereview.chromium.org/2542123002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:431:
toV8(frame->document(), context->Global(), m_isolate);
On 2016/12/02 10:17:48, haraken wrote:
> On 2016/12/02 09:55:56, Yuki wrote:
> > On 2016/12/02 09:53:05, haraken wrote:
> > > 
> > > Can you add CHECK(m_world->domDataStore().get(document,
> m_isolate).IsEmpty())
> > ?
> > 
> > THAT CHECK CAUSED THE CRASH.
> 
> I'm behind. Would you help me understand:
> 
> - How is it possible that a document wrapper is created before here?
> 
> - If that really happens, we will have some period where window.document (the
> cached accessor) is not pointing to a valid document wrapper (because it is
set
> at line 434). Why is it okay?

I wrote the explanation at the first message of this CL.  I copy the example
part here:
    <image id="foo"/>
    <script>
    iframe = document.createElement('iframe');
    document.body.appendChild(iframe);
    d = iframe.contentDocument;  // document wrapper gets instantiated
    d.appendChild(window.foo);  // initialize the iframe's WindowProxy in order
to add an named object
    </script>


> - How is it possible that a document wrapper is created before here?

Suppose that there are two windows, parent and child, and each of them has its
own document.  In the above example, child's document's wrapper is created for
the parent context, without initializing child's context, without using child's
context.

> - If that really happens, we will have some period where window.document (the
> cached accessor) is not pointing to a valid document wrapper (because it is
set
> at line 434). Why is it okay?

Since child's context is not initialized yet, there is no |window| object for
child yet, hence no |window.document| exists yet.  In this meaning, there is no
such period.

[haraken, 2016-12-02 10:54:25]

On 2016/12/02 10:39:50, Yuki wrote:
>
https://codereview.chromium.org/2542123002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):
> 
>
https://codereview.chromium.org/2542123002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:431:
> toV8(frame->document(), context->Global(), m_isolate);
> On 2016/12/02 10:17:48, haraken wrote:
> > On 2016/12/02 09:55:56, Yuki wrote:
> > > On 2016/12/02 09:53:05, haraken wrote:
> > > > 
> > > > Can you add CHECK(m_world->domDataStore().get(document,
> > m_isolate).IsEmpty())
> > > ?
> > > 
> > > THAT CHECK CAUSED THE CRASH.
> > 
> > I'm behind. Would you help me understand:
> > 
> > - How is it possible that a document wrapper is created before here?
> > 
> > - If that really happens, we will have some period where window.document
(the
> > cached accessor) is not pointing to a valid document wrapper (because it is
> set
> > at line 434). Why is it okay?
> 
> I wrote the explanation at the first message of this CL.  I copy the example
> part here:
>     <image id="foo"/>
>     <script>
>     iframe = document.createElement('iframe');
>     document.body.appendChild(iframe);
>     d = iframe.contentDocument;  // document wrapper gets instantiated
>     d.appendChild(window.foo);  // initialize the iframe's WindowProxy in
order
> to add an named object
>     </script>
> 
> 
> > - How is it possible that a document wrapper is created before here?
> 
> Suppose that there are two windows, parent and child, and each of them has its
> own document.  In the above example, child's document's wrapper is created for
> the parent context, without initializing child's context, without using
child's
> context.
> 
> > - If that really happens, we will have some period where window.document
(the
> > cached accessor) is not pointing to a valid document wrapper (because it is
> set
> > at line 434). Why is it okay?
> 
> Since child's context is not initialized yet, there is no |window| object for
> child yet, hence no |window.document| exists yet.  In this meaning, there is
no
> such period.

Thanks for the clarification! LGTM.

[commit-bot: I haz the power, 2016-12-02 11:08:28]

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

[commit-bot: I haz the power, 2016-12-02 11:08:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yuki, 2016-12-02 12:49:53]

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

[commit-bot: I haz the power, 2016-12-02 12:50:07]

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, 2016-12-02 15:48:35]

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

[commit-bot: I haz the power, 2016-12-02 15:48:36]

Dry run: This issue passed the CQ dry run.

[Yuki, 2016-12-05 06:19:03]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2016-12-05 06:19:03]

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/2542123002/#ps80001
(title: "Fixed test failures.")

[commit-bot: I haz the power, 2016-12-05 06:19:13]

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, 2016-12-05 08:03:38]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480918743494230,
"parent_rev": "4ecdb3d1b9ee20060f7f8936b39cab014df966ed", "commit_rev":
"42ff900b77adc997d53b05a0401af131bcd2c746"}

[commit-bot: I haz the power, 2016-12-05 08:04:05]

Description was changed from

==========
binding: Removes Document::wrap that must be equivalent to Node::wrap. (2nd try)

Removes unnecessary Document::wrap and associateWithWrapper.
The special code in these functions were made in order to keep
WindowProxy::m_document updated and consistent with toV8(document).
But we no longer rely on the hack, and we can remove the dead code.

WindowProxy::m_document is used to support named properties on the
document, and only used in WindowProxy::namedItem{Added,Removed}.
These functions are only called from ScriptController::namedItem
{Added,Removed} and ScriptController already guarantees that the
WindowProxy is initialized at once.  So, there is no need for
Document::wrap to call WindowProxy::updateDocumentProperty.

Plus, this CL removes WindowProxy::m_document.  Given that we knew
it's the main world, we should be able to retrieve the wrapper
object of the document in almost the same speed as before.  The
new code to retrieve the wrapper object should be almost equivalent
to
    ScriptWrappable::mainWorldWrapper(isolate)
and it's almost the same as
    m_document.newLocal(isolate)

The first attempt was: http://crrev.com/2525313004

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

to

==========
binding: Removes Document::wrap that must be equivalent to Node::wrap. (2nd try)

Removes unnecessary Document::wrap and associateWithWrapper.
The special code in these functions were made in order to keep
WindowProxy::m_document updated and consistent with toV8(document).
But we no longer rely on the hack, and we can remove the dead code.

WindowProxy::m_document is used to support named properties on the
document, and only used in WindowProxy::namedItem{Added,Removed}.
These functions are only called from ScriptController::namedItem
{Added,Removed} and ScriptController already guarantees that the
WindowProxy is initialized at once.  So, there is no need for
Document::wrap to call WindowProxy::updateDocumentProperty.

Plus, this CL removes WindowProxy::m_document.  Given that we knew
it's the main world, we should be able to retrieve the wrapper
object of the document in almost the same speed as before.  The
new code to retrieve the wrapper object should be almost equivalent
to
    ScriptWrappable::mainWorldWrapper(isolate)
and it's almost the same as
    m_document.newLocal(isolate)

The first attempt was: http://crrev.com/2525313004

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

[commit-bot: I haz the power, 2016-12-05 08:04:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-05 08:06:29]

Description was changed from

==========
binding: Removes Document::wrap that must be equivalent to Node::wrap. (2nd try)

Removes unnecessary Document::wrap and associateWithWrapper.
The special code in these functions were made in order to keep
WindowProxy::m_document updated and consistent with toV8(document).
But we no longer rely on the hack, and we can remove the dead code.

WindowProxy::m_document is used to support named properties on the
document, and only used in WindowProxy::namedItem{Added,Removed}.
These functions are only called from ScriptController::namedItem
{Added,Removed} and ScriptController already guarantees that the
WindowProxy is initialized at once.  So, there is no need for
Document::wrap to call WindowProxy::updateDocumentProperty.

Plus, this CL removes WindowProxy::m_document.  Given that we knew
it's the main world, we should be able to retrieve the wrapper
object of the document in almost the same speed as before.  The
new code to retrieve the wrapper object should be almost equivalent
to
    ScriptWrappable::mainWorldWrapper(isolate)
and it's almost the same as
    m_document.newLocal(isolate)

The first attempt was: http://crrev.com/2525313004

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

to

==========
binding: Removes Document::wrap that must be equivalent to Node::wrap. (2nd try)

Removes unnecessary Document::wrap and associateWithWrapper.
The special code in these functions were made in order to keep
WindowProxy::m_document updated and consistent with toV8(document).
But we no longer rely on the hack, and we can remove the dead code.

WindowProxy::m_document is used to support named properties on the
document, and only used in WindowProxy::namedItem{Added,Removed}.
These functions are only called from ScriptController::namedItem
{Added,Removed} and ScriptController already guarantees that the
WindowProxy is initialized at once.  So, there is no need for
Document::wrap to call WindowProxy::updateDocumentProperty.

Plus, this CL removes WindowProxy::m_document.  Given that we knew
it's the main world, we should be able to retrieve the wrapper
object of the document in almost the same speed as before.  The
new code to retrieve the wrapper object should be almost equivalent
to
    ScriptWrappable::mainWorldWrapper(isolate)
and it's almost the same as
    m_document.newLocal(isolate)

The first attempt was: http://crrev.com/2525313004

BUG=

Committed: https://crrev.com/23d2ae42f228825c3edb45ca50c4cb5e2e1045c0
Cr-Commit-Position: refs/heads/master@{#436243}
==========

[commit-bot: I haz the power, 2016-12-05 08:06:30]

Patchset 5 (id:??) landed as
https://crrev.com/23d2ae42f228825c3edb45ca50c4cb5e2e1045c0
Cr-Commit-Position: refs/heads/master@{#436243}