Deferred frame loading stats v2

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 380 matching lines @@
 
 // This doesn't work with non-Document ExecutionContext.
 static void runAutofocusTask(ExecutionContext* context) {
   Document* document = toDocument(context);
   if (Element* element = document->autofocusElement()) {
     document->setAutofocusElement(0);
     element->focus();
   }
 }
 
-// These are logged to UMA, so don't re-arrange them without creating a new
-// histogram.
-enum DocumentVisibilityForDeferredLoading {
-  Created,
-  WouldLoadBecauseVisible,
-  // TODO(dgrogan): Add WouldLoadBecauseTopOrLeft, WouldLoadBecauseDisplayNone,
-  // etc
-
-  DocumentVisibilityForDeferredLoadingEnd
-};
-
-static void RecordStateToHistogram(DocumentVisibilityForDeferredLoading state) {
-  DEFINE_STATIC_LOCAL(EnumerationHistogram, unseenFrameHistogram,
-                      ("Navigation.DeferredDocumentLoading.StatesV1",
-                       DocumentVisibilityForDeferredLoadingEnd));
-  unseenFrameHistogram.count(state);
+static void recordReasonToHistogram(WouldLoadReason reason) {

[dcheng, 2016/10/08 00:46:22]

Nit: recordLoadReasonToHistogram

[dgrogan, 2016/10/13 01:06:21]

Done.

+  DEFINE_STATIC_LOCAL(
+      EnumerationHistogram, unseenFrameHistogram,
+      ("Navigation.DeferredDocumentLoading.StatesV2", WouldLoadReasonEnd));
+  unseenFrameHistogram.count(reason);
 }
 
 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()),
@@ skipping 62 matching lines @@
       m_timeline(DocumentTimeline::create(this)),
       m_compositorPendingAnimations(new CompositorPendingAnimations()),
       m_templateDocumentHost(nullptr),
       m_didAssociateFormControlsTimer(
           this,
           &Document::didAssociateFormControlsTimerFired),
       m_timers(TaskRunnerHelper::get(TaskType::Timer, this)->clone()),
       m_hasViewportUnits(false),
       m_parserSyncPolicy(AllowAsynchronousParsing),
       m_nodeCount(0),
-      m_visibilityWasLogged(false) {
+      m_wouldLoadReason(Created) {
   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);
 
     CustomElementRegistry* registry =
         m_frame->localDOMWindow()
             ? m_frame->localDOMWindow()->maybeCustomElements()
@@ skipping 11 matching lines @@
   // We depend on the url getting immediately set in subframes, but we
   // also depend on the url NOT getting immediately set in opened windows.
   // See fast/dom/early-frame-url.html
   // and fast/dom/location-new-window-no-crash.html, respectively.
   // FIXME: Can/should we unify this behavior?
   if (initializer.shouldSetURL())
     setURL(initializer.url());
 
   initSecurityContext(initializer);
   DCHECK(getSecurityOrigin());
   if (frame() && frame()->tree().top()->securityContext() &&

[dcheng, 2016/10/08 00:46:22]

Nit: Not introduced by this CL, but can this be frame() &&
frame()->isCrossOriginSubframe()?

[dgrogan, 2016/10/13 01:06:21]

That would be nice but unfortunately not; LocalFrame::document() returns
nullptr. The DCHECK this CL adds to LocalFrame.cpp fails with the stack below.
[25406:25406:1012/152748:2351789511288:FATAL:LocalFrame.cpp(541)] Check failed:
securityContext(). 
#0 0x7fe4542cbb4e base::debug::StackTrace::StackTrace()
#1 0x7fe4543395ff logging::LogMessage::~LogMessage()
#2 0x7fe4503a7e5d blink::LocalFrame::isCrossOriginSubframe()
#3 0x7fe44ff30f5a blink::Document::Document()
#4 0x7fe4504457f5 blink::HTMLDocument::HTMLDocument()
#5 0x7fe44ff13aed blink::HTMLDocument::create()
#6 0x7fe44ff1343d blink::DOMImplementation::createDocument()
#7 0x7fe45038fd2b blink::LocalDOMWindow::createDocument()
#8 0x7fe45038fe2c blink::LocalDOMWindow::installNewDocument()
#9 0x7fe450a75f37 blink::DocumentLoader::createWriterFor()
#10 0x7fe450a75bbf blink::DocumentLoader::ensureWriter()
#11 0x7fe450a742f5 blink::DocumentLoader::commitData()
#12 0x7fe450a73e27 blink::DocumentLoader::finishedLoading()
#13 0x7fe450a76b14 blink::DocumentLoader::maybeLoadEmpty()
#14 0x7fe450a76cf1 blink::DocumentLoader::startLoadingMainResource()
#15 0x7fe450a94fd1 blink::FrameLoader::init()
#16 0x7fe4548151fc blink::LocalFrame::init()
#17 0x7fe4548cb493 blink::WebLocalFrameImpl::initializeCoreFrame()
#18 0x7fe454913146 blink::WebViewImpl::setMainFrame()

[dcheng, 2016/10/13 23:00:43]

Let's just update isCrossOriginSubframe() to handle this case: returning false
immediately when isMainFrame is true should handle this, I think.

[dgrogan, 2016/10/14 21:45:08]

Same stacktrace :( But thanks for looking into this, the reimplementation of
that function in this constructor is a terrible smell.

I'm completely hand-waving but it's like the localframe doesn't know about the
new document yet.

[dcheng, 2016/10/14 23:40:30]

Ah OK. So I patched this in and I understand the problem: it's a chicken-and-egg
problem.

When a frame is first created, it has no document attached. Thus,
LocalFrame::securityContext() will return null. When we create the initial empty
document for that frame and call isCrossOriginSubframe(), it's unable to say.

If we move this check after m_document is set in
LocalDOMWindow::installNewDocument(), then it will work.

I guess my question is: how long do we expect this code to live in tree?
Indefinitely? or just for some time while we gather stats? The cleaner way is to
probably just put it in installNewDocument(), but that requires exposing more
stuff on Document.

       !getSecurityOrigin()->canAccess(
-          frame()->tree().top()->securityContext()->getSecurityOrigin()))
-    RecordStateToHistogram(Created);
+          frame()->tree().top()->securityContext()->getSecurityOrigin()) &&
+      frame()->loader().stateMachine()->committedFirstRealDocumentLoad()) {
+    recordReasonToHistogram(Created);
+  }
 
   initDNSPrefetch();
 
   InstanceCounters::incrementCounter(InstanceCounters::DocumentCounter);
 
   m_lifecycle.advanceTo(DocumentLifecycle::Inactive);
 
   // Since CSSFontSelector requires Document::m_fetcher and StyleEngine owns
   // CSSFontSelector, need to initialize m_styleEngine after initializing
   // m_fetcher.
@@ skipping 5814 matching lines @@
   visitor->trace(m_resizeObserverController);
   visitor->trace(m_propertyRegistry);
   visitor->trace(m_nonAttachedStyle);
   Supplementable<Document>::trace(visitor);
   TreeScope::trace(visitor);
   ContainerNode::trace(visitor);
   ExecutionContext::trace(visitor);
   SecurityContext::trace(visitor);
 }
 
-void Document::onVisibilityMaybeChanged(bool visible) {
+void Document::wouldLoadBecause(WouldLoadReason reason) {
+  DCHECK(reason != Created);
   DCHECK(frame());
-  if (visible && !m_visibilityWasLogged && frame()->isCrossOriginSubframe()) {
-    m_visibilityWasLogged = true;
-    RecordStateToHistogram(WouldLoadBecauseVisible);
+  if (m_wouldLoadReason == Created && frame()->isCrossOriginSubframe() &&
+      frame()->loader().stateMachine()->committedFirstRealDocumentLoad()) {
+    recordReasonToHistogram(reason);
   }
+  m_wouldLoadReason = reason;
 }
 
 DEFINE_TRACE_WRAPPERS(Document) {
   visitor->traceWrappers(m_importsController);
   visitor->traceWrappers(m_implementation);
   visitor->traceWrappers(m_styleSheetList);
   visitor->traceWrappers(m_styleEngine);
   visitor->traceWrappers(Supplementable<Document>::m_supplements.get(
       FontFaceSet::supplementName()));
   for (int i = 0; i < numNodeListInvalidationTypes; ++i) {
@@ skipping 17 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