Surface synchronization: Convert invariants violations to UMA + optional dump

As per anicolao@'s advice, this CL converts surface invariants violations dumps
into an optional feature whereas we always report UMA. This allows us to ship
with invariants violations that do not impact the user experience.

Bug: 814690, 672962
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I206bc32d30afcccc8dd0e0200601961c72943a1f
Reviewed-on: https://chromium-review.googlesource.com/962929Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543187}

components/viz/common/features.cc

@@ -23,6 +23,10 @@ const base::Feature kEnableSurfaceSynchronization{
     "SurfaceSynchronization", base::FEATURE_DISABLED_BY_DEFAULT};
 #endif
 
+// Enables DumpWithoutCrashing of surface invariants violations.
+const base::Feature kEnableInvariantsViolationLogging{
+    "InvariantsViolationLogging", base::FEATURE_DISABLED_BY_DEFAULT};
+
 // Enables running the display compositor as part of the viz service in the GPU
 // process. This is also referred to as out-of-process display compositor
 // (OOP-D).
@@ -40,6 +44,11 @@ bool IsSurfaceSynchronizationEnabled() {
          base::FeatureList::IsEnabled(kVizDisplayCompositor);
 }
 
+bool IsSurfaceInvariantsViolationLoggingEnabled() {
+  return IsSurfaceSynchronizationEnabled() &&
+         base::FeatureList::IsEnabled(kEnableInvariantsViolationLogging);
+}
+
 bool IsVizHitTestingDrawQuadEnabled() {
   return base::FeatureList::IsEnabled(kEnableVizHitTestDrawQuad) ||
          base::FeatureList::IsEnabled(kVizDisplayCompositor);

components/viz/common/features.h

@@ -13,11 +13,13 @@ namespace features {
 
 VIZ_COMMON_EXPORT extern const base::Feature kEnableDrawOcclusion;
 VIZ_COMMON_EXPORT extern const base::Feature kEnableSurfaceSynchronization;
+VIZ_COMMON_EXPORT extern const base::Feature kEnableInvariantsViolationLogging;
 VIZ_COMMON_EXPORT extern const base::Feature kEnableVizHitTestDrawQuad;
 VIZ_COMMON_EXPORT extern const base::Feature kVizDisplayCompositor;
 
 VIZ_COMMON_EXPORT bool IsDrawOcclusionEnabled();
 VIZ_COMMON_EXPORT bool IsSurfaceSynchronizationEnabled();
+VIZ_COMMON_EXPORT bool IsSurfaceInvariantsViolationLoggingEnabled();
 VIZ_COMMON_EXPORT bool IsVizHitTestingDrawQuadEnabled();
 VIZ_COMMON_EXPORT bool IsVizHitTestingEnabled();
 VIZ_COMMON_EXPORT bool IsVizHitTestingSurfaceLayerEnabled();

content/browser/renderer_host/render_widget_host_impl.cc

@@ -2778,10 +2778,17 @@ void RenderWidgetHostImpl::SubmitCompositorFrame(
         new_surface_properties.ToDiffString(last_surface_properties_).c_str());
     LOG(ERROR) << "Surface invariants violation: " << error;
 
-    static auto* crash_key = base::debug::AllocateCrashKeyString(
-        "surface-invariants-violation", base::debug::CrashKeySize::Size256);
-    base::debug::ScopedCrashKeyString key_value(crash_key, error);
-    base::debug::DumpWithoutCrashing();
+    static int invariants_violation_count = 0;
+    ++invariants_violation_count;
+    UMA_HISTOGRAM_COUNTS_1000("Compositing.SurfaceInvariantsViolations",
+                              invariants_violation_count);
+
+    if (features::IsSurfaceInvariantsViolationLoggingEnabled()) {
+      static auto* crash_key = base::debug::AllocateCrashKeyString(
+          "surface-invariants-violation", base::debug::CrashKeySize::Size256);
+      base::debug::ScopedCrashKeyString key_value(crash_key, error);
+      base::debug::DumpWithoutCrashing();
+    }
 
     if (view_) {
       frame.metadata.begin_frame_ack.has_damage = false;

tools/metrics/histograms/histograms.xml

@@ -11551,6 +11551,16 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="Compositing.SurfaceInvariantsViolations" units="violations">
+  <owner>fsamuel@chromium.org</owner>
+  <summary>
+    The number of times a renderer in this browser session has violated a
+    surface synchronization invariant. This manifests as a skipped frame.
+    Ideally this metric should always report 0, but realistically there are
+    races in the system and this tracks the frequency that we hit these races.
+  </summary>
+</histogram>
+
 <histogram name="Compositing.SurfaceManager.NumOldTemporaryReferences">
   <obsolete>
     Deprecated 2018/01/24.