Introduce a lifecycle model to WindowProxy

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 65 matching lines @@
 
 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) {}
+    : m_frame(frame),
+      m_isolate(isolate),
+      m_world(world),
+      m_lifecycle(Lifecycle::ContextUninitialized) {}
 
 WindowProxy::~WindowProxy() {
   // clearForClose() or clearForNavigation() must be invoked before destruction
   // starts.
-  ASSERT(!isContextInitialized());
+  DCHECK(m_lifecycle != Lifecycle::ContextInitialized);
 }
 
 DEFINE_TRACE(WindowProxy) {
   visitor->trace(m_frame);
 }
 
 void WindowProxy::disposeContext(GlobalDetachmentBehavior behavior) {
-  if (!isContextInitialized())
+  if (m_lifecycle != Lifecycle::ContextInitialized)
     return;
 
   ScriptState::Scope scope(m_scriptState.get());
   v8::Local<v8::Context> context = m_scriptState->context();
   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,
@@ skipping 16 matching lines @@
     m_scriptState->detachGlobalObject();
   }
 
   m_scriptState->disposePerContextData();
 
   // It's likely that disposing the context has created a lot of
   // garbage. Notify V8 about this so it'll have a chance of cleaning
   // it up when idle.
   V8GCForContextDispose::instance().notifyContextDisposed(
       m_frame->isMainFrame());
+
+  DCHECK(m_lifecycle == Lifecycle::ContextInitialized);
+  m_lifecycle = Lifecycle::ContextDetached;
 }
 
 void WindowProxy::clearForClose() {
   disposeContext(DoNotDetachGlobal);
 }
 
 void WindowProxy::clearForNavigation() {
   disposeContext(DetachGlobal);
 }
 
 v8::Local<v8::Object> WindowProxy::globalIfNotDetached() {
-  if (!isContextInitialized())
-    return v8::Local<v8::Object>();
-  DCHECK(m_scriptState->contextIsValid());
-  DCHECK(m_globalProxy == m_scriptState->context()->Global());
-  return m_globalProxy.newLocal(m_isolate);
+  if (m_lifecycle == Lifecycle::ContextInitialized) {
+    DCHECK(m_scriptState->contextIsValid());
+    DCHECK(m_globalProxy == m_scriptState->context()->Global());
+    return m_globalProxy.newLocal(m_isolate);
+  }
+  return v8::Local<v8::Object>();
 }
 
 v8::Local<v8::Object> WindowProxy::releaseGlobal() {
-  ASSERT(!isContextInitialized());
-  // If a ScriptState was created, the context was initialized at some point.
+  DCHECK(m_lifecycle != Lifecycle::ContextInitialized);
   // Make sure the global object was detached from the proxy by calling
   // clearForNavigation().
-  if (m_scriptState)
+  if (m_lifecycle == Lifecycle::ContextDetached)
     ASSERT(m_scriptState->isGlobalObjectDetached());
+
   v8::Local<v8::Object> global = m_globalProxy.newLocal(m_isolate);
   m_globalProxy.clear();
   return global;
 }
 
 void WindowProxy::setGlobal(v8::Local<v8::Object> global) {
   m_globalProxy.set(m_isolate, global);
 
   // Initialize the window proxy now, to re-establish the connection between
   // the global object and the v8::Context. This is really only needed for a
@@ skipping 33 matching lines @@
 // has a security token which is the domain. The outer window cannot
 // have its own properties. window.foo = 'x' is delegated to the
 // inner window.
 //
 // When a frame navigates to a new page, the inner window is cut off
 // the outer window, and the outer window identify is preserved for
 // the frame. However, a new inner window is created for the new page.
 // If there are JS code holds a closure to the old inner window,
 // it won't be able to reach the outer window via its global object.
 void WindowProxy::initializeIfNeeded() {
-  if (isContextInitialized())
-    return;
-  initialize();
-
-  if (m_world->isMainWorld() && m_frame->isLocalFrame())
-    toLocalFrame(m_frame)->loader().dispatchDidClearWindowObjectInMainWorld();
+  // TODO(haraken): It is wrong to re-initialize an already detached window
+  // proxy. This must be 'if(m_lifecycle == Lifecycle::ContextUninitialized)'.

[haraken, 2017/01/06 10:53:19]

If I change the condition to 'if(m_lifecycle ==
Lifecycle::ContextUninitialized)', a couple of webkit_unit_tests start failing.
This is clearly a bug. I'll investigate the bug after landing this CL.

Note that this CL does not change any existing behavior. This CL preserves the
existing wrong behavior.

+  if (m_lifecycle != Lifecycle::ContextInitialized) {
+    initialize();
+    if (m_world->isMainWorld() && m_frame->isLocalFrame())
+      toLocalFrame(m_frame)->loader().dispatchDidClearWindowObjectInMainWorld();
+  }
 }
 
 void WindowProxy::initialize() {
   TRACE_EVENT1("v8", "WindowProxy::initialize", "isMainWindow",
                m_frame->isMainFrame());
   SCOPED_BLINK_UMA_HISTOGRAM_TIMER(
       m_frame->isMainFrame() ? "Blink.Binding.InitializeMainWindowProxy"
                              : "Blink.Binding.InitializeNonMainWindowProxy");
 
   ScriptForbiddenScope::AllowUserAgentScript allowScript;
 
   v8::HandleScope handleScope(m_isolate);
 
   createContext();
-  CHECK(isContextInitialized());
 
   ScriptState::Scope scope(m_scriptState.get());
   v8::Local<v8::Context> context = m_scriptState->context();
   if (m_globalProxy.isEmpty()) {
     m_globalProxy.set(m_isolate, context->Global());
     CHECK(!m_globalProxy.isEmpty());
   }
 
   setupWindowPrototypeChain();
 
@@ skipping 65 matching lines @@
   {
     V8PerIsolateData::UseCounterDisabledScope useCounterDisabled(
         V8PerIsolateData::from(m_isolate));
     context =
         v8::Context::New(m_isolate, &extensionConfiguration, globalTemplate,
                          m_globalProxy.newLocal(m_isolate));
   }
   CHECK(!context.IsEmpty());
 
   m_scriptState = ScriptState::create(context, m_world);
+
+  // TODO(haraken): Currently we cannot enable the following DCHECK because
+  // an already detached window proxy can be re-initialized. This is wrong.
+  // DCHECK(m_lifecycle == Lifecycle::ContextUninitialized);

[haraken, 2017/01/06 10:53:19]

Ditto.

+  m_lifecycle = Lifecycle::ContextInitialized;
+  DCHECK(m_scriptState->contextIsValid());
 }
 
 void WindowProxy::setupWindowPrototypeChain() {
   // Associate the window wrapper object and its prototype chain with the
   // corresponding native DOMWindow object.
   // The full structure of the global object's prototype chain is as follows:
   //
   // global proxy object [1]
   //   -- has prototype --> global object (window wrapper object) [2]
   //   -- has prototype --> Window.prototype
@@ skipping 128 matching lines @@
 
   // 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() {
   DCHECK(m_world->isMainWorld());
   // For an uninitialized main window proxy, there's nothing we need
   // to update. The update is done when the window proxy gets initialized later.
-  if (!isContextInitialized())
+  if (m_lifecycle == Lifecycle::ContextUninitialized)
+    return;
+  // TODO(yukishiino): Is it okay to not update document when the context
+  // is detached? It's not trivial to fix this because udpateDocumentProperty
+  // requires a not-yet-detached context to instantiate a document wrapper.
+  if (m_lifecycle == Lifecycle::ContextDetached)
     return;
 
   updateActivityLogger();
   updateDocumentProperty();
   updateSecurityOrigin(m_frame->securityContext()->getSecurityOrigin());
 }
 
 static v8::Local<v8::Value> getNamedProperty(
     HTMLDocument* htmlDocument,
     const AtomicString& key,
@@ skipping 37 matching lines @@
   if (info.Holder()
           ->GetRealNamedPropertyInPrototypeChain(
               info.GetIsolate()->GetCurrentContext(), property.As<v8::String>())
           .ToLocal(&value))
     v8SetReturnValue(info, value);
 }
 
 void WindowProxy::namedItemAdded(HTMLDocument* document,
                                  const AtomicString& name) {
   DCHECK(m_world->isMainWorld());
-  DCHECK(m_scriptState);
-  if (!isContextInitialized())
+
+  // Context must be initialized before this point.
+  DCHECK(m_lifecycle >= Lifecycle::ContextInitialized);
+  // TODO(yukishiino): Is it okay to not update named properties
+  // after the context gets detached?
+  if (m_lifecycle == Lifecycle::ContextDetached)
     return;
 
   ScriptState::Scope scope(m_scriptState.get());
   v8::Local<v8::Object> documentWrapper =
       m_world->domDataStore().get(document, m_isolate);
   // TODO(yukishiino,peria): We should check if the own property with the same
   // name already exists or not, and if it exists, we shouldn't define a new
   // accessor property (it fails).
   documentWrapper->SetAccessor(m_isolate->GetCurrentContext(),
                                v8String(m_isolate, name), getter);
 }
 
 void WindowProxy::namedItemRemoved(HTMLDocument* document,
                                    const AtomicString& name) {
   DCHECK(m_world->isMainWorld());
-  DCHECK(m_scriptState);
-  if (!isContextInitialized())
+
+  // Context must be initialized before this point.
+  DCHECK(m_lifecycle >= Lifecycle::ContextInitialized);
+  // TODO(yukishiino): Is it okay to not update named properties
+  // after the context gets detached?
+  if (m_lifecycle == Lifecycle::ContextDetached)
     return;
+
   if (document->hasNamedItem(name) || document->hasExtraNamedItem(name))
     return;
-
   ScriptState::Scope scope(m_scriptState.get());
   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())
+  // For an uninitialized main window proxy, there's nothing we need
+  // to update. The update is done when the window proxy gets initialized later.
+  if (m_lifecycle == Lifecycle::ContextUninitialized)
     return;
+  // TODO(yukishiino): Is it okay to not update security origin when the context
+  // is detached?
+  if (m_lifecycle == Lifecycle::ContextDetached)
+    return;
+
   setSecurityToken(origin);
 }
 
 }  // namespace blink