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 083ed51d91f4be61eec1603eeb3ac754d5ea3198..cf798e4f370a121b996bc1fd6025c27412574148 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 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,
@@ -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());
@@ -530,8 +519,10 @@ Document::Document(const DocumentInit& 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();
 
@@ -6366,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()) {
+    recordReasonToHistogram(reason);
   }
+  m_wouldLoadReason = reason;
 }
 
 DEFINE_TRACE_WRAPPERS(Document) {