Bug 1708668 - Enable Glean testing in xpcshell tests. r=chutten

This ensures Glean is initialized in an xpcshell environment on Android.
When GeckoView is embedded the surrounding app is responsible to
initialize Glean from the Kotlin side.

This also enables the first few FOG tests to run on Android.
For now it changes the Android-specific test file.
As long as not all metric types are enabled we can't reliably test other metrics.

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

testing/xpcshell/head.js

@@ -528,6 +528,12 @@ function _execute_test() {
       );
       geckoViewStartup.observe(null, "profile-after-change", null);
       geckoViewStartup.observe(null, "app-startup", null);
+
+      // Glean needs to be initialized for metric recording & tests to work.
+      // Usually this happens through Glean Kotlin,
+      // but for xpcshell tests we initialize it from here.
+      const FOG = Cc["@mozilla.org/toolkit/glean;1"].createInstance(Ci.nsIFOG);
+      FOG.initializeFOG();
     } catch (ex) {
       do_throw(`Failed to initialize GeckoView: ${ex}`, ex.stack);
     }

toolkit/components/glean/src/android_init.rs

@@ -1,23 +0,0 @@
-// This Source Code Form is subject to the terms of the Mozilla Public
-// License, v. 2.0. If a copy of the MPL was not distributed with this
-// file, You can obtain one at https://mozilla.org/MPL/2.0/.
-
-use nserror::{nsresult, NS_OK};
-use nsstring::nsACString;
-
-// Needed for re-export.
-pub use glean_ffi;
-
-/// On Android initialize the bare minimum of FOG.
-///
-/// This does not initialize Glean, which should be handled by the bundling application.
-#[no_mangle]
-pub unsafe extern "C" fn fog_init(
-    _data_path_override: &nsACString,
-    _app_id_override: &nsACString,
-) -> nsresult {
-    // Register all custom pings before we initialize.
-    fog::pings::register_pings();
-
-    NS_OK
-}

toolkit/components/glean/src/init/mod.rs

@@ -7,48 +7,96 @@ use std::ffi::CString;
 use std::ops::DerefMut;
 use std::path::PathBuf;
 
-use nserror::{nsresult, NS_ERROR_FAILURE, NS_OK};
+use nserror::{nsresult, NS_ERROR_FAILURE};
 use nsstring::{nsACString, nsCString, nsString};
-use xpcom::interfaces::{mozIViaduct, nsIFile, nsIXULAppInfo};
+#[cfg(not(target_os = "android"))]
+use xpcom::interfaces::mozIViaduct;
+use xpcom::interfaces::{nsIFile, nsIXULAppInfo};
 use xpcom::XpCom;
 
 use glean::{ClientInfoMetrics, Configuration};
 
+#[cfg(not(target_os = "android"))]
 mod upload_pref;
+#[cfg(not(target_os = "android"))]
 mod user_activity;
+#[cfg(not(target_os = "android"))]
 mod viaduct_uploader;
 
+#[cfg(not(target_os = "android"))]
 use upload_pref::UploadPrefObserver;
+#[cfg(not(target_os = "android"))]
 use user_activity::UserActivityObserver;
+#[cfg(not(target_os = "android"))]
 use viaduct_uploader::ViaductUploader;
 
 /// Project FOG's entry point.
 ///
 /// This assembles client information and the Glean configuration and then initializes the global
 /// Glean instance.
+#[cfg(not(target_os = "android"))]
 #[no_mangle]
-pub unsafe extern "C" fn fog_init(
+pub extern "C" fn fog_init(
     data_path_override: &nsACString,
     app_id_override: &nsACString,
 ) -> nsresult {
+    let upload_enabled = static_prefs::pref!("datareporting.healthreport.uploadEnabled");
+    let uploader = Some(Box::new(ViaductUploader) as Box<dyn glean::net::PingUploader>);
+
+    fog_init_internal(
+        data_path_override,
+        app_id_override,
+        upload_enabled,
+        uploader,
+    )
+    .into()
+}
+
+/// Project FOG's entry point on Android.
+///
+/// This assembles client information and the Glean configuration and then initializes the global
+/// Glean instance.
+/// It always enables upload and set no uploader.
+/// This should only be called in test scenarios.
+/// In normal use Glean should be initialized and controlled by the Glean Kotlin SDK.
+#[cfg(target_os = "android")]
+#[no_mangle]
+pub extern "C" fn fog_init(
+    data_path_override: &nsACString,
+    app_id_override: &nsACString,
+) -> nsresult {
+    // On Android always enable Glean upload.
+    let upload_enabled = true;
+    // Don't set up an uploader.
+    let uploader = None;
+
+    fog_init_internal(
+        data_path_override,
+        app_id_override,
+        upload_enabled,
+        uploader,
+    )
+    .into()
+}
+
+fn fog_init_internal(
+    data_path_override: &nsACString,
+    app_id_override: &nsACString,
+    upload_enabled: bool,
+    uploader: Option<Box<dyn glean::net::PingUploader>>,
+) -> Result<(), nsresult> {
     fog::metrics::fog::initialization.start();
 
     log::debug!("Initializing FOG.");
 
     let data_path_str = if data_path_override.is_empty() {
-        match get_data_path() {
-            Ok(dp) => dp,
-            Err(e) => return e,
-        }
+        get_data_path()?
     } else {
         data_path_override.to_utf8().to_string()
     };
     let data_path = PathBuf::from(&data_path_str);
 
-    let (app_build, app_display_version, channel) = match get_app_info() {
-        Ok(ai) => ai,
-        Err(e) => return e,
-    };
+    let (app_build, app_display_version, channel) = get_app_info()?;
 
     let client_info = ClientInfoMetrics {
         app_build,
@@ -56,21 +104,7 @@ pub unsafe extern "C" fn fog_init(
     };
     log::debug!("Client Info: {:#?}", client_info);
 
-    if let Err(e) = UploadPrefObserver::begin_observing() {
-        log::error!(
-            "Could not observe data upload pref. Abandoning FOG init due to {:?}",
-            e
-        );
-        return e;
-    }
-
-    if let Err(e) = UserActivityObserver::begin_observing() {
-        log::error!(
-            "Could not observe user activity. Abandoning FOG init due to {:?}",
-            e
-        );
-        return e;
-    }
+    setup_observers()?;
 
     const SERVER: &str = "https://incoming.telemetry.mozilla.org";
     let localhost_port = static_prefs::pref!("telemetry.fog.test.localhost_port");
@@ -88,7 +122,6 @@ pub unsafe extern "C" fn fog_init(
         app_id_override.to_utf8().to_string()
     };
 
-    let upload_enabled = static_prefs::pref!("datareporting.healthreport.uploadEnabled");
     let configuration = Configuration {
         upload_enabled,
         data_path,
@@ -97,24 +130,13 @@ pub unsafe extern "C" fn fog_init(
         delay_ping_lifetime_io: true,
         channel: Some(channel),
         server_endpoint: Some(server),
-        uploader: Some(Box::new(ViaductUploader) as Box<dyn glean::net::PingUploader>),
+        uploader,
         use_core_mps,
     };
 
     log::debug!("Configuration: {:#?}", configuration);
 
-    // Ensure Viaduct is initialized for networking unconditionally so we don't
-    // need to check again if upload is later enabled.
-    if let Some(viaduct) =
-        xpcom::create_instance::<mozIViaduct>(cstr!("@mozilla.org/toolkit/viaduct;1"))
-    {
-        let result = viaduct.EnsureInitialized();
-        if result.failed() {
-            log::error!("Failed to ensure viaduct was initialized due to {}. Ping upload may not be available.", result.error_name());
-        }
-    } else {
-        log::error!("Failed to create Viaduct via XPCOM. Ping upload may not be available.");
-    }
+    setup_viaduct();
 
     // If we're operating in automation without any specific source tags to set,
     // set the tag "automation" so any pings that escape don't clutter the tables.
@@ -131,7 +153,64 @@ pub unsafe extern "C" fn fog_init(
 
     fog::metrics::fog::initialization.stop();
 
-    NS_OK
+    Ok(())
+}
+
+#[cfg(not(target_os = "android"))]
+fn setup_observers() -> Result<(), nsresult> {
+    if let Err(e) = UploadPrefObserver::begin_observing() {
+        log::error!(
+            "Could not observe data upload pref. Abandoning FOG init due to {:?}",
+            e
+        );
+        return Err(e);
+    }
+
+    if let Err(e) = UserActivityObserver::begin_observing() {
+        log::error!(
+            "Could not observe user activity. Abandoning FOG init due to {:?}",
+            e
+        );
+        return Err(e);
+    }
+
+    Ok(())
+}
+
+#[cfg(target_os = "android")]
+fn setup_observers() -> Result<(), nsresult> {
+    // No observers are set up on Android.
+    Ok(())
+}
+
+/// Ensure Viaduct is initialized for networking unconditionally so we don't
+/// need to check again if upload is later enabled.
+///
+/// Failing to initialize viaduct will log an error.
+#[cfg(not(target_os = "android"))]
+fn setup_viaduct() {
+    // SAFETY: Everything here is self-contained.
+    //
+    // * We try to create an instance of an xpcom interface
+    // * We bail out if that fails.
+    // * We call a method on it without additional inputs.
+    unsafe {
+        if let Some(viaduct) =
+            xpcom::create_instance::<mozIViaduct>(cstr!("@mozilla.org/toolkit/viaduct;1"))
+        {
+            let result = viaduct.EnsureInitialized();
+            if result.failed() {
+                log::error!("Failed to ensure viaduct was initialized due to {}. Ping upload may not be available.", result.error_name());
+            }
+        } else {
+            log::error!("Failed to create Viaduct via XPCOM. Ping upload may not be available.");
+        }
+    }
+}
+
+#[cfg(target_os = "android")]
+fn setup_viaduct() {
+    // No viaduct is setup on Android.
 }
 
 /// Construct and return the data_path from the profile dir, or return an error.

toolkit/components/glean/src/init/upload_pref.rs

@@ -20,15 +20,25 @@ pub(crate) struct InitUploadPrefObserver {}
 
 #[allow(non_snake_case)]
 impl UploadPrefObserver {
-    pub(crate) unsafe fn begin_observing() -> Result<(), nsresult> {
-        let pref_obs = Self::allocate(InitUploadPrefObserver {});
-        let pref_service = xpcom::services::get_PrefService().ok_or(NS_ERROR_FAILURE)?;
-        let pref_branch: RefPtr<nsIPrefBranch> =
-            (*pref_service).query_interface().ok_or(NS_ERROR_FAILURE)?;
-        let pref_nscstr = &nsCStr::from("datareporting.healthreport.uploadEnabled") as &nsACString;
-        (*pref_branch)
-            .AddObserverImpl(pref_nscstr, pref_obs.coerce(), false)
-            .to_result()?;
+    pub(crate) fn begin_observing() -> Result<(), nsresult> {
+        // SAFETY: Everything here is self-contained.
+        //
+        // * We allocate the pref observer, created by the xpcom macro
+        // * We query the pref service and bail out if it doesn't exist.
+        // * We create a nsCStr from a static string.
+        // * We control all input to `AddObserverImpl`
+        unsafe {
+            let pref_obs = Self::allocate(InitUploadPrefObserver {});
+            let pref_service = xpcom::services::get_PrefService().ok_or(NS_ERROR_FAILURE)?;
+            let pref_branch: RefPtr<nsIPrefBranch> =
+                (*pref_service).query_interface().ok_or(NS_ERROR_FAILURE)?;
+            let pref_nscstr =
+                &nsCStr::from("datareporting.healthreport.uploadEnabled") as &nsACString;
+            (*pref_branch)
+                .AddObserverImpl(pref_nscstr, pref_obs.coerce(), false)
+                .to_result()?;
+        }
+
         Ok(())
     }
 

toolkit/components/glean/src/init/user_activity.rs

@@ -29,31 +29,38 @@ pub(crate) struct InitUserActivityObserver {
 /// for more info.
 #[allow(non_snake_case)]
 impl UserActivityObserver {
-    pub(crate) unsafe fn begin_observing() -> Result<(), nsresult> {
+    pub(crate) fn begin_observing() -> Result<(), nsresult> {
         // First and foremost, even if we can't get the ObserverService,
         // init always means client activity.
         glean::handle_client_active();
 
-        let activity_obs = Self::allocate(InitUserActivityObserver {
-            last_edge: RwLock::new(Instant::now()),
-            was_active: AtomicBool::new(false),
-        });
-        let obs_service = xpcom::services::get_ObserverService().ok_or(NS_ERROR_FAILURE)?;
-        let rv = obs_service.AddObserver(
-            activity_obs.coerce(),
-            cstr!("user-interaction-active").as_ptr(),
-            false,
-        );
-        if !rv.succeeded() {
-            return Err(rv);
-        }
-        let rv = obs_service.AddObserver(
-            activity_obs.coerce(),
-            cstr!("user-interaction-inactive").as_ptr(),
-            false,
-        );
-        if !rv.succeeded() {
-            return Err(rv);
+        // SAFETY: Everything here is self-contained.
+        //
+        // * We allocate the activity observer, created by the xpcom macro
+        // * We create cstr from a static string.
+        // * We control all input to `AddObserver`
+        unsafe {
+            let activity_obs = Self::allocate(InitUserActivityObserver {
+                last_edge: RwLock::new(Instant::now()),
+                was_active: AtomicBool::new(false),
+            });
+            let obs_service = xpcom::services::get_ObserverService().ok_or(NS_ERROR_FAILURE)?;
+            let rv = obs_service.AddObserver(
+                activity_obs.coerce(),
+                cstr!("user-interaction-active").as_ptr(),
+                false,
+            );
+            if !rv.succeeded() {
+                return Err(rv);
+            }
+            let rv = obs_service.AddObserver(
+                activity_obs.coerce(),
+                cstr!("user-interaction-inactive").as_ptr(),
+                false,
+            );
+            if !rv.succeeded() {
+                return Err(rv);
+            }
         }
         Ok(())
     }

toolkit/components/glean/src/lib.rs

@@ -25,18 +25,15 @@ use nserror::{nsresult, NS_ERROR_FAILURE, NS_OK};
 use nsstring::{nsACString, nsCString};
 use thin_vec::ThinVec;
 
-#[cfg(not(target_os = "android"))]
+// Needed for re-export.
+#[cfg(target_os = "android")]
+pub use glean_ffi;
+
 #[macro_use]
 extern crate cstr;
-#[cfg(not(target_os = "android"))]
-#[macro_use]
+#[cfg_attr(not(target_os = "android"), macro_use)]
 extern crate xpcom;
 
-#[cfg(not(target_os = "android"))]
-mod init;
-
-#[cfg(target_os = "android")]
-#[path = "android_init.rs"]
 mod init;
 
 pub use init::fog_init;

toolkit/components/glean/tests/xpcshell/test_GleanAndroid.js

@@ -1,12 +1,6 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
-/* FIXME: Remove these global markers.
- * FOG doesn't follow the stricter naming patterns as expected by tool configuration yet.
- * See https://searchfox.org/mozilla-central/source/.eslintrc.js#24
- * Reorganizing the directory structure will take this into account.
- */
-/* global add_task, Assert */
 "use strict";
 
 Cu.importGlobalProperties(["Glean", "GleanPings"]);
@@ -17,23 +11,16 @@ function sleep(ms) {
   return new Promise(resolve => setTimeout(resolve, ms));
 }
 
-// No call to `FOG.initializeFOG()`.
-// Glean is not initialized on Android and we need to ensure
-// both the regular API and the test APIs work regardless.
-
 add_task(function test_fog_counter_works() {
   Glean.testOnly.badCode.add(31);
-  Assert.equal(undefined, Glean.testOnly.badCode.testGetValue("test-ping"));
+  Assert.equal(31, Glean.testOnly.badCode.testGetValue("test-ping"));
 });
 
 add_task(async function test_fog_string_works() {
   const value = "a cheesy string!";
   Glean.testOnly.cheesyString.set(value);
 
-  Assert.equal(
-    undefined,
-    Glean.testOnly.cheesyString.testGetValue("test-ping")
-  );
+  Assert.equal(value, Glean.testOnly.cheesyString.testGetValue("test-ping"));
 });
 
 add_task(async function test_fog_string_list_works() {
@@ -84,9 +71,9 @@ add_task({ skip_if: () => true }, function test_fog_datetime_works() {
 
 add_task(function test_fog_boolean_works() {
   Glean.testOnly.canWeFlagIt.set(false);
-  Assert.equal(undefined, Glean.testOnly.canWeFlagIt.testGetValue("test-ping"));
+  Assert.equal(false, Glean.testOnly.canWeFlagIt.testGetValue("test-ping"));
   // While you're here, might as well test that the ping name's optional.
-  Assert.equal(undefined, Glean.testOnly.canWeFlagIt.testGetValue());
+  Assert.equal(false, Glean.testOnly.canWeFlagIt.testGetValue());
 });
 
 add_task(async function test_fog_event_works() {
@@ -141,11 +128,11 @@ add_task(async function test_fog_labeled_boolean_works() {
   Glean.testOnly.mabelsLikeBalloons.at_parties.set(true);
   Glean.testOnly.mabelsLikeBalloons.at_funerals.set(false);
   Assert.equal(
-    undefined,
+    true,
     Glean.testOnly.mabelsLikeBalloons.at_parties.testGetValue()
   );
   Assert.equal(
-    undefined,
+    false,
     Glean.testOnly.mabelsLikeBalloons.at_funerals.testGetValue()
   );
   Assert.equal(
@@ -153,8 +140,9 @@ add_task(async function test_fog_labeled_boolean_works() {
     Glean.testOnly.mabelsLikeBalloons.__other__.testGetValue()
   );
   Glean.testOnly.mabelsLikeBalloons.InvalidLabel.set(true);
+  // FIXME: Booleans don't return errors, but labeled booleans could. We should check for that properly.
   Assert.equal(
-    undefined,
+    true,
     Glean.testOnly.mabelsLikeBalloons.__other__.testGetValue()
   );
   // TODO: Test that we have the right number and type of errors (bug 1683171)
@@ -169,11 +157,11 @@ add_task(async function test_fog_labeled_counter_works() {
   Glean.testOnly.mabelsKitchenCounters.near_the_sink.add(1);
   Glean.testOnly.mabelsKitchenCounters.with_junk_on_them.add(2);
   Assert.equal(
-    undefined,
+    1,
     Glean.testOnly.mabelsKitchenCounters.near_the_sink.testGetValue()
   );
   Assert.equal(
-    undefined,
+    2,
     Glean.testOnly.mabelsKitchenCounters.with_junk_on_them.testGetValue()
   );
   // What about invalid/__other__?
@@ -182,11 +170,11 @@ add_task(async function test_fog_labeled_counter_works() {
     Glean.testOnly.mabelsKitchenCounters.__other__.testGetValue()
   );
   Glean.testOnly.mabelsKitchenCounters.InvalidLabel.add(1);
-  Assert.equal(
-    undefined,
-    Glean.testOnly.mabelsKitchenCounters.__other__.testGetValue()
+  Assert.throws(
+    () => Glean.testOnly.mabelsKitchenCounters.__other__.testGetValue(),
+    /NS_ERROR_LOSS_OF_SIGNIFICANT_DATA/,
+    "Should throw because of a recording error."
   );
-  // TODO: Test that we have the right number and type of errors (bug 1683171)
 });
 
 add_task(async function test_fog_labeled_string_works() {
@@ -197,11 +185,11 @@ add_task(async function test_fog_labeled_string_works() {
   Glean.testOnly.mabelsBalloonStrings.colour_of_99.set("crimson");
   Glean.testOnly.mabelsBalloonStrings.string_lengths.set("various");
   Assert.equal(
-    undefined,
+    "crimson",
     Glean.testOnly.mabelsBalloonStrings.colour_of_99.testGetValue()
   );
   Assert.equal(
-    undefined,
+    "various",
     Glean.testOnly.mabelsBalloonStrings.string_lengths.testGetValue()
   );
   // What about invalid/__other__?
@@ -210,9 +198,9 @@ add_task(async function test_fog_labeled_string_works() {
     Glean.testOnly.mabelsBalloonStrings.__other__.testGetValue()
   );
   Glean.testOnly.mabelsBalloonStrings.InvalidLabel.set("valid");
-  Assert.equal(
-    undefined,
-    Glean.testOnly.mabelsBalloonStrings.__other__.testGetValue()
+  Assert.throws(
+    () => Glean.testOnly.mabelsBalloonStrings.__other__.testGetValue(),
+    /NS_ERROR_LOSS_OF_SIGNIFICANT_DATA/,
+    "Should throw because of a recording error."
   );
-  // TODO: Test that we have the right number and type of errors (bug 1683171)
 });