Devirtualize Frame::domWindow().

third_party/WebKit/Source/core/dom/Document.cpp

Index: third_party/WebKit/Source/core/dom/Document.cpp
diff --git a/third_party/WebKit/Source/core/dom/Document.cpp b/third_party/WebKit/Source/core/dom/Document.cpp
index 696a3f2a897752505930b541a4dc930940e74ff4..8be0ee4e17ee8f4bd46ba46dc494769f676102b9 100644
--- a/third_party/WebKit/Source/core/dom/Document.cpp
+++ b/third_party/WebKit/Source/core/dom/Document.cpp
@@ -409,7 +409,9 @@ Document::Document(const DocumentInit& initializer,
       m_evaluateMediaQueriesOnStyleRecalc(false),
       m_pendingSheetLayout(NoLayoutWithPendingSheets),
       m_frame(initializer.frame()),
-      m_domWindow(m_frame ? m_frame->localDOMWindow() : 0),
+      // TODO(dcheng): Why does this need both a LocalFrame and LocalDOMWindow
+      // pointer?

[haraken, 2016/12/12 09:54:59]

I want to guarantee the following fact:

- When a context is detached, frame()->domWindow(), domWindow()->document(),
document()->frame() and frame()->document() start returning nullptr.

To that goal, I tried to clear domWindow()->document() when the context gets
detached (https://codereview.chromium.org/2393133005) but found that it needs to
fix a lot of call sites that are assuming that domWindow()->document() doesn't
return null after the detachment...

[dcheng, 2016/12/12 10:21:39]

Yes, that's basically not possible today: because JS can access properties of a
detached window, we can't clear any of the pointers when a frame is detached.

If we ever want to change this invariant, we'll have to revisit this, but until
then, I think it would be better if code doesn't null-check unnecessarily,
because it makes the invariants harder to understand.

[dcheng, 2016/12/12 10:32:23]

ALso, in a followup CL, I want to explore removing m_frame or m_domWindow, so
it's easier to keep these two in sync. That would help resolve the TODO below as
well. Probaby I would want to remove m_frame, since we clear
m_domWindow->frame() on detach already.

[haraken, 2016/12/13 07:22:22]

You're right that e.g., frame()->domWindow() should not be cleared when the
context is detached. However, document()->frame() is cleared when the context is
detached.

I think we shouldn't break the connection between Frame and Window on the
context detachment. However, I think we should break the connection between
Document and Frame and the connection between Document and Window.

That's confusing anyway :)

+      m_domWindow(m_frame ? m_frame->domWindow() : nullptr),
       m_importsController(this, initializer.importsController()),
       m_contextFeatures(ContextFeatures::defaultSwitch()),
       m_wellFormed(false),
@@ -499,10 +501,10 @@ Document::Document(const DocumentInit& initializer,
     m_fetcher = m_frame->loader().documentLoader()->fetcher();
     FrameFetchContext::provideDocumentToContext(m_fetcher->context(), this);
 
+    // TODO(dcheng): Why does this need to check that DOMWindow is non-null?
     CustomElementRegistry* registry =
-        m_frame->localDOMWindow()
-            ? m_frame->localDOMWindow()->maybeCustomElements()
-            : nullptr;
+        m_frame->domWindow() ? m_frame->domWindow()->maybeCustomElements()
+                             : nullptr;
     if (registry && m_registrationContext)
       registry->entangle(m_registrationContext);
   } else if (m_importsController) {
@@ -3028,11 +3030,10 @@ void Document::dispatchUnloadEvents() {
         DocumentLoadTiming& timing = documentLoader->timing();
         DCHECK(timing.navigationStart());
         timing.markUnloadEventStart();
-        m_frame->localDOMWindow()->dispatchEvent(unloadEvent, this);
+        m_frame->domWindow()->dispatchEvent(unloadEvent, this);
         timing.markUnloadEventEnd();
       } else {
-        m_frame->localDOMWindow()->dispatchEvent(unloadEvent,
-                                                 m_frame->document());
+        m_frame->domWindow()->dispatchEvent(unloadEvent, m_frame->document());
       }
     }
     m_loadEventProgress = UnloadEventHandled;