Devirtualize Frame::domWindow().

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

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  *           (C) 2006 Alexey Proskuryakov (ap@webkit.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2011, 2012 Apple Inc. All
  * rights reserved.
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  * (http://www.torchmobile.com/)
  * Copyright (C) 2008, 2009, 2011, 2012 Google Inc. All rights reserved.
@@ skipping 391 matching lines @@
 }
 
 Document::Document(const DocumentInit& initializer,
                    DocumentClassFlags documentClasses)
     : ContainerNode(0, CreateDocument),
       TreeScope(*this),
       m_hasNodesWithPlaceholderStyle(false),
       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),
       m_implementation(this, nullptr),
       m_printing(NotPrinting),
       m_paginatedForScreen(false),
       m_compatibilityMode(NoQuirksMode),
       m_compatibilityModeLocked(false),
       m_hasAutofocused(false),
       m_clearFocusedElementTimer(
@@ skipping 69 matching lines @@
       m_nodeCount(0),
       m_wouldLoadReason(Created),
       m_passwordCount(0) {
   if (m_frame) {
     DCHECK(m_frame->page());
     provideContextFeaturesToDocumentFrom(*this, *m_frame->page());
 
     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) {
     m_fetcher = FrameFetchContext::createFetcherFromDocument(this);
   } else {
     m_fetcher = ResourceFetcher::create(nullptr);
   }
   DCHECK(m_fetcher);
 
   m_rootScrollerController = RootScrollerController::create(*this);
@@ skipping 2505 matching lines @@
 
       DocumentLoader* documentLoader =
           m_frame->loader().provisionalDocumentLoader();
       m_loadEventProgress = UnloadEventInProgress;
       Event* unloadEvent(Event::create(EventTypeNames::unload));
       if (documentLoader && !documentLoader->timing().unloadEventStart() &&
           !documentLoader->timing().unloadEventEnd()) {
         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;
   }
 
   if (!m_frame)
     return;
 
   // Don't remove event listeners from a transitional empty document (see
   // https://bugs.webkit.org/show_bug.cgi?id=28716 for more information).
@@ skipping 3542 matching lines @@
 }
 
 void showLiveDocumentInstances() {
   WeakDocumentSet& set = liveDocumentSet();
   fprintf(stderr, "There are %u documents currently alive:\n", set.size());
   for (Document* document : set)
     fprintf(stderr, "- Document %p URL: %s\n", document,
             document->url().getString().utf8().data());
 }
 #endif