From aaf06239d9378cc5d8683bd13c03680d6e53658a Mon Sep 17 00:00:00 2001 From: Mike Conley Date: Mon, 19 Oct 2020 17:42:51 +0000 Subject: [PATCH] Bug 1661304 - Allow UserInteractions to clobber one another. r=chutten,dthayer Differential Revision: https://phabricator.services.mozilla.com/D93590 --- dom/chrome-webidl/UserInteraction.webidl | 5 +- .../components/telemetry/core/Stopwatch.cpp | 14 +++-- .../docs/collection/user-interactions.rst | 3 + .../tests/unit/test_UserInteraction.js | 22 ++++---- .../unit/test_UserInteraction_annotations.js | 55 +++++++++++++++++++ 5 files changed, 82 insertions(+), 17 deletions(-) diff --git a/dom/chrome-webidl/UserInteraction.webidl b/dom/chrome-webidl/UserInteraction.webidl index fb37b2066ceb..ccde57dcacdd 100644 --- a/dom/chrome-webidl/UserInteraction.webidl +++ b/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, diff --git a/toolkit/components/telemetry/core/Stopwatch.cpp b/toolkit/components/telemetry/core/Stopwatch.cpp index 8649dd40e954..08bffbd72453 100644 --- a/toolkit/components/telemetry/core/Stopwatch.cpp +++ b/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; } diff --git a/toolkit/components/telemetry/docs/collection/user-interactions.rst b/toolkit/components/telemetry/docs/collection/user-interactions.rst index 8f7006ef8854..ecbf3683fa95 100644 --- a/toolkit/components/telemetry/docs/collection/user-interactions.rst +++ b/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. diff --git a/toolkit/components/telemetry/tests/unit/test_UserInteraction.js b/toolkit/components/telemetry/tests/unit/test_UserInteraction.js index 60539bf2085b..5fc3c5ecd127 100644 --- a/toolkit/components/telemetry/tests/unit/test_UserInteraction.js +++ b/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)); diff --git a/toolkit/components/telemetry/tests/unit/test_UserInteraction_annotations.js b/toolkit/components/telemetry/tests/unit/test_UserInteraction_annotations.js index 52c67f7abe12..d89e87684409 100644 --- a/toolkit/components/telemetry/tests/unit/test_UserInteraction_annotations.js +++ b/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." + ); +});