UMA recording the kind of target frame when extensions pierce browsing

instance.

Extensions violate browsing instance boundaries.  The UMA metrics added
in this CL should help confirm or deny the theory that violating
browsing instance boundaries is only needed for looking up background
contents / pages (of VIEW_TYPE_BACKGROUND_CONTENTS type).

Bug: 718489
Change-Id: I69246c7d38962c85c55b2e4d271b19eaae1dae1e
Reviewed-on: https://chromium-review.googlesource.com/766469
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532899}

base/test/histogram_tester.h

@@ -25,6 +25,10 @@ class HistogramSamples;
 // HistogramTester provides a simple interface for examining histograms, UMA
 // or otherwise. Tests can use this interface to verify that histogram data is
 // getting logged as intended.
+//
+// Note: When using this class from a browser test, one might have to call
+// SubprocessMetricsProvider::MergeHistogramDeltasForTesting() to sync the
+// histogram data between the renderer and browser processes.
 class HistogramTester {
  public:
   using CountsMap = std::map<std::string, HistogramBase::Count>;

chrome/browser/extensions/extension_functional_browsertest.cc

@@ -4,11 +4,13 @@
 
 #include <stddef.h>
 
+#include "base/test/histogram_tester.h"
 #include "build/build_config.h"
 #include "chrome/browser/extensions/crx_installer.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_util.h"
+#include "chrome/browser/metrics/subprocess_metrics_provider.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
@@ -23,6 +25,8 @@
 #include "extensions/browser/extension_util.h"
 #include "extensions/browser/notification_types.h"
 #include "extensions/browser/test_extension_registry_observer.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
 namespace extensions {
 
@@ -165,6 +169,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionFunctionalTest,
   // behavior.  We want to change the old behavior - this would expectedly make
   // the assestion below fail and in this case we would need to tweak the test
   // to look-up another window (most likely a background page).
+  base::HistogramTester histogram_tester;
   std::string location_of_opened_window;
   EXPECT_TRUE(ExecuteScriptAndExtractString(
       tab1_popup,
@@ -172,6 +177,24 @@ IN_PROC_BROWSER_TEST_F(ExtensionFunctionalTest,
       "window.domAutomationController.send(w.location.href);",
       &location_of_opened_window));
   EXPECT_EQ(tab2->GetLastCommittedURL(), location_of_opened_window);
+
+  // Verify UMA got recorded as expected.
+  SubprocessMetricsProvider::MergeHistogramDeltasForTesting();
+  EXPECT_THAT(histogram_tester.GetAllSamples(
+                  "Extensions.BrowsingInstanceViolation.ExtensionType"),
+              testing::ElementsAre(base::Bucket(Manifest::TYPE_EXTENSION, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples(
+          "Extensions.BrowsingInstanceViolation.SourceExtensionViewType"),
+      testing::ElementsAre(base::Bucket(VIEW_TYPE_TAB_CONTENTS, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples(
+          "Extensions.BrowsingInstanceViolation.TargetExtensionViewType"),
+      testing::ElementsAre(base::Bucket(VIEW_TYPE_TAB_CONTENTS, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples(
+          "Extensions.BrowsingInstanceViolation.IsBackgroundSourceOrTarget"),
+      testing::ElementsAre(base::Bucket(false, 1)));
 }
 
 }  // namespace extensions

extensions/common/manifest.h

@@ -9,6 +9,7 @@
 #include <memory>
 #include <set>
 #include <string>
+#include <vector>
 
 #include "base/macros.h"
 #include "base/strings/string16.h"
@@ -54,19 +55,19 @@ class Manifest {
     NUM_LOCATIONS
   };
 
-  // Do not change the order of entries or remove entries in this list
-  // as this is used in UMA_HISTOGRAM_ENUMERATIONs about extensions.
+  // Do not change the order of entries or remove entries in this list as this
+  // is used in ExtensionType enum in tools/metrics/histograms/enums.xml.
   enum Type {
     TYPE_UNKNOWN = 0,
-    TYPE_EXTENSION,
-    TYPE_THEME,
-    TYPE_USER_SCRIPT,
-    TYPE_HOSTED_APP,
+    TYPE_EXTENSION = 1,
+    TYPE_THEME = 2,
+    TYPE_USER_SCRIPT = 3,
+    TYPE_HOSTED_APP = 4,
     // This is marked legacy because platform apps are preferred. For
     // backwards compatibility, we can't remove support for packaged apps
-    TYPE_LEGACY_PACKAGED_APP,
-    TYPE_PLATFORM_APP,
-    TYPE_SHARED_MODULE,
+    TYPE_LEGACY_PACKAGED_APP = 5,
+    TYPE_PLATFORM_APP = 6,
+    TYPE_SHARED_MODULE = 7,
 
     // New enum values must go above here.
     NUM_LOAD_TYPES

extensions/common/view_type.h

@@ -12,19 +12,26 @@ namespace extensions {
 // Icky RTTI used by a few systems to distinguish the host type of a given
 // WebContents.
 //
+// Do not change the order of entries or remove entries in this list as this
+// is used in ExtensionViewType enum in tools/metrics/histograms/enums.xml.
+//
 // TODO(aa): Remove this and teach those systems to keep track of their own
 // data.
 enum ViewType {
-  VIEW_TYPE_INVALID,
-  VIEW_TYPE_APP_WINDOW,
-  VIEW_TYPE_BACKGROUND_CONTENTS,
-  VIEW_TYPE_COMPONENT,  // For custom parts of Chrome if no other type applies.
-  VIEW_TYPE_EXTENSION_BACKGROUND_PAGE,
-  VIEW_TYPE_EXTENSION_DIALOG,
-  VIEW_TYPE_EXTENSION_GUEST,
-  VIEW_TYPE_EXTENSION_POPUP,
-  VIEW_TYPE_PANEL,
-  VIEW_TYPE_TAB_CONTENTS,
+  VIEW_TYPE_INVALID = 0,
+  VIEW_TYPE_APP_WINDOW = 1,
+  VIEW_TYPE_BACKGROUND_CONTENTS = 2,
+
+  // For custom parts of Chrome if no other type applies.
+  VIEW_TYPE_COMPONENT = 3,
+
+  VIEW_TYPE_EXTENSION_BACKGROUND_PAGE = 4,
+  VIEW_TYPE_EXTENSION_DIALOG = 5,
+  VIEW_TYPE_EXTENSION_GUEST = 6,
+  VIEW_TYPE_EXTENSION_POPUP = 7,
+  VIEW_TYPE_PANEL = 8,
+  VIEW_TYPE_TAB_CONTENTS = 9,
+
   VIEW_TYPE_LAST = VIEW_TYPE_TAB_CONTENTS
 };
 

extensions/renderer/extension_frame_helper.cc

@@ -200,17 +200,43 @@ content::RenderFrame* ExtensionFrameHelper::FindFrame(
   if (!extension)
     return nullptr;
 
-  for (const ExtensionFrameHelper* helper : g_frame_helpers.Get()) {
+  for (const ExtensionFrameHelper* target : g_frame_helpers.Get()) {
+    // Skip frames with a mismatched name.
+    if (target->render_frame()->GetWebFrame()->AssignedName().Utf8() != name)
+      continue;
+
     // Only pierce browsing instance boundaries if the target frame is from the
     // same extension (but not when another extension shares the same renderer
     // process because of reuse trigerred by process limit).
-    // TODO(lukasza): https://crbug.com/764487: Investigate if we can further
-    // restrict scenarios that allow piercing of browsing instance boundaries.
-    if (extension != GetExtensionFromFrame(helper->render_frame()))
+    if (extension != GetExtensionFromFrame(target->render_frame()))
       continue;
 
-    if (helper->render_frame()->GetWebFrame()->AssignedName().Utf8() == name)
-      return helper->render_frame();
+    // TODO(lukasza): https://crbug.com/764487: Investigate if we can further
+    // restrict scenarios that allow piercing of browsing instance boundaries.
+    // We hope that the piercing is only needed if the source or target frames
+    // are for background contents or background page.
+    ViewType target_view_type = target->view_type();
+    ViewType source_view_type =
+        ExtensionFrameHelper::Get(relative_to_frame)->view_type();
+    UMA_HISTOGRAM_ENUMERATION(
+        "Extensions.BrowsingInstanceViolation.ExtensionType",
+        extension->GetType(), Manifest::NUM_LOAD_TYPES);
+    UMA_HISTOGRAM_ENUMERATION(
+        "Extensions.BrowsingInstanceViolation.SourceExtensionViewType",
+        source_view_type, VIEW_TYPE_LAST + 1);
+    UMA_HISTOGRAM_ENUMERATION(
+        "Extensions.BrowsingInstanceViolation.TargetExtensionViewType",
+        target_view_type, VIEW_TYPE_LAST + 1);
+    bool is_background_source_or_target =
+        source_view_type == VIEW_TYPE_EXTENSION_BACKGROUND_PAGE ||
+        source_view_type == VIEW_TYPE_BACKGROUND_CONTENTS ||
+        target_view_type == VIEW_TYPE_EXTENSION_BACKGROUND_PAGE ||
+        target_view_type == VIEW_TYPE_BACKGROUND_CONTENTS;
+    UMA_HISTOGRAM_BOOLEAN(
+        "Extensions.BrowsingInstanceViolation.IsBackgroundSourceOrTarget",
+        is_background_source_or_target);
+
+    return target->render_frame();
   }
 
   return nullptr;

tools/metrics/histograms/enums.xml

@@ -15069,6 +15069,7 @@ Called by update_net_error_codes.py.-->
   <int value="4" label="HOSTED_APP"/>
   <int value="5" label="LEGACY_PACKAGED_APP"/>
   <int value="6" label="PLATFORM_APP"/>
+  <int value="7" label="SHARED_MODULE"/>
 </enum>
 
 <enum name="ExtensionUninstallDialogAction">
@@ -15132,6 +15133,24 @@ Called by update_net_error_codes.py.-->
   <int value="35" label="CRX_EXPECTED_HASH_INVALID"/>
 </enum>
 
+<enum name="ExtensionViewType">
+  <int value="0" label="INVALID"/>
+  <int value="1" label="APP_WINDOW"/>
+  <int value="2" label="BACKGROUND_CONTENTS"/>
+  <int value="3" label="COMPONENT"/>
+  <int value="4" label="EXTENSION_BACKGROUND_PAGE"/>
+  <int value="5" label="EXTENSION_DIALOG"/>
+  <int value="6" label="EXTENSION_GUEST"/>
+  <int value="7" label="EXTENSION_POPUP"/>
+  <int value="8" label="PANEL"/>
+  <int value="9" label="TAB_CONTENTS"/>
+</enum>
+
+<enum name="ExtensionViewTypeIsBackground">
+  <int value="0" label="Not background"/>
+  <int value="1" label="Background"/>
+</enum>
+
 <enum name="ExternalDeviceAction">
   <int value="0" label="Import to Drive"/>
   <int value="1" label="View files"/>

tools/metrics/histograms/histograms.xml

@@ -23094,6 +23094,60 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="Extensions.BrowsingInstanceViolation.ExtensionType"
+    enum="ExtensionType">
+  <owner>lukasza@chromium.org</owner>
+  <summary>
+    When an extension violates browsing instance boundaries, this metric records
+    the extension type.
+
+    This is a temporary metric - probably okay to remove it in M66, after we've
+    gathered sufficient UMA data in M65.  See also http://crbug.com/718489.
+  </summary>
+</histogram>
+
+<histogram
+    name="Extensions.BrowsingInstanceViolation.IsBackgroundSourceOrTarget"
+    enum="ExtensionViewTypeIsBackground">
+  <owner>lukasza@chromium.org</owner>
+  <summary>
+    When an extension violates browsing instance boundaries, this metric records
+    whether either the source or the target frame was of the extension view type
+    of VIEW_TYPE_BACKGROUND_CONTENTS.
+
+    This is a temporary metric - probably okay to remove it in M66, after we've
+    gathered sufficient UMA data in M65.  See also http://crbug.com/718489.
+  </summary>
+</histogram>
+
+<histogram name="Extensions.BrowsingInstanceViolation.SourceExtensionViewType"
+    enum="ExtensionViewType">
+  <owner>lukasza@chromium.org</owner>
+  <summary>
+    When an extension violates browsing instance boundaries, this metric records
+    the extension view type of the source frame.
+
+    This is a temporary metric - probably okay to remove it in M66, after we've
+    gathered sufficient UMA data in M65.  See also http://crbug.com/718489.
+  </summary>
+</histogram>
+
+<histogram name="Extensions.BrowsingInstanceViolation.TargetExtensionViewType"
+    enum="ExtensionViewType">
+  <owner>lukasza@chromium.org</owner>
+  <summary>
+    When an extension violates browsing instance boundaries, this metric records
+    the extension view type of the found frame.
+
+    This metric should help confirm or deny the theory that violating browsing
+    instance boundaries is only needed for looking up background contents /
+    pages (of VIEW_TYPE_BACKGROUND_CONTENTS type).
+
+    This is a temporary metric - probably okay to remove it in M66, after we've
+    gathered sufficient UMA data in M65.  See also http://crbug.com/718489.
+  </summary>
+</histogram>
+
 <histogram name="Extensions.CheckForExternalUpdatesTime" units="ms">
   <owner>rkaplow@chromium.org</owner>
   <summary>