Bug 1356673 - Run the pingsender executable in a way that avoids shutdown races with mozalloc; r=Dexter

When sending the shutdown ping we launched the pingsender executable via
PR_CreateProcessDetached() which on both Linux and MacOS X would fork()
gecko's main process and then exec() the pingsender executable. On MacOS X
this seemed to trigger a race with the mozalloc shutdown procedure within the
forked process. This patch changes the telemetry logic to use nsIProcess
instead which relies on posix_spawnp() to launch the new executable making it
immune to issues related to mozalloc's shutdown.

Since we don't need C++ code anymore to run the pingsender the runPingSender()
method is also moved to TelemtrySend.

MozReview-Commit-ID: C7fZw1ZpVBO

--HG--
extra : rebase_source : b6e1bd08f45533fceab4a2f2d81abd81c2267c9b

browser/app/profile/firefox.js

@@ -1457,6 +1457,8 @@ pref("browser.translation.engine", "bing");
 // Telemetry settings.
 // Determines if Telemetry pings can be archived locally.
 pref("toolkit.telemetry.archive.enabled", true);
+// Enables sending the shutdown ping when Firefox shuts down.
+pref("toolkit.telemetry.shutdownPingSender.enabled", true);
 
 // Telemetry experiments settings.
 pref("experiments.enabled", true);

toolkit/components/telemetry/Telemetry.cpp

@@ -84,7 +84,6 @@
 #include "mozilla/HangMonitor.h"
 #include "nsNativeCharsetUtils.h"
 #include "nsProxyRelease.h"
-#include "nsDirectoryServiceDefs.h"
 
 #if defined(MOZ_GECKO_PROFILER)
 #include "shared-libraries.h"
@@ -2871,78 +2870,6 @@ TelemetryImpl::FlushBatchedChildTelemetry()
   return NS_OK;
 }
 
-#ifndef MOZ_WIDGET_ANDROID
-
-static nsresult
-LocatePingSender(nsAString& aPath)
-{
-  nsCOMPtr<nsIFile> xreGreBinDir;
-  nsresult rv = NS_GetSpecialDirectory(NS_GRE_BIN_DIR,
-                                       getter_AddRefs(xreGreBinDir));
-
-  if (NS_WARN_IF(NS_FAILED(rv))) {
-    return NS_ERROR_FAILURE;
-  }
-
-  // Make sure that the executable exists. Otherwise, quit.
-  bool exists = false;
-  if (NS_FAILED(xreGreBinDir->Exists(&exists)) || !exists) {
-    return NS_ERROR_FAILURE;
-  }
-
-  xreGreBinDir->AppendNative(NS_LITERAL_CSTRING("pingsender" BIN_SUFFIX));
-  xreGreBinDir->GetPath(aPath);
-  return NS_OK;
-}
-
-#endif // MOZ_WIDGET_ANDROID
-
-NS_IMETHODIMP
-TelemetryImpl::RunPingSender(const nsACString& aUrl,
-                             const nsACString& aPingFilePath)
-{
-#ifdef MOZ_WIDGET_ANDROID
-  Unused << aUrl;
-  Unused << aPingFilePath;
-
-  return NS_ERROR_NOT_IMPLEMENTED;
-#else // Windows, Mac, Linux, etc...
-  // Obtain the path of the pingsender executable
-  nsAutoString path;
-  nsresult rv = LocatePingSender(path);
-
-  if (NS_WARN_IF(NS_FAILED(rv))) {
-    return NS_ERROR_FAILURE;
-  }
-
-  PRProcessAttr* attr = PR_NewProcessAttr();
-  if (!attr) {
-    return NS_ERROR_FAILURE;
-  }
-
-  // We pretend we're redirecting stdout to force NSPR not to show a
-  // command prompt when launching the program.
-  PR_ProcessAttrSetStdioRedirect(attr, PR_StandardOutput, PR_STDOUT);
-
-  UniquePtr<char[]> asciiPath(ToNewCString(aPingFilePath));
-  UniquePtr<char[]> pingSenderPath(ToNewCString(path));
-  UniquePtr<char[]> serverURL(ToNewCString(aUrl));
-
-  // The next lines are needed as |args| is not const.
-  char* args[] = {
-    pingSenderPath.get(),
-    serverURL.get(),
-    asciiPath.get(),
-    nullptr,
-  };
-
-  Unused << NS_WARN_IF(PR_CreateProcessDetached(args[0], args, nullptr, attr));
-  PR_DestroyProcessAttr(attr);
-
-  return NS_OK;
-#endif
-}
-
 size_t
 TelemetryImpl::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf)
 {

toolkit/components/telemetry/TelemetrySend.jsm

@@ -15,7 +15,7 @@ this.EXPORTED_SYMBOLS = [
   "TelemetrySend",
 ];
 
-const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
+const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
 
 Cu.import("resource://gre/modules/AppConstants.jsm", this);
 Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);
@@ -280,6 +280,27 @@ this.TelemetrySend = {
   getShutdownState() {
     return TelemetrySendImpl.getShutdownState();
   },
+
+  /**
+   * Send a ping using the ping sender.
+   * This method will not wait for the ping to be sent, instead it will return
+   * as soon as the pingsender program has been launched.
+   *
+   * This method is currently exposed here only for testing purposes as it's
+   * only used internally.
+   *
+   * @param {String} aUrl The telemetry server URL
+   * @param {String} aPingFilePath The path to the file holding the ping
+   *        contents, if if sent successfully the pingsender will delete it.
+   *
+   * @throws NS_ERROR_FAILURE if we couldn't find or run the pingsender
+   *         executable.
+   * @throws NS_ERROR_NOT_IMPLEMENTED on Android as the pingsender is not
+   *         available.
+   */
+  testRunPingSender(url, pingPath) {
+    TelemetrySendImpl.runPingSender(url, pingPath);
+  },
 };
 
 var CancellableTimeout = {
@@ -784,7 +805,7 @@ var TelemetrySendImpl = {
     this._log.trace("_sendWithPingSender - sending " + pingId + " to " + submissionURL);
     try {
       const pingPath = OS.Path.join(TelemetryStorage.pingDirectoryPath, pingId);
-      Telemetry.runPingSender(submissionURL, pingPath);
+      this.runPingSender(submissionURL, pingPath);
     } catch (e) {
       this._log.error("_sendWithPingSender - failed to submit shutdown ping", e);
     }
@@ -1226,4 +1247,21 @@ var TelemetrySendImpl = {
       schedulerState: SendScheduler.getShutdownState(),
     };
   },
+
+  runPingSender(url, pingPath) {
+    if (AppConstants.platform === "android") {
+      throw Cr.NS_ERROR_NOT_IMPLEMENTED;
+    }
+
+    const exeName = AppConstants.platform === "win" ? "pingsender.exe"
+                                                    : "pingsender";
+
+    let exe = Services.dirsvc.get("GreBinD", Ci.nsIFile);
+    exe.append(exeName);
+
+    let process = Cc["@mozilla.org/process/util;1"]
+                  .createInstance(Ci.nsIProcess);
+    process.init(exe);
+    process.run(/* blocking */ false, [url, pingPath], 2);
+  },
 };

toolkit/components/telemetry/moz.build

@@ -15,7 +15,6 @@ DIRS = [
 ]
 
 DEFINES['MOZ_APP_VERSION'] = '"%s"' % CONFIG['MOZ_APP_VERSION']
-DEFINES['BIN_SUFFIX'] = '"%s"' % CONFIG['BIN_SUFFIX']
 
 LOCAL_INCLUDES += [
     '/xpcom/build',

toolkit/components/telemetry/nsITelemetry.idl

@@ -496,18 +496,4 @@ interface nsITelemetry : nsISupports
    * Resets all the stored events. This is intended to be only used in tests.
    */
   void clearEvents();
-
-  /**
-   * Send a ping using the ping sender.
-   * This method will not wait for the ping to be sent, instead it will return
-   * as soon as the contents of the ping have been handed over to the ping
-   * sender.
-   *
-   * @param {String} aUrl The telemetry server URL
-   * @param {String} aPingFilePath The path to the file holding the ping
-   *        contents, if if sent successfully the pingsender will delete it.
-   *
-   * @throws NS_ERROR_FAILURE if we couldn't find or run the pingsender executable.
-   */
-  void runPingSender(in ACString aUrl, in ACString aPingFilePath);
 };

toolkit/components/telemetry/tests/unit/test_PingSender.js

@@ -10,6 +10,7 @@ Cu.import("resource://gre/modules/osfile.jsm", this);
 Cu.import("resource://gre/modules/Preferences.jsm", this);
 Cu.import("resource://gre/modules/PromiseUtils.jsm", this);
 Cu.import("resource://gre/modules/Services.jsm", this);
+Cu.import("resource://gre/modules/TelemetrySend.jsm", this);
 Cu.import("resource://gre/modules/TelemetryStorage.jsm", this);
 Cu.import("resource://gre/modules/TelemetryUtils.jsm", this);
 Cu.import("resource://gre/modules/Timer.jsm", this);
@@ -83,8 +84,8 @@ add_task(function* test_pingSender() {
 
   // Try to send the ping twice using the pingsender (we expect 404 both times).
   const errorUrl = "http://localhost:" + failingServer.identity.primaryPort + "/lookup_fail";
-  Telemetry.runPingSender(errorUrl, pingPath);
-  Telemetry.runPingSender(errorUrl, pingPath);
+  TelemetrySend.testRunPingSender(errorUrl, pingPath);
+  TelemetrySend.testRunPingSender(errorUrl, pingPath);
 
   // Wait until we hit the 404 server twice. After that, make sure that the ping
   // still exists locally.
@@ -94,7 +95,7 @@ add_task(function* test_pingSender() {
 
   // Try to send it using the pingsender.
   const url = "http://localhost:" + PingServer.port + "/submit/telemetry/";
-  Telemetry.runPingSender(url, pingPath);
+  TelemetrySend.testRunPingSender(url, pingPath);
 
   let req = yield PingServer.promiseNextRequest();
   let ping = decodeRequestPayload(req);