Bug 1356673 - Run the pingsender executable in a way that avoids shutdown races with mozalloc; r?Dexter draft
authorGabriele Svelto <gsvelto@mozilla.com>
Tue, 25 Apr 2017 01:07:36 +0200
changeset 568670 d7c7000ad921d838a85f51286ad8392bcf4486e7
parent 568252 3f0c8da53c5cb015933b10b52ded3f30432b378a
child 626000 c6c948cdcdc78d9dc300e1cc5972eb64e014c2ec
push id55951
push usergsvelto@mozilla.com
push dateWed, 26 Apr 2017 13:09:04 +0000
reviewersDexter
bugs1356673
milestone55.0a1
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
browser/app/profile/firefox.js
toolkit/components/telemetry/Telemetry.cpp
toolkit/components/telemetry/TelemetrySend.jsm
toolkit/components/telemetry/moz.build
toolkit/components/telemetry/nsITelemetry.idl
toolkit/components/telemetry/tests/unit/test_PingSender.js
--- a/browser/app/profile/firefox.js
+++ b/browser/app/profile/firefox.js
@@ -1439,16 +1439,18 @@ pref("browser.translation.neverForLangua
 // Show the translation UI bits, like the info bar, notification icon and preferences.
 pref("browser.translation.ui.show", false);
 // Allows to define the translation engine. Bing is default, Yandex may optionally switched on.
 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);
 pref("experiments.manifest.fetchIntervalSeconds", 86400);
 pref("experiments.manifest.uri", "https://telemetry-experiment.cdn.mozilla.net/manifest/v1/firefox/%VERSION%/%CHANNEL%");
 // Whether experiments are supported by the current application profile.
 pref("experiments.supported", true);
 
--- a/toolkit/components/telemetry/Telemetry.cpp
+++ b/toolkit/components/telemetry/Telemetry.cpp
@@ -79,17 +79,16 @@
 #include "mozilla/Preferences.h"
 #include "mozilla/StaticPtr.h"
 #include "mozilla/IOInterposer.h"
 #include "mozilla/PoisonIOInterposer.h"
 #include "mozilla/StartupTimeline.h"
 #include "mozilla/HangMonitor.h"
 #include "nsNativeCharsetUtils.h"
 #include "nsProxyRelease.h"
-#include "nsDirectoryServiceDefs.h"
 
 #if defined(MOZ_GECKO_PROFILER)
 #include "shared-libraries.h"
 #define ENABLE_STACK_CAPTURE
 #include "mozilla/StackWalk.h"
 #include "nsPrintfCString.h"
 #endif // MOZ_GECKO_PROFILER
 
@@ -2866,88 +2865,16 @@ TelemetryImpl::SetEventRecordingEnabled(
 
 NS_IMETHODIMP
 TelemetryImpl::FlushBatchedChildTelemetry()
 {
   TelemetryIPCAccumulator::IPCTimerFired(nullptr, nullptr);
   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)
 {
   size_t n = aMallocSizeOf(this);
 
   n += TelemetryHistogram::GetMapShallowSizesOfExcludingThis(aMallocSizeOf);
   n += TelemetryScalar::GetMapShallowSizesOfExcludingThis(aMallocSizeOf);
   n += mWebrtcTelemetry.SizeOfExcludingThis(aMallocSizeOf);
--- a/toolkit/components/telemetry/TelemetrySend.jsm
+++ b/toolkit/components/telemetry/TelemetrySend.jsm
@@ -10,17 +10,17 @@
  */
 
 "use strict";
 
 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);
 Cu.import("resource://gre/modules/Task.jsm", this);
 Cu.import("resource://gre/modules/ClientID.jsm");
 Cu.import("resource://gre/modules/Log.jsm", this);
 Cu.import("resource://gre/modules/Preferences.jsm");
 Cu.import("resource://gre/modules/PromiseUtils.jsm");
@@ -275,16 +275,37 @@ this.TelemetrySend = {
   },
 
   /**
    * This returns state info for this module for AsyncShutdown timeout diagnostics.
    */
   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 = {
   _deferred: null,
   _timer: null,
 
   /**
    * This waits until either the given timeout passed or the timeout was cancelled.
@@ -779,17 +800,17 @@ var TelemetrySendImpl = {
    *
    * @param {String} pingId The id of the ping to send.
    * @param {String} submissionURL The complete Telemetry-compliant URL for the ping.
    */
   _sendWithPingSender(pingId, submissionURL) {
     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);
     }
   },
 
   submitPing(ping, options) {
     this._log.trace("submitPing - ping id: " + ping.id + ", options: " + JSON.stringify(options));
 
@@ -1221,9 +1242,26 @@ var TelemetrySendImpl = {
       sendingEnabled: this._sendingEnabled,
       pendingPingRequestCount: this._pendingPingRequests.size,
       pendingPingActivityCount: this._pendingPingActivity.size,
       unpersistedPingCount: this._currentPings.size,
       persistedPingCount: TelemetryStorage.getPendingPingList().length,
       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);
+  },
 };
--- a/toolkit/components/telemetry/moz.build
+++ b/toolkit/components/telemetry/moz.build
@@ -10,17 +10,16 @@ include('/ipc/chromium/chromium-config.m
 
 FINAL_LIBRARY = 'xul'
 
 DIRS = [
     'pingsender',
 ]
 
 DEFINES['MOZ_APP_VERSION'] = '"%s"' % CONFIG['MOZ_APP_VERSION']
-DEFINES['BIN_SUFFIX'] = '"%s"' % CONFIG['BIN_SUFFIX']
 
 LOCAL_INCLUDES += [
     '/xpcom/build',
     '/xpcom/threads',
 ]
 
 SPHINX_TREES['telemetry'] = 'docs'
 
--- a/toolkit/components/telemetry/nsITelemetry.idl
+++ b/toolkit/components/telemetry/nsITelemetry.idl
@@ -491,23 +491,9 @@ interface nsITelemetry : nsISupports
    */
   [implicit_jscontext, optional_argc]
   jsval snapshotBuiltinEvents(in uint32_t aDataset, [optional] in boolean aClear);
 
   /**
    * 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);
 };
--- a/toolkit/components/telemetry/tests/unit/test_PingSender.js
+++ b/toolkit/components/telemetry/tests/unit/test_PingSender.js
@@ -5,16 +5,17 @@
 // This tests submitting a ping using the stand-alone pingsender program.
 
 "use strict";
 
 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);
 
 const PREF_FHR_UPLOAD_ENABLED = "datareporting.healthreport.uploadEnabled";
 const PREF_TELEMETRY_SERVER = "toolkit.telemetry.server";
 
 /**
@@ -78,28 +79,28 @@ add_task(function* test_pingSender() {
       // Resolve the promise on the next tick.
       Services.tm.dispatchToMainThread(() => deferred404Hit.resolve());
     }
   });
   failingServer.start(-1);
 
   // 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.
   yield deferred404Hit.promise;
   Assert.ok((yield OS.File.exists(pingPath)),
             "The pending ping must not be deleted if we fail to send using the 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);
 
   Assert.equal(req.getHeader("User-Agent"), "pingsender/1.0",
                "Should have received the correct user agent string.");
   Assert.equal(req.getHeader("X-PingSender-Version"), "1.0",
                "Should have received the correct PingSender version string.");