Kotlin: Don't inline the closure when measuring

This can change control flow, as the code is essentially copied 1:1 in
place, which means `return` inside will be an early return before our
measure wrapper gets to stop the timer.

Thanks to @mcomella for the detailed explanation in the bug report.
See: https://blog.mindorks.com/understanding-inline-noinline-and-crossinline-in-kotlin

.dictionary

@@ -1,4 +1,4 @@
-personal_ws-1.1 en 215 utf-8
+personal_ws-1.1 en 216 utf-8
 AAR
 AARs
 ABI
@@ -140,6 +140,7 @@ hotfix
 html
 illumos
 init
+inlined
 integrations
 io
 ios

CHANGELOG.md

@@ -2,6 +2,11 @@
 
 [Full changelog](https://github.com/mozilla/glean/compare/v36.0.0...main)
 
+* Android
+  * BUGFIX: `TimespanMetricType.measure` and `TimingDistributionMetricType.measure` won't get inlined anymore ([#1560](https://github.com/mozilla/glean/pull/1560)).
+    This avoids a potential bug where a `return` used inside the closure would end up not measuring the time.
+    Use `return@measure <val>` for early returns.
+
 # v36.0.0 (2021-03-16)
 
 [Full changelog](https://github.com/mozilla/glean/compare/v35.0.0...v36.0.0)

glean-core/android/src/main/java/mozilla/telemetry/glean/private/TimespanMetricType.kt

@@ -94,7 +94,7 @@ class TimespanMetricType internal constructor(
      * If the measured function throws, the measurement is canceled and the exception rethrown.
      */
     @Suppress("TooGenericExceptionCaught")
-    inline fun <U> measure(funcToMeasure: () -> U): U {
+    fun <U> measure(funcToMeasure: () -> U): U {
         start()
 
         val returnValue = try {

glean-core/android/src/main/java/mozilla/telemetry/glean/private/TimingDistributionMetricType.kt

@@ -113,7 +113,7 @@ class TimingDistributionMetricType internal constructor(
      * If the measured function throws, the measurement is canceled and the exception rethrown.
      */
     @Suppress("TooGenericExceptionCaught")
-    inline fun <U> measure(funcToMeasure: () -> U): U {
+    fun <U> measure(funcToMeasure: () -> U): U {
         val timerId = start()
 
         val returnValue = try {

glean-core/android/src/test/java/mozilla/telemetry/glean/private/TimespanMetricTypeTest.kt

@@ -288,6 +288,36 @@ class TimespanMetricTypeTest {
         assertTrue("Metric value must be greater than zero", metric.testGetValue() >= 0)
     }
 
+    @Test
+    fun `measure function does not change behavior with early return`() {
+        val metric = TimespanMetricType(
+            disabled = false,
+            category = "telemetry",
+            lifetime = Lifetime.Ping,
+            name = "inlined",
+            sendInPings = listOf("store1"),
+            timeUnit = TimeUnit.Nanosecond
+        )
+
+        // We define a function that measures the whole function call runtime
+        fun testFunc(): Long = metric.measure {
+            // We want to simulate an early return.
+            if (true) {
+                // Blank 'return' is not allowed here, because `measure` is not inlined.
+                // We can return by label though.
+                return@measure 17
+            }
+
+            42
+        }
+
+        val res = testFunc()
+        assertEquals("Test value must match", 17, res)
+
+        assertTrue("Metric must have a value", metric.testHasValue())
+        assertTrue("Metric value must be greater than zero", metric.testGetValue() >= 0)
+    }
+
     @Test
     fun `measure function bubbles up exceptions and timing is canceled`() {
         // Define a timespan metric, which will be stored in "store1"

glean-core/android/src/test/java/mozilla/telemetry/glean/private/TimingDistributionMetricTypeTest.kt

@@ -280,6 +280,44 @@ class TimingDistributionMetricTypeTest {
         assertEquals(1L, snapshot.values[3])
     }
 
+    @Test
+    fun `measure function does not change behavior with early return`() {
+        val metric = spy(TimingDistributionMetricType(
+            disabled = false,
+            category = "telemetry",
+            lifetime = Lifetime.Ping,
+            name = "inlined",
+            sendInPings = listOf("store1"),
+            timeUnit = TimeUnit.Nanosecond
+        ))
+
+        // We define a function that measures the whole function call runtime
+        fun testFunc(): Long = metric.measure {
+            // Stop should call `getElapsedTimeNanos` again,
+            // so we give it a later timestamp
+            `when`(metric.getElapsedTimeNanos()).thenReturn(10L)
+
+            // We want to simulate an early return.
+            if (true) {
+                // Blank 'return' is not allowed here, because `measure` is not inlined.
+                // We can return by label though.
+                return@measure 17
+            }
+
+            42
+        }
+
+        // We start time at `0`
+        `when`(metric.getElapsedTimeNanos()).thenReturn(0L)
+
+        val res = testFunc()
+        assertEquals("Test value must match", 17, res)
+
+        assertTrue("Metric must have a value", metric.testHasValue())
+        val snapshot = metric.testGetValue()
+        assertEquals("Should have stored 10 nanoseconds", 10L, snapshot.sum)
+    }
+
     @Test
     fun `measure function bubbles up exceptions and timing is canceled`() {
         val metric = TimingDistributionMetricType(