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

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

 /*
  * Copyright (C) 2008, 2009, 2011 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 15 matching lines @@
  * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "bindings/core/v8/WindowProxy.h"
 
 #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"
 #include "bindings/core/v8/V8GCForContextDispose.h"
 #include "bindings/core/v8/V8HTMLCollection.h"
 #include "bindings/core/v8/V8HTMLDocument.h"
 #include "bindings/core/v8/V8HiddenValue.h"
 #include "bindings/core/v8/V8Initializer.h"
 #include "bindings/core/v8/V8ObjectConstructor.h"
 #include "bindings/core/v8/V8PagePopupControllerBinding.h"
@@ skipping 20 matching lines @@
 #include "wtf/Assertions.h"
 #include "wtf/StringExtras.h"
 #include "wtf/text/CString.h"
 #include <algorithm>
 #include <utility>
 #include <v8-debug.h>
 #include <v8.h>
 
 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) {
   return new WindowProxy(frame, &world, isolate);
 }
 
 WindowProxy::WindowProxy(Frame* frame,
                          PassRefPtr<DOMWrapperWorld> world,
                          v8::Isolate* isolate)
     : m_frame(frame), m_isolate(isolate), m_world(world) {}
@@ skipping 17 matching lines @@
   if (m_frame->isLocalFrame()) {
     LocalFrame* frame = toLocalFrame(m_frame);
     // The embedder could run arbitrary code in response to the
     // willReleaseScriptContext callback, so all disposing should happen after
     // it returns.
     frame->loader().client()->willReleaseScriptContext(context,
                                                        m_world->worldId());
     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()) {
       // TODO(yukishiino): This DCHECK failed on Canary (M57) and Dev (M56).
       // We need to figure out why m_globalProxy != context->Global().
       DCHECK(m_globalProxy == context->Global());
       DCHECK_EQ(toScriptWrappable(context->Global()),
                 toScriptWrappable(
                     context->Global()->GetPrototype().As<v8::Object>()));
       m_globalProxy.get().SetWrapperClassId(0);
@@ skipping 287 matching lines @@
   V8DOMWrapper::setNativeInfo(m_isolate, windowProperties, wrapperTypeInfo,
                               window);
 
   // TODO(keishi): Remove installPagePopupController and implement
   // PagePopupController in another way.
   V8PagePopupControllerBinding::installPagePopupController(context,
                                                            windowWrapper);
   return true;
 }
 
-void WindowProxy::updateDocumentWrapper(v8::Local<v8::Object> wrapper) {
-  ASSERT(m_world->isMainWorld());
-  m_document.set(m_isolate, wrapper);
-}
+void WindowProxy::updateDocumentProperty() {
+  DCHECK(m_world->isMainWorld());
 
-void WindowProxy::updateDocumentProperty() {
-  if (!m_world->isMainWorld())
+  if (m_frame->isRemoteFrame())
     return;
 
-  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));
 }
 
 void WindowProxy::updateActivityLogger() {
   m_scriptState->perContextData()->setActivityLogger(
       V8DOMActivityLogger::activityLogger(
           m_world->worldId(),
           m_frame->isLocalFrame() && toLocalFrame(m_frame)->document()
               ? toLocalFrame(m_frame)->document()->baseURI()
@@ skipping 45 matching lines @@
     }
     token = frameSecurityToken + token;
   }
 
   // NOTE: V8 does identity comparison in fast path, must use a symbol
   // as the security token.
   context->SetSecurityToken(v8AtomicString(m_isolate, token));
 }
 
 void WindowProxy::updateDocument() {
-  ASSERT(m_world->isMainWorld());
+  DCHECK(m_world->isMainWorld());
   if (!isGlobalInitialized())
     return;
   if (!isContextInitialized())
     return;
   updateActivityLogger();
   updateDocumentProperty();
   updateSecurityOrigin(m_frame->securityContext()->getSecurityOrigin());
 }
 
 static v8::Local<v8::Value> getNamedProperty(
@@ skipping 36 matching lines @@
     return;
   }
   v8::Local<v8::Value> value;
   if (info.Holder()
           ->GetRealNamedPropertyInPrototypeChain(
               info.GetIsolate()->GetCurrentContext(), property.As<v8::String>())
           .ToLocal(&value))
     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;
 
   if (document->hasNamedItem(name) || document->hasExtraNamedItem(name))
     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) {
   if (!isContextInitialized())
     return;
   setSecurityToken(origin);
 }
 
 }  // namespace blink