Log to UMA when x-origin frames (1) are created and (2) become visible

third_party/WebKit/Source/core/frame/LocalFrame.cpp

Index: third_party/WebKit/Source/core/frame/LocalFrame.cpp
diff --git a/third_party/WebKit/Source/core/frame/LocalFrame.cpp b/third_party/WebKit/Source/core/frame/LocalFrame.cpp
index ccb59c9d1649bfd00846ebd78fbd0b4b0676d3a7..890d93b260e11fbed475e05381294e3d350e3970 100644
--- a/third_party/WebKit/Source/core/frame/LocalFrame.cpp
+++ b/third_party/WebKit/Source/core/frame/LocalFrame.cpp
@@ -69,6 +69,7 @@
 #include "core/paint/TransformRecorder.h"
 #include "core/svg/SVGDocumentExtensions.h"
 #include "platform/DragImage.h"
+#include "platform/Histogram.h"
 #include "platform/JSONValues.h"
 #include "platform/PluginScriptForbiddenScope.h"
 #include "platform/RuntimeEnabledFeatures.h"
@@ -238,6 +239,21 @@ inline float parentTextZoomFactor(LocalFrame* frame)
     return toLocalFrame(parent)->textZoomFactor();
 }
 
+// These are logged to UMA, so don't re-arrange them without creating a new histogram.
+enum FrameStateForDeferredLoading {
+    Created,
+    WouldLoadBecauseVisible,
+    // TODO(dgrogan): Add WouldLoadBecauseTopOrLeft, WouldLoadBecauseDisplayNone, etc
+
+    FrameStateForDeferredLoadingEnd
+};
+
+void RecordStateToHistogram(FrameStateForDeferredLoading state)
+{
+    DEFINE_STATIC_LOCAL(EnumerationHistogram, unseenFrameHistogram, ("FrameLoading.Deferred.StateCountsV1", FrameStateForDeferredLoadingEnd));
+    unseenFrameHistogram.count(state);
+}
+
 } // namespace
 
 template class CORE_TEMPLATE_EXPORT Supplement<LocalFrame>;
@@ -489,6 +505,8 @@ void LocalFrame::setDOMWindow(LocalDOMWindow* domWindow)
     if (domWindow)
         script().clearWindowProxy();
 
+    // Can't call isCrossOrigin until we get here, which seems to happen
+    // more than once.

[ojan, 2016/08/08 22:01:54]

Nit:
I think this would get called every time a frame is navigated? Or are you seeing
it multiple times with a single frame's page load?

s/seems to happen/happens/

More importantly, I think this comment is not in the most useful place. The
DCHECK you added to isCrossOrigin seems to cover this?

[dgrogan, 2016/08/09 00:30:16]

In my testing it happens exactly twice per frame load, but I didn't dig in to
find out why.

About the comment, agreed it has no place in the repository.

     if (m_domWindow)
         m_domWindow->reset();
     m_domWindow = domWindow;
@@ -820,6 +838,8 @@ inline LocalFrame::LocalFrame(FrameLoaderClient* client, FrameHost* host, FrameO
     , m_textZoomFactor(parentTextZoomFactor(this))
     , m_inViewSourceMode(false)
     , m_serviceRegistry(serviceRegistry)
+    , m_visibilityWasLogged(false)
+    , m_creationWasLogged(false)
 {
     if (isLocalRoot())
         m_instrumentingAgents = new InstrumentingAgents();
@@ -855,6 +875,27 @@ PluginData* LocalFrame::pluginData() const
     return page()->pluginData();
 }
 
+void LocalFrame::onVisibilityMaybeChanged(bool visible)
+{
+    // Which x-origin check do we want? Frame::isCrossOrigin checks this origin against
+    // the top. But FrameView::m_crossOriginForThrottling is set if this origin is unable
+    // to access ANY origin between here and the top, inclusive.
+    if (!isCrossOrigin())

[ojan, 2016/08/08 22:01:54]

I think this is probably the right one. I think the comment is good for now, but
should probably be phrased in terms of a TODO to figure out which one to use
long-term.

+        return;
+
+    // Logging creation here is lame but I don't know where else to do it.
+    // Can't do it in the ctor because isCrossOrigin relies on document(), which isn't set yet.

[ojan, 2016/08/08 22:01:54]

How about we log this in setDOMWindow? That actually seems better in retrospect
because it will reset everytime a frame is navigated (which is the behavior we
want I think).

Also in setDOMWindow, you can reset m_visibilityWasLogged = false so that we get
a fresh count for each time a frame is navigated.

[dgrogan, 2016/08/09 00:30:16]

We have the same isCrossOrigin problem there: the domWindow object has a null
m_document, so isCrossOrigin fails the new DCHECK.
Even if we could log from setDOMWindow, I'm not sure we'd want to reset
m_visibilityWasLogged. Resetting could lead to more visible pages than created
pages... unless I'm misunderstanding what you mean.

[dcheng, 2016/08/09 01:24:04]

If this needs to depend on isCrossOrigin(), it should probably be done in
installNewDocument() or later: until that's complete, there's no meaningful
security origin.

+    if (!m_creationWasLogged) {
+        m_creationWasLogged = true;
+        RecordStateToHistogram(Created);
+    }
+
+    if (visible && !m_visibilityWasLogged) {
+        m_visibilityWasLogged = true;
+        RecordStateToHistogram(WouldLoadBecauseVisible);
+    }
+}
+
 DEFINE_WEAK_IDENTIFIER_MAP(LocalFrame);
 
 FrameNavigationDisabler::FrameNavigationDisabler(LocalFrame& frame)