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
--- 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.");