Deferred frame loading stats v2

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 1d02cf51a8f803f3045df673fe1cb1b8534986fd..d453e3c439864907019a02f8f600b2784b1bd1c5 100644
--- a/third_party/WebKit/Source/core/dom/Document.cpp
+++ b/third_party/WebKit/Source/core/dom/Document.cpp
@@ -398,22 +398,11 @@ static void runAutofocusTask(ExecutionContext* context) {
   }
 }
 
-// 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 recordLoadReasonToHistogram(WouldLoadReason reason) {
+  DEFINE_STATIC_LOCAL(
+      EnumerationHistogram, unseenFrameHistogram,
+      ("Navigation.DeferredDocumentLoading.StatesV2", WouldLoadReasonEnd));
+  unseenFrameHistogram.count(reason);
 }
 
 Document::Document(const DocumentInit& initializer,
@@ -496,7 +485,7 @@ Document::Document(const DocumentInit& initializer,
       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());
@@ -528,10 +517,13 @@ Document::Document(const DocumentInit& initializer,
 
   initSecurityContext(initializer);
   DCHECK(getSecurityOrigin());
-  if (frame() && frame()->tree().top()->securityContext() &&
+  if (frame() &&
+      frame()->loader().stateMachine()->committedFirstRealDocumentLoad() &&
+      frame()->tree().top()->securityContext() &&

[dgrogan, 2016/10/18 22:11:45]

Even checking committedFirstRealDocumentLoad() first, changing this to
frame()->isCrossOriginSubframe() still dies with the same null pointer deref.

[dcheng, 2016/10/19 01:34:54]

OK, yeah, I think this logic should really just live in installNewDocument().

[dgrogan, 2016/10/19 18:59:54]

Done.

       !getSecurityOrigin()->canAccess(
-          frame()->tree().top()->securityContext()->getSecurityOrigin()))
-    RecordStateToHistogram(Created);
+          frame()->tree().top()->securityContext()->getSecurityOrigin())) {
+    recordLoadReasonToHistogram(Created);
+  }
 
   initDNSPrefetch();
 
@@ -6365,12 +6357,14 @@ DEFINE_TRACE(Document) {
   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()) {
+    recordLoadReasonToHistogram(reason);
   }
+  m_wouldLoadReason = reason;
 }
 
 DEFINE_TRACE_WRAPPERS(Document) {