Backed out changeset c859ac601495 (bug 1737157) for causing xpc failures in glean/tests/xpcshell/test_FOGPrefs. CLOSED TREE

toolkit/components/glean/bindings/private/Ping.cpp

@@ -6,7 +6,6 @@
 
 #include "mozilla/glean/bindings/Ping.h"
 
-#include "mozilla/ClearOnShutdown.h"
 #include "mozilla/Components.h"
 #include "nsIClassInfoImpl.h"
 #include "nsString.h"
@@ -23,10 +22,6 @@ static MetricIdToCallbackMutex::AutoLock GetCallbackMapLock() {
   auto lock = sCallbacks.Lock();
   if (!*lock) {
     *lock = MakeUnique<CallbackMapType>();
-    RunOnShutdown([&] {
-      auto lock = sCallbacks.Lock();
-      *lock = nullptr;  // deletes, see UniquePtr.h
-    });
   }
   return lock;
 }

toolkit/components/glean/docs/dev/preferences.md

@@ -16,16 +16,11 @@ and no data collections will be persisted or reported from that point.
 
 If set to a value `port` which is greater than 0, pings will be sent to
 `http://localhost:port` instead of `https://incoming.telemetry.mozilla.org`.
-If set to a value `port` which is less than 0, FOG will:
-1) Tell Glean that upload is enabled, even if it isn't.
-2) Take all pings scheduled for upload and drop them on the floor,
-   telling the Glean SDK that it was sent successfully.
-
+If set to a value which is less than 0,
+FOG will take all pings scheduled for upload and drop them on the floor,
+telling the Glean SDK that it was sent successfully.
 This is how you emulate "recording enabled but upload disabled"
 like developer builds have in Firefox Telemetry.
-When switching from `port < 0` to `port >= 0`,
-Glean will be told (if just temporarily) that upload is disabled.
-This clears the stores of recorded-but-not-reported data.
 Defaults to 0.
 
 `telemetry.fog.test.activity_limit`

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

@@ -41,13 +41,12 @@ pub extern "C" fn fog_init(
     app_id_override: &nsACString,
 ) -> nsresult {
     let upload_enabled = static_prefs::pref!("datareporting.healthreport.uploadEnabled");
-    let recording_enabled = static_prefs::pref!("telemetry.fog.test.localhost_port") < 0;
     let uploader = Some(Box::new(ViaductUploader) as Box<dyn glean::net::PingUploader>);
 
     fog_init_internal(
         data_path_override,
         app_id_override,
-        upload_enabled || recording_enabled,
+        upload_enabled,
         uploader,
     )
     .into()

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

@@ -4,7 +4,6 @@
 
 use std::ffi::CStr;
 use std::os::raw::c_char;
-use std::sync::atomic::{AtomicBool, Ordering};
 
 use nserror::{nsresult, NS_ERROR_FAILURE, NS_OK};
 use nsstring::{nsACString, nsCStr};
@@ -13,10 +12,6 @@ use xpcom::{
     RefPtr, XpCom,
 };
 
-/// Whether the current value of the localhost testing pref is permitting
-/// metric recording (even if upload is disabled).
-static RECORDING_ENABLED: AtomicBool = AtomicBool::new(false);
-
 // Partially cargo-culted from https://searchfox.org/mozilla-central/rev/598e50d2c3cd81cd616654f16af811adceb08f9f/security/manager/ssl/cert_storage/src/lib.rs#1192
 #[derive(xpcom)]
 #[xpimplements(nsIObserver)]
@@ -42,10 +37,6 @@ impl UploadPrefObserver {
             (*pref_branch)
                 .AddObserverImpl(pref_nscstr, pref_obs.coerce(), false)
                 .to_result()?;
-            let pref_nscstr = &nsCStr::from("telemetry.fog.test.localhost_port") as &nsACString;
-            (*pref_branch)
-                .AddObserverImpl(pref_nscstr, pref_obs.coerce(), false)
-                .to_result()?;
         }
 
         Ok(())
@@ -59,7 +50,7 @@ impl UploadPrefObserver {
     ) -> nserror::nsresult {
         let topic = CStr::from_ptr(topic).to_str().unwrap();
         // Conversion utf16 to utf8 is messy.
-        // We should only ever observe changes to one of the two prefs we want,
+        // We should only ever observe changes to the one pref we want,
         // but just to be on the safe side let's assert.
 
         // cargo-culted from https://searchfox.org/mozilla-central/rev/598e50d2c3cd81cd616654f16af811adceb08f9f/security/manager/ssl/cert_storage/src/lib.rs#1606-1612
@@ -71,23 +62,12 @@ impl UploadPrefObserver {
             Err(_) => return NS_ERROR_FAILURE,
         };
         log::info!("Observed {:?}, {:?}", topic, pref_name);
-        debug_assert!(topic == "nsPref:changed");
         debug_assert!(
-            pref_name == "datareporting.healthreport.uploadEnabled"
-                || pref_name == "telemetry.fog.test.localhost_port"
+            topic == "nsPref:changed" && pref_name == "datareporting.healthreport.uploadEnabled"
         );
 
         let upload_enabled = static_prefs::pref!("datareporting.healthreport.uploadEnabled");
-        let recording_enabled = static_prefs::pref!("telemetry.fog.test.localhost_port") < 0;
-        if RECORDING_ENABLED.load(Ordering::SeqCst) && !recording_enabled {
-            // Whenever the test pref goes from permitting recording to forbidding it,
-            // ensure Glean is told to wipe the stores.
-            // This may send a "deletion-request" ping for a client_id that's never sent
-            // any other pings.
-            glean::set_upload_enabled(false);
-        }
-        RECORDING_ENABLED.store(recording_enabled, Ordering::SeqCst);
-        glean::set_upload_enabled(upload_enabled || recording_enabled);
+        glean::set_upload_enabled(upload_enabled);
         NS_OK
     }
 }

toolkit/components/glean/tests/test_pings.yaml

@@ -23,16 +23,3 @@ one-ping-only:
   notification_emails:
     - chutten@mozilla.com
     - glean-team@mozilla.com
-
-test-ping:
-  description: |
-    This ping is for tests only.
-  include_client_id: false
-  send_if_empty: true
-  bugs:
-    - https://bugzilla.mozilla.org/show_bug.cgi?id=1737157
-  data_reviews:
-    - https://bugzilla.mozilla.org/show_bug.cgi?id=1737157#c1
-  notification_emails:
-    - chutten@mozilla.com
-    - glean-team@mozilla.com

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

@@ -1,78 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-"use strict";
-
-Cu.importGlobalProperties(["Glean", "GleanPings"]);
-const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
-
-// FOG needs a profile directory to put its data in.
-do_get_profile();
-
-const FOG = Cc["@mozilla.org/toolkit/glean;1"].createInstance(Ci.nsIFOG);
-// We need to initialize it once, otherwise operations will be stuck in the pre-init queue.
-FOG.initializeFOG();
-
-const TELEMETRY_SERVER_PREF = "toolkit.telemetry.server";
-const UPLOAD_PREF = "datareporting.healthreport.uploadEnabled";
-const LOCALHOST_PREF = "telemetry.fog.test.localhost_port";
-
-add_task(function test_fog_upload_only() {
-  // Don't forget to point the telemetry server to localhost, or Telemetry
-  // might make a non-local connection during the test run.
-  Services.prefs.setStringPref(
-    TELEMETRY_SERVER_PREF,
-    "http://localhost/telemetry-fake/"
-  );
-  // Be sure to set port=-1 for faking success _before_ enabling upload.
-  // Or else there's a short window where we might send something.
-  Services.prefs.setIntPref(LOCALHOST_PREF, -1);
-  Services.prefs.setBoolPref(UPLOAD_PREF, true);
-
-  // Ensure we don't send "test-ping".
-  GleanPings.testPing.testBeforeNextSubmit(() => {
-    Assert.ok(false, "Ping 'test-ping' must not be sent.");
-  });
-
-  const value = 42;
-  Glean.testOnly.meaningOfLife.set(value);
-  // We specifically look at the custom ping's value because we know it
-  // won't be reset by being sent.
-  Assert.equal(value, Glean.testOnly.meaningOfLife.testGetValue("test-ping"));
-
-  // Despite upload being disabled, we keep the old values around.
-  Services.prefs.setBoolPref(UPLOAD_PREF, false);
-  Assert.equal(value, Glean.testOnly.meaningOfLife.testGetValue("test-ping"));
-
-  // Now this is the part where we'd test the behaviour of "when we turn upload
-  // back on and turn fake upload off, we remember to delete any stored data".
-  // Except, when we disabled upload we triggered a "deletion-request" ping.
-  // If we turn fake upload off, it'll get sent and crash the test runner.
-
-  // Thus, we'll test that in the next test instead of in this one.
-
-  // Cleanup: Replace the asserting callback with a noop.
-  GleanPings.testPing.testBeforeNextSubmit(() => {});
-});
-
-add_task(function test_fog_deletes_upload_only_data() {
-  Services.prefs.setIntPref(LOCALHOST_PREF, -1);
-
-  // Ensure we don't send "test-ping".
-  GleanPings.testPing.testBeforeNextSubmit(() => {
-    Assert.ok(false, "Ping 'test-ping' must not be sent.");
-  });
-
-  const value = 42 * 2; // It's the second time we're using it in this file.
-  Glean.testOnly.meaningOfLife.set(value);
-  // We specifically look at the custom ping's value because we know it
-  // won't be reset by being sent.
-  Assert.equal(value, Glean.testOnly.meaningOfLife.testGetValue("test-ping"));
-
-  // Now, when we turn the fake upload off, we clear the stores
-  Services.prefs.setIntPref(LOCALHOST_PREF, 0);
-  Assert.equal(
-    undefined,
-    Glean.testOnly.meaningOfLife.testGetValue("test-ping")
-  );
-});

toolkit/components/glean/tests/xpcshell/xpcshell.ini

@@ -1,10 +1,6 @@
-# Please keep test files lexicographically sorted, with whitespace between.
 [DEFAULT]
 firefox-appdir = browser
 
-[test_FOGPrefs.js]
-skip-if = os == "android" # FOG isn't responsible for monitoring prefs and controlling upload on Android
-
 [test_GIFFT.js]
 
 [test_Glean.js]