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

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

 /*
  * Copyright (C) 1998, 1999 Torben Weis <weis@kde.org>
  *                     1999 Lars Knoll <knoll@kde.org>
  *                     1999 Antti Koivisto <koivisto@kde.org>
  *                     2000 Simon Hausmann <hausmann@kde.org>
  *                     2000 Stefan Schimanski <1Stein@gmx.de>
  *                     2001 George Staikos <staikos@kde.org>
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2005 Alexey Proskuryakov <ap@nypop.com>
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
@@ skipping 51 matching lines @@
 #include "core/page/ChromeClient.h"
 #include "core/page/FocusController.h"
 #include "core/page/Page.h"
 #include "core/page/scrolling/ScrollingCoordinator.h"
 #include "core/paint/ObjectPainter.h"
 #include "core/paint/PaintInfo.h"
 #include "core/paint/PaintLayer.h"
 #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"
 #include "platform/ScriptForbiddenScope.h"
 #include "platform/graphics/GraphicsContext.h"
 #include "platform/graphics/StaticBitmapImage.h"
 #include "platform/graphics/paint/ClipRecorder.h"
 #include "platform/graphics/paint/PaintController.h"
 #include "platform/graphics/paint/SkPictureBuilder.h"
 #include "platform/graphics/paint/TransformDisplayItem.h"
@@ skipping 149 matching lines @@
 }
 
 inline float parentTextZoomFactor(LocalFrame* frame)
 {
     Frame* parent = frame->tree().parent();
     if (!parent || !parent->isLocalFrame())
         return 1;
     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>;
 
 LocalFrame* LocalFrame::create(FrameLoaderClient* client, FrameHost* host, FrameOwner* owner, ServiceRegistry* serviceRegistry)
 {
     LocalFrame* frame = new LocalFrame(client, host, owner, serviceRegistry ? serviceRegistry : ServiceRegistry::getEmptyServiceRegistry());
     InspectorInstrumentation::frameAttachedToParent(frame);
     return frame;
 }
@@ skipping 231 matching lines @@
     //    will have been done.
     //
     //  - Calling LocalDOMWindow::reset() is not needed (called from
     //    Frame::setDOMWindow().) The Member references it clears will now
     //    die with the window. And the registered DOMWindowProperty instances that don't,
     //    only keep a weak reference to this frame, so there's no need to be
     //    explicitly notified that this frame is going away.
     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;
 }
 
 Document* LocalFrame::document() const
 {
     return m_domWindow ? m_domWindow->document() : nullptr;
 }
 
@@ skipping 311 matching lines @@
     , m_spellChecker(SpellChecker::create(*this))
     , m_selection(FrameSelection::create(this))
     , m_eventHandler(new EventHandler(this))
     , m_console(FrameConsole::create(*this))
     , m_inputMethodController(InputMethodController::create(*this))
     , m_navigationDisableCount(0)
     , m_pageZoomFactor(parentPageZoomFactor(this))
     , m_textZoomFactor(parentTextZoomFactor(this))
     , m_inViewSourceMode(false)
     , m_serviceRegistry(serviceRegistry)
+    , m_visibilityWasLogged(false)
+    , m_creationWasLogged(false)
 {
     if (isLocalRoot())
         m_instrumentingAgents = new InstrumentingAgents();
     else
         m_instrumentingAgents = localFrameRoot()->m_instrumentingAgents;
 }
 
 WebFrameScheduler* LocalFrame::frameScheduler()
 {
     if (!m_frameScheduler.get())
@@ skipping 15 matching lines @@
     return static_cast<FrameLoaderClient*>(Frame::client());
 }
 
 PluginData* LocalFrame::pluginData() const
 {
     if (!loader().allowPlugins(NotAboutToInstantiatePlugin))
         return nullptr;
     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)
     : m_frame(&frame)
 {
     m_frame->disableNavigation();
 }
 
 FrameNavigationDisabler::~FrameNavigationDisabler()
 {
     m_frame->enableNavigation();
 }
 
 ScopedFrameBlamer::ScopedFrameBlamer(LocalFrame* frame)
     : m_frame(frame)
 {
     if (m_frame && m_frame->client() && m_frame->client()->frameBlameContext())
         m_frame->client()->frameBlameContext()->Enter();
 }
 
 ScopedFrameBlamer::~ScopedFrameBlamer()
 {
     if (m_frame && m_frame->client() && m_frame->client()->frameBlameContext())
         m_frame->client()->frameBlameContext()->Leave();
 }
 
 } // namespace blink