Duplicate WindowProxy::disposeContext() into subclasses.

third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.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 19 matching lines @@
 
 #include "bindings/core/v8/LocalWindowProxy.h"
 
 #include "bindings/core/v8/ConditionalFeaturesForCore.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/V8DOMWrapper.h"
+#include "bindings/core/v8/V8GCForContextDispose.h"
 #include "bindings/core/v8/V8HTMLDocument.h"
 #include "bindings/core/v8/V8HiddenValue.h"
 #include "bindings/core/v8/V8Initializer.h"
 #include "bindings/core/v8/V8PagePopupControllerBinding.h"
 #include "bindings/core/v8/V8PrivateProperty.h"
 #include "bindings/core/v8/V8Window.h"
 #include "core/dom/Modulator.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/LocalFrameClient.h"
 #include "core/frame/csp/ContentSecurityPolicy.h"
@@ skipping 20 matching lines @@
 
   ScriptState::Scope scope(m_scriptState.get());
   v8::Local<v8::Context> context = m_scriptState->context();
   // 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());
 
-  WindowProxy::disposeContext(behavior);
+  if (behavior == DetachGlobal) {
+    v8::Local<v8::Context> context = m_scriptState->context();
+    // 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);
+    }
+    V8DOMWrapper::clearNativeInfo(isolate(), context->Global());

[haraken, 2017/03/08 06:21:37]

Is your crash related to the fact that we're explicitly clearing an internal
field here? I think this is the only place we clear an internal field.

[dcheng, 2017/03/08 06:46:40]

It's unclear. In theory, we shouldn't clear the internal fields without clearing
the wrapper class ID as well...

However, I think the DCHECK on line 87 has a hint to the problem. It seems like
it's possible to trigger situations where m_globalProxy != context->Global(). I
guess my WindowProxy CL must have made that more common.

Looking at the crash stack in
https://bugs.chromium.org/p/chromium/issues/detail?id=698739,
initializeIfNeeded() is crashing in the already-initialized branch. However, at
this point, we are in WebFrame::swap(): the frame we're swapping in should be
new, and should never have been initialized yet.

If it is initialized, it is a problem because:
- We call setGlobal() to transfer the global proxy during frame swap
- setGlobal() unconditionally overwrites m_globalProxy
- But context->Global() is already initialized and points to another global
proxy
- In teardown, we clear the wrapper class ID on m_globalProxy, but clear the
internal fields on context->Global().

Now, context->Global() has null internal fields but still has a wrapper class
ID.

+mlippautz, who mentioned that we only process non-null things from the marking
deque:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

But I'm not sure if that check would help here.

Regardless, I'm planning on investigating this by changing line 87/88 to CHECKs.
In WindowProxy::initializeIfNeeded(), I will capture a stack trace. This should
help us understand how we get into this situation.

+    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(
+      frame()->isMainFrame());
+
+  DCHECK(m_lifecycle == Lifecycle::ContextInitialized);
+  m_lifecycle = Lifecycle::ContextDetached;
 }
 
 void LocalWindowProxy::initialize() {
   TRACE_EVENT1("v8", "LocalWindowProxy::initialize", "isMainWindow",
                frame()->isMainFrame());
   SCOPED_BLINK_UMA_HISTOGRAM_TIMER(
       frame()->isMainFrame() ? "Blink.Binding.InitializeMainWindowProxy"
                              : "Blink.Binding.InitializeNonMainWindowProxy");
 
   ScriptForbiddenScope::AllowUserAgentScript allowScript;
@@ skipping 314 matching lines @@
 
   setSecurityToken(origin);
 }
 
 LocalWindowProxy::LocalWindowProxy(v8::Isolate* isolate,
                                    LocalFrame& frame,
                                    RefPtr<DOMWrapperWorld> world)
     : WindowProxy(isolate, frame, std::move(world)) {}
 
 }  // namespace blink