Bug 1661304 - Allow UserInteractions to clobber one another. r=chutten,dthayer

Differential Revision: https://phabricator.services.mozilla.com/D93590

dom/chrome-webidl/UserInteraction.webidl

@@ -27,8 +27,9 @@ namespace UserInteraction {
    * associated with different objects.
    *
    * @returns True if the timer was successfully started, false otherwise.
-   * If a timer already exists, it can't be started again, and the existing
-   * one will be cleared in order to avoid measurement errors.
+   * If a timer already exists, it will be overwritten, and the new timer
+   * will include a "(clobbered)" suffix in any BHR annotations that get
+   * created.
    */
   boolean start(DOMString id,
                 UTF8String value,

toolkit/components/telemetry/core/Stopwatch.cpp

@@ -426,13 +426,19 @@ bool Timers::StartUserInteraction(JSContext* aCx,
                      NS_ConvertUTF16toUTF8(aUserInteraction).get()));
       }
       Delete(aCx, aUserInteraction, aObj, VoidString());
+      timer = Get(aCx, aUserInteraction, aObj, VoidString());
+
+      nsAutoString clobberText(aUserInteraction);
+      clobberText.AppendLiteral(u" (clobbered)");
+      timer->SetBHRAnnotation(clobberText, aValue);
     } else {
       timer->SetBHRAnnotation(aUserInteraction, aValue);
-      mBHRAnnotationTimers.insertBack(timer);
-
-      timer->Start(false);
-      return true;
     }
+
+    mBHRAnnotationTimers.insertBack(timer);
+
+    timer->Start(false);
+    return true;
   }
   return false;
 }

toolkit/components/telemetry/docs/collection/user-interactions.rst

@@ -164,6 +164,9 @@ This API is main-thread only, and all functions will return `false` if accessed
 Starts recording a User Interaction.
 Any hangs that occur on the main thread while recording this User Interaction result in an annotation being added to the background hang report.
 
+If a pre-existing UserInteraction already exists with the same ``id`` and the same ``object``, that pre-existing UserInteraction will be overwritten.
+The newly created UserInteraction will include a "(clobbered)" suffix on its BHR annotation name.
+
 * ``id``: Required. A string value, limited to 80 characters. This is the category name concatenated with the User Interaction name.
 * ``value``: Required. A string value, limited to 50 characters.
 * ``object``: Optional. If specified, the User Interaction is associated with this object, so multiple recordings can be done concurrently.

toolkit/components/telemetry/tests/unit/test_UserInteraction.js

@@ -25,24 +25,24 @@ function run_test() {
   Assert.ok(UserInteraction.running(TEST_USER_INTERACTION_ID, obj1));
   Assert.ok(UserInteraction.running(TEST_USER_INTERACTION_ID, obj2));
 
-  // Same UserInteraction can't be re-started before being stopped
-  Assert.ok(!UserInteraction.start(TEST_USER_INTERACTION_ID, TEST_VALUE_1));
+  // Unlike TelemetryStopwatch, we can clobber UserInteractions.
+  Assert.ok(UserInteraction.start(TEST_USER_INTERACTION_ID, TEST_VALUE_1));
   Assert.ok(
-    !UserInteraction.start(TEST_USER_INTERACTION_ID, TEST_VALUE_1, obj1)
+    UserInteraction.start(TEST_USER_INTERACTION_ID, TEST_VALUE_1, obj1)
   );
   Assert.ok(
-    !UserInteraction.start(TEST_USER_INTERACTION_ID, TEST_VALUE_1, obj2)
+    UserInteraction.start(TEST_USER_INTERACTION_ID, TEST_VALUE_1, obj2)
   );
 
-  Assert.ok(!UserInteraction.running(TEST_USER_INTERACTION_ID));
-  Assert.ok(!UserInteraction.running(TEST_USER_INTERACTION_ID, obj1));
-  Assert.ok(!UserInteraction.running(TEST_USER_INTERACTION_ID, obj2));
+  Assert.ok(UserInteraction.running(TEST_USER_INTERACTION_ID));
+  Assert.ok(UserInteraction.running(TEST_USER_INTERACTION_ID, obj1));
+  Assert.ok(UserInteraction.running(TEST_USER_INTERACTION_ID, obj2));
 
-  // Ensure that we can't finish a UserInteraction that was accidentally started
+  // Ensure that we can finish a UserInteraction that was accidentally started
   // twice
-  Assert.ok(!UserInteraction.finish(TEST_USER_INTERACTION_ID));
-  Assert.ok(!UserInteraction.finish(TEST_USER_INTERACTION_ID, obj1));
-  Assert.ok(!UserInteraction.finish(TEST_USER_INTERACTION_ID, obj2));
+  Assert.ok(UserInteraction.finish(TEST_USER_INTERACTION_ID));
+  Assert.ok(UserInteraction.finish(TEST_USER_INTERACTION_ID, obj1));
+  Assert.ok(UserInteraction.finish(TEST_USER_INTERACTION_ID, obj2));
 
   // Make sure we can't start or finish non-existent UserInteractions.
   Assert.ok(!UserInteraction.start("non-existent.interaction", TEST_VALUE_1));

toolkit/components/telemetry/tests/unit/test_UserInteraction_annotations.js

@@ -9,6 +9,7 @@ const { TestUtils } = ChromeUtils.import(
 
 const HANG_TIME = 500; // ms
 const TEST_USER_INTERACTION_ID = "testing.interaction";
+const TEST_CLOBBERED_USER_INTERACTION_ID = `${TEST_USER_INTERACTION_ID} (clobbered)`;
 const TEST_VALUE_1 = "some value";
 const TEST_VALUE_2 = "some other value";
 const TEST_ADDITIONAL_TEXT_1 = "some additional text";
@@ -115,6 +116,29 @@ function hasHangAnnotation(report, value) {
   });
 }
 
+/**
+ * Given an nsIHangReport, returns true if there are one or more annotations
+ * with the TEST_CLOBBERED_USER_INTERACTION_ID name, and the passed value.
+ *
+ * This check should be used when we expect a pre-existing UserInteraction to
+ * have been clobbered by a new UserInteraction.
+ *
+ * @param {nsIHangReport} report
+ *   The hang report to check the annotations of.
+ * @param {String} value
+ *   The value that the annotation should have.
+ * @returns {boolean}
+ *   True if the annotation was found.
+ */
+function hasClobberedHangAnnotation(report, value) {
+  return report.annotations.some(annotation => {
+    return (
+      annotation[0] == TEST_CLOBBERED_USER_INTERACTION_ID &&
+      annotation[1] == value
+    );
+  });
+}
+
 /**
  * Tests that UserInteractions cause BHR annotations and profiler
  * markers to be written.
@@ -406,3 +430,34 @@ add_task(async function test_cancelling_annotations_and_markers() {
     "Should not have the second BHR annotation set."
   );
 });
+
+/**
+ * Tests that starting UserInteractions with the same ID and object
+ * creates a clobber annotation.
+ */
+add_task(async function test_clobbered_annotations() {
+  if (!Services.telemetry.canRecordExtended) {
+    Assert.ok("Hang reporting not enabled.");
+    return;
+  }
+
+  UserInteraction.start(TEST_USER_INTERACTION_ID, TEST_VALUE_1);
+  // Now clobber the original UserInteraction
+  UserInteraction.start(TEST_USER_INTERACTION_ID, TEST_VALUE_2);
+
+  let report = await hangAndWaitForReport();
+  Assert.ok(
+    UserInteraction.finish(TEST_USER_INTERACTION_ID),
+    "Should have been able to finish the UserInteraction."
+  );
+
+  Assert.ok(
+    !hasHangAnnotation(report, TEST_VALUE_1),
+    "Should not have the original BHR annotation set."
+  );
+
+  Assert.ok(
+    hasClobberedHangAnnotation(report, TEST_VALUE_2),
+    "Should have the clobber BHR annotation set."
+  );
+});