v8binding: Initializes WindowProxy iff it's uninitialized.

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

Index: third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp b/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp
index 139ed88755d5dc6b65b9f7e211821b49bc008987..dfbac29a304a264a17e3f4cbd5e83fffc389dd75 100644
--- a/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp
@@ -104,8 +104,9 @@ void LocalWindowProxy::disposeContext(GlobalDetachmentBehavior behavior) {
   V8GCForContextDispose::instance().notifyContextDisposed(
       frame()->isMainFrame());
 
-  DCHECK(m_lifecycle == Lifecycle::ContextInitialized);
-  m_lifecycle = Lifecycle::ContextDetached;
+  DCHECK_EQ(m_lifecycle, Lifecycle::ContextInitialized);
+  m_lifecycle = behavior == DetachGlobal ? Lifecycle::GlobalObjectDetached
+                                         : Lifecycle::ContextDetachedFromFrame;
 }
 
 void LocalWindowProxy::initialize() {
@@ -196,9 +197,8 @@ void LocalWindowProxy::createContext() {
 
   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);
+  DCHECK(m_lifecycle == Lifecycle::ContextUninitialized ||
+         m_lifecycle == Lifecycle::GlobalObjectDetached);
   m_lifecycle = Lifecycle::ContextInitialized;
   DCHECK(m_scriptState->contextIsValid());
 }
@@ -318,10 +318,9 @@ void LocalWindowProxy::updateDocument() {
   if (m_lifecycle == Lifecycle::ContextUninitialized)
     return;
 
-  // If this WindowProxy was previously initialized, reinitialize it now to
-  // preserve JS object identity. Otherwise, extant references to the
-  // WindowProxy will be broken.
-  if (m_lifecycle == Lifecycle::ContextDetached) {
+  // For a navigated-away window proxy, reinitialize it as a new window with new
+  // context and document.
+  if (m_lifecycle == Lifecycle::GlobalObjectDetached) {
     initialize();
     DCHECK_EQ(Lifecycle::ContextInitialized, m_lifecycle);
     // Initialization internally updates the document properties, so just
@@ -390,18 +389,12 @@ void LocalWindowProxy::namedItemAdded(HTMLDocument* document,
   DCHECK(m_world->isMainWorld());
 
   // 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)

[dcheng, 2017/03/28 21:25:42]

Is this just reverting us to the original behavior here?

[Yuki, 2017/03/30 14:48:37]

Good catch.  I considered this point again and again, and now I think that:
a) Blink's current implementation reaches here only when ContextIsInitialized.
b) However, we should support FrameIsDetached case, too, as the spec is saying
so.

Updated the code and comments.

I'll leave the if-clause in this CL, but will remove it in a follow-up CL.

-    return;
+  DCHECK(m_lifecycle == Lifecycle::ContextInitialized ||
+         m_lifecycle == Lifecycle::ContextDetachedFromFrame);
 
   ScriptState::Scope scope(m_scriptState.get());
   v8::Local<v8::Object> documentWrapper =
       m_world->domDataStore().get(document, 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(isolate()->GetCurrentContext(),
                                v8String(isolate(), name), getter).ToChecked();
 }
@@ -411,11 +404,8 @@ void LocalWindowProxy::namedItemRemoved(HTMLDocument* document,
   DCHECK(m_world->isMainWorld());
 
   // 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;
+  DCHECK(m_lifecycle == Lifecycle::ContextInitialized ||
+         m_lifecycle == Lifecycle::ContextDetachedFromFrame);
 
   if (document->hasNamedItem(name) || document->hasExtraNamedItem(name))
     return;
@@ -428,13 +418,10 @@ void LocalWindowProxy::namedItemRemoved(HTMLDocument* document,
 }
 
 void LocalWindowProxy::updateSecurityOrigin(SecurityOrigin* origin) {
-  // 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)
+  // For an uninitialized 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 ||
+      m_lifecycle == Lifecycle::GlobalObjectDetached)
     return;
 
   setSecurityToken(origin);