Merge pull request #1823 from mozilla/fenix-crashes

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 [Full changelog](https://github.com/mozilla/glean/compare/v42.0.0...main)
 
+* General
+  * BUGFIX: Avoid a crash when accessing labeled metrics by caching created objects ([#1823](https://github.com/mozilla/glean/pull/1823)).
+
 # v42.0.0 (2021-10-06)
 
 [Full changelog](https://github.com/mozilla/glean/compare/v41.1.1...v42.0.0)
@@ -78,8 +81,8 @@
 [Full changelog](https://github.com/mozilla/glean/compare/v39.1.0...v40.0.0)
 
 * Android
-  * **Breaking Change**: Split the Glean Kotlin SDK into two packages: `glean` and `glean-native` ([#1595](https://github.com/mozilla/glean/pull/1595)).  
-    Consumers will need to switch to `org.mozilla.telemetry:glean-native-forUnitTests`.  
+  * **Breaking Change**: Split the Glean Kotlin SDK into two packages: `glean` and `glean-native` ([#1595](https://github.com/mozilla/glean/pull/1595)).
+    Consumers will need to switch to `org.mozilla.telemetry:glean-native-forUnitTests`.
     Old code in `build.gradle`:
 
     ```

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

@@ -456,4 +456,39 @@ class LabeledMetricTypeTest {
         assertEquals(1, labeledCounterMetric["bar"].testGetValue())
         assertEquals(100, labeledCounterMetric["__other__"].testGetValue())
     }
+
+    @Test
+    fun `rapidly re-creating labeled metrics does not crash`() {
+        // Regression test for bug 1733757.
+        // The underlying map implementation has an upper limit of entries it can handle,
+        // currently set to (1<<15)-1 = 32767.
+        // We used to create a new object every time a label was referenced,
+        // leading to exhausting the available storage in that map, which finally results in a panic.
+
+        val counterMetric = CounterMetricType(
+            disabled = false,
+            category = "telemetry",
+            lifetime = Lifetime.Application,
+            name = "labeled_nocrash_counter",
+            sendInPings = listOf("metrics")
+        )
+
+        val labeledCounterMetric = LabeledMetricType(
+            disabled = false,
+            category = "telemetry",
+            lifetime = Lifetime.Application,
+            name = "labeled_nocrash",
+            sendInPings = listOf("metrics"),
+            subMetric = counterMetric,
+            labels = setOf("foo")
+        )
+
+        // We go higher than the maximum of `(1<<15)-1 = 32767`.
+        val maxAttempts = 1 shl 16
+        for (ignored in 1..maxAttempts) {
+            labeledCounterMetric["foo"].add(1)
+        }
+
+        assertEquals(maxAttempts, labeledCounterMetric["foo"].testGetValue())
+    }
 }

glean-core/ffi/src/labeled.rs

@@ -74,10 +74,38 @@ macro_rules! impl_labeled_metric {
         /// Create a new instance of the sub-metric of this labeled metric.
         #[no_mangle]
         pub extern "C" fn $get_name(handle: u64, label: FfiStr) -> u64 {
-            $global.call_infallible_mut(handle, |labeled| {
-                let metric = labeled.get(label.as_str());
-                $metric_global.insert_with_log(|| Ok(metric))
-            })
+            // A map from a unique ID for the labeled submetric to a handle of an instantiated
+            // metric type.
+            static LABEL_MAP: once_cell::sync::Lazy<
+                std::sync::Mutex<std::collections::HashMap<String, u64>>,
+            > = once_cell::sync::Lazy::new(|| {
+                std::sync::Mutex::new(std::collections::HashMap::new())
+            });
+
+            // The handle is a unique number per metric.
+            // The label identifies the submetric.
+            let id = format!("{}/{}", handle, label.as_str());
+
+            let mut map = LABEL_MAP.lock().unwrap();
+
+            use std::collections::hash_map::Entry;
+            match map.entry(id) {
+                Entry::Occupied(entry) => {
+                    // This label handle _could_ get stale if language SDKs call the corresponding
+                    // `glean_destroy_*` functions on the sub-metric.
+                    // We don't guard against this for now, but ensure our language SDKs don't call
+                    // it for metric types that can be labeled (counter, string, boolean).
+                    *entry.get()
+                }
+                Entry::Vacant(entry) => {
+                    let label_handle = $global.call_infallible_mut(handle, |labeled| {
+                        let metric = labeled.get(label.as_str());
+                        $metric_global.insert_with_log(|| Ok(metric))
+                    });
+                    entry.insert(label_handle);
+                    label_handle
+                }
+            }
         }
 
         #[no_mangle]

glean-core/ios/Glean/Metrics/BooleanMetric.swift

@@ -39,13 +39,6 @@ public class BooleanMetricType {
         self.sendInPings = sendInPings
     }
 
-    /// Destroy this metric.
-    deinit {
-        if self.handle != 0 {
-            glean_destroy_boolean_metric(self.handle)
-        }
-    }
-
     /// Set a boolean value.
     ///
     /// - parameters:

glean-core/ios/Glean/Metrics/CounterMetric.swift

@@ -39,13 +39,6 @@ public class CounterMetricType {
         self.sendInPings = sendInPings
     }
 
-    /// Destroy this metric.
-    deinit {
-        if self.handle != 0 {
-            glean_destroy_counter_metric(self.handle)
-        }
-    }
-
     /// Add to counter value.
     ///
     /// - parameters:

glean-core/ios/Glean/Metrics/StringMetric.swift

@@ -39,13 +39,6 @@ public class StringMetricType {
         self.sendInPings = sendInPings
     }
 
-    /// Destroy this metric.
-    deinit {
-        if self.handle != 0 {
-            glean_destroy_string_metric(self.handle)
-        }
-    }
-
     /// Set a string value.
     ///
     /// - parameters:

glean-core/ios/GleanTests/Metrics/LabeledMetricTests.swift

@@ -149,7 +149,7 @@ class LabeledMetricTypeTests: XCTestCase {
     }
 
     func testLabeledStringType() {
-        let counterMetric = StringMetricType(
+        let stringMetric = StringMetricType(
             category: "telemetry",
             name: "labeled_counter_metric",
             sendInPings: ["metrics"],
@@ -163,7 +163,7 @@ class LabeledMetricTypeTests: XCTestCase {
             sendInPings: ["metrics"],
             lifetime: .application,
             disabled: false,
-            subMetric: counterMetric
+            subMetric: stringMetric
         )
 
         labeledStringMetric["label1"].set("foo")
@@ -218,4 +218,38 @@ class LabeledMetricTypeTests: XCTestCase {
             XCTAssertEqual(error as! String, "Can not create a labeled version of this metric type")
         }
     }
+
+    func testRapidlyRecreatingLabeledMetricsDoesNotCrash() {
+        // Regression test for bug 1733757.
+        // The underlying map implementation has an upper limit of entries it can handle,
+        // currently set to (1<<15)-1 = 32767.
+        // We used to create a new object every time a label was referenced,
+        // leading to exhausting the available storage in that map, which finally results in a panic.
+
+        let counterMetric = CounterMetricType(
+            category: "telemetry",
+            name: "labeled_nocrash_counter",
+            sendInPings: ["metrics"],
+            lifetime: .application,
+            disabled: false
+        )
+
+        let labeledCounterMetric = try! LabeledMetricType<CounterMetricType>(
+            category: "telemetry",
+            name: "labeled_nocrash",
+            sendInPings: ["metrics"],
+            lifetime: .application,
+            disabled: false,
+            subMetric: counterMetric,
+            labels: ["foo"]
+        )
+
+        // We go higher than the maximum of `(1<<15)-1 = 32767`.
+        let maxAttempts = Int32(1 << 16)
+        for _ in 1 ... maxAttempts {
+            labeledCounterMetric["foo"].add(1)
+        }
+
+        XCTAssertEqual(maxAttempts, try labeledCounterMetric["foo"].testGetValue())
+    }
 }

glean-core/python/glean/metrics/boolean.py

@@ -44,10 +44,6 @@ def __init__(
             disabled,
         )
 
-    def __del__(self):
-        if getattr(self, "_handle", 0) != 0:
-            _ffi.lib.glean_destroy_boolean_metric(self._handle)
-
     def set(self, value: bool) -> None:
         """
         Set a boolean value.

glean-core/python/glean/metrics/counter.py

@@ -47,10 +47,6 @@ def __init__(
             disabled,
         )
 
-    def __del__(self):
-        if getattr(self, "_handle", 0) != 0:
-            _ffi.lib.glean_destroy_counter_metric(self._handle)
-
     def add(self, amount: int = 1) -> None:
         """
         Add to counter value.

glean-core/python/glean/metrics/event.py

@@ -127,7 +127,9 @@ def __del__(self):
         if getattr(self, "_handle", 0) != 0:
             _ffi.lib.glean_destroy_event_metric(self._handle)
 
-    def record(self, extra: Optional[Union[Dict[int, str], EventExtras]] = None) -> None:
+    def record(
+        self, extra: Optional[Union[Dict[int, str], EventExtras]] = None
+    ) -> None:
         """
         Record an event by using the information provided by the instance of
         this class.

glean-core/python/glean/metrics/string.py

@@ -47,10 +47,6 @@ def __init__(
             disabled,
         )
 
-    def __del__(self):
-        if getattr(self, "_handle", 0) != 0:
-            _ffi.lib.glean_destroy_string_metric(self._handle)
-
     def set(self, value: str) -> None:
         """
         Set a string value.

glean-core/python/tests/metrics/test_event.py

@@ -318,7 +318,8 @@ def test_event_enum_is_generated_correctly():
 
 def test_event_extra_is_generated_correctly():
     metrics = load_metrics(
-        ROOT.parent / "data" / "events_with_types.yaml", config={"allow_reserved": False}
+        ROOT.parent / "data" / "events_with_types.yaml",
+        config={"allow_reserved": False},
     )
 
     metrics.core.preference_toggled.record(
@@ -333,7 +334,9 @@ def test_event_extra_is_generated_correctly():
 
 def test_the_convenient_extrakeys_api():
     class ClickKeys(metrics.EventExtras):
-        def __init__(self, object_id: Optional[str] = None, other: Optional[str] = None) -> None:
+        def __init__(
+            self, object_id: Optional[str] = None, other: Optional[str] = None
+        ) -> None:
             self._object_id = object_id
             self._other = other
 
@@ -390,15 +393,18 @@ def to_ffi_extra(self) -> Tuple[List[int], List[str]]:
 
 def test_event_extra_does_typechecks():
     metrics = load_metrics(
-        ROOT.parent / "data" / "events_with_types.yaml", config={"allow_reserved": False}
+        ROOT.parent / "data" / "events_with_types.yaml",
+        config={"allow_reserved": False},
     )
 
     # Valid combinations of extras.
     # These do not throw.
     metrics.core.PreferenceToggledExtra(preference="value1")
     metrics.core.PreferenceToggledExtra(enabled=True)
     metrics.core.PreferenceToggledExtra(swapped=1)
-    extras = metrics.core.PreferenceToggledExtra(preference="value1", enabled=True, swapped=1)
+    extras = metrics.core.PreferenceToggledExtra(
+        preference="value1", enabled=True, swapped=1
+    )
     # Check conversion to FFI types, extras are sorted by name
     ffi = extras.to_ffi_extra()
     expected = ([0, 1, 2], ["true", "value1", "1"])

glean-core/python/tests/metrics/test_labeled.py

@@ -251,3 +251,30 @@ def test_invalid_labels_go_to_other():
     assert 4 == labeled_counter_metric.test_get_num_recorded_errors(
         ErrorType.INVALID_LABEL
     )
+
+
+def test_rapidly_recreating_labeled_metrics_does_not_crash():
+    """
+    Regression test for bug 1733757.
+    The underlying map implementation has an upper limit of entries it can handle,
+    currently set to (1<<15)-1 = 32767.
+    We used to create a new object every time a label was referenced,
+    leading to exhausting the available storage in that map, which finally results in a panic.
+    """
+
+    labeled_counter_metric = metrics.LabeledCounterMetricType(
+        category="telemetry",
+        name="labeled_nocrash",
+        send_in_pings=["metrics"],
+        lifetime=Lifetime.APPLICATION,
+        disabled=False,
+        labels=["foo"],
+    )
+
+    # We go higher than the maximum of `(1<<15)-1 = 32767`.
+    # Python is slow, so we only go a tiny bit higher.
+    max_attempts = (1 << 15) + 1  # 32769
+    for _ in range(max_attempts):
+        labeled_counter_metric["foo"].add(1)
+
+    assert max_attempts == labeled_counter_metric["foo"].test_get_value()