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

third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp

Index: third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp b/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp
index dec4bd5fea06c5ce882790166ed7a9e404996d1c..0f0e933ca84538ca9eee1b1d798f93861e572685 100644
--- a/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp
@@ -33,6 +33,7 @@
 #include "bindings/core/v8/ConditionalFeatures.h"
 #include "bindings/core/v8/DOMWrapperWorld.h"
 #include "bindings/core/v8/ScriptController.h"
+#include "bindings/core/v8/ToV8.h"
 #include "bindings/core/v8/V8Binding.h"
 #include "bindings/core/v8/V8DOMActivityLogger.h"
 #include "bindings/core/v8/V8Document.h"
@@ -73,11 +74,6 @@
 
 namespace blink {
 
-static void checkDocumentWrapper(v8::Local<v8::Object> wrapper,
-                                 Document* document) {
-  ASSERT(V8Document::toImpl(wrapper) == document);
-}
-
 WindowProxy* WindowProxy::create(v8::Isolate* isolate,
                                  Frame* frame,
                                  DOMWrapperWorld& world) {
@@ -115,8 +111,6 @@ void WindowProxy::disposeContext(GlobalDetachmentBehavior behavior) {
     MainThreadDebugger::instance()->contextWillBeDestroyed(m_scriptState.get());
   }
 
-  m_document.clear();
-
   if (behavior == DetachGlobal) {
     // Clean up state on the global proxy, which will be reused.
     if (!m_globalProxy.isEmpty()) {
@@ -424,35 +418,19 @@ bool WindowProxy::setupWindowPrototypeChain() {
   return true;
 }
 
-void WindowProxy::updateDocumentWrapper(v8::Local<v8::Object> wrapper) {
-  ASSERT(m_world->isMainWorld());
-  m_document.set(m_isolate, wrapper);
-}
-
 void WindowProxy::updateDocumentProperty() {
-  if (!m_world->isMainWorld())
-    return;
+  DCHECK(m_world->isMainWorld());
 
-  if (m_frame->isRemoteFrame()) {
+  if (m_frame->isRemoteFrame())
     return;
-  }
 
   ScriptState::Scope scope(m_scriptState.get());
   v8::Local<v8::Context> context = m_scriptState->context();
   LocalFrame* frame = toLocalFrame(m_frame);
   v8::Local<v8::Value> documentWrapper =
-      toV8(frame->document(), context->Global(), context->GetIsolate());
-  if (documentWrapper.IsEmpty())
-    return;
-  ASSERT(documentWrapper == m_document.newLocal(m_isolate) ||
-         m_document.isEmpty());
-  if (m_document.isEmpty())
-    updateDocumentWrapper(v8::Local<v8::Object>::Cast(documentWrapper));
-  checkDocumentWrapper(m_document.newLocal(m_isolate), frame->document());
-
-  ASSERT(documentWrapper->IsObject());
-
-  // Update cached accessor.
+      toV8(frame->document(), context->Global(), m_isolate);

[haraken, 2016/12/02 09:53:05]

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

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

THAT CHECK CAUSED THE CRASH.

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

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:49]

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

+  DCHECK(documentWrapper->IsObject());
+  // Update the cached accessor for window.document.
   CHECK(V8PrivateProperty::getWindowDocumentCachedAccessor(m_isolate).set(
       context, context->Global(), documentWrapper));
 }
@@ -518,7 +496,7 @@ void WindowProxy::setSecurityToken(SecurityOrigin* origin) {
 }
 
 void WindowProxy::updateDocument() {
-  ASSERT(m_world->isMainWorld());
+  DCHECK(m_world->isMainWorld());
   if (!isGlobalInitialized())
     return;
   if (!isContextInitialized())
@@ -575,24 +553,33 @@ static void getter(v8::Local<v8::Name> property,
     v8SetReturnValue(info, value);
 }
 
+void WindowProxy::checkDocumentWrapper(v8::Local<v8::Object> wrapper,
+                                       Document* document) const {
+  DCHECK(!wrapper.IsEmpty());
+  DCHECK_EQ(V8Document::toImpl(wrapper), document);
+  DCHECK(wrapper ==

[haraken, 2016/12/02 09:53:05]

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]

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

+         toV8(document, m_globalProxy.newLocal(m_isolate), m_isolate));
+}
+
 void WindowProxy::namedItemAdded(HTMLDocument* document,
                                  const AtomicString& name) {
-  ASSERT(m_world->isMainWorld());
+  DCHECK(m_world->isMainWorld());
 
-  if (!isContextInitialized() || !m_scriptState->contextIsValid())
+  if (!isContextInitialized())
     return;
 
   ScriptState::Scope scope(m_scriptState.get());
-  ASSERT(!m_document.isEmpty());
-  v8::Local<v8::Context> context = m_scriptState->context();
-  v8::Local<v8::Object> documentHandle = m_document.newLocal(m_isolate);
-  checkDocumentWrapper(documentHandle, document);
-  documentHandle->SetAccessor(context, v8String(m_isolate, name), getter);
+  v8::Local<v8::Object> documentWrapper =
+      m_world->domDataStore().get(document, m_isolate);
+  documentWrapper
+      ->SetAccessor(m_isolate->GetCurrentContext(), v8String(m_isolate, name),
+                    getter)
+      .ToChecked();
 }
 
 void WindowProxy::namedItemRemoved(HTMLDocument* document,
                                    const AtomicString& name) {
-  ASSERT(m_world->isMainWorld());
+  DCHECK(m_world->isMainWorld());
 
   if (!isContextInitialized())
     return;
@@ -601,11 +588,11 @@ void WindowProxy::namedItemRemoved(HTMLDocument* document,
     return;
 
   ScriptState::Scope scope(m_scriptState.get());
-  ASSERT(!m_document.isEmpty());
-  v8::Local<v8::Object> documentHandle = m_document.newLocal(m_isolate);
-  checkDocumentWrapper(documentHandle, document);
-  documentHandle->Delete(m_isolate->GetCurrentContext(),
-                         v8String(m_isolate, name));
+  v8::Local<v8::Object> documentWrapper =
+      m_world->domDataStore().get(document, m_isolate);
+  documentWrapper
+      ->Delete(m_isolate->GetCurrentContext(), v8String(m_isolate, name))
+      .ToChecked();
 }
 
 void WindowProxy::updateSecurityOrigin(SecurityOrigin* origin) {