diff --git a/services/healthreport/healthreporter.jsm b/services/healthreport/healthreporter.jsm index 58129ca9890..225adf5e3e8 100644 --- a/services/healthreport/healthreporter.jsm +++ b/services/healthreport/healthreporter.jsm @@ -88,6 +88,8 @@ function AbstractHealthReporter(branch, policy, sessionRecorder) { this._shutdownComplete = false; this._shutdownCompleteCallback = null; + this._errors = []; + this._pullOnlyProviders = {}; this._pullOnlyProvidersRegistered = false; this._lastDailyDate = null; @@ -128,8 +130,7 @@ AbstractHealthReporter.prototype = Object.freeze({ delete this._initHistogram; delete this._dbOpenHistogram; - this._log.error("Error during initialization: " + - CommonUtils.exceptionStr(error)); + this._recordError("Error during initialization", error); this._initializeHadError = true; this._initiateShutdown(); this._initializedDeferred.reject(error); @@ -175,6 +176,7 @@ AbstractHealthReporter.prototype = Object.freeze({ this._log.info("Initializing collector."); this._collector = new Metrics.Collector(this._storage); + this._collector.onProviderError = this._errors.push.bind(this._errors); this._collectorInProgress = true; let catString = this._prefs.get("service.providerCategories") || ""; @@ -484,8 +486,8 @@ AbstractHealthReporter.prototype = Object.freeze({ promises.push(promise); } } catch (ex) { - this._log.warn("Error registering provider from category manager: " + - entry + "; " + CommonUtils.exceptionStr(ex)); + this._recordError("Error registering provider from category manager : " + + entry + ": ", ex); continue; } } @@ -525,8 +527,7 @@ AbstractHealthReporter.prototype = Object.freeze({ let provider = this.initProviderFromType(providerType); yield this.registerProvider(provider); } catch (ex) { - this._log.warn("Error registering pull-only provider: " + - CommonUtils.exceptionStr(ex)); + this._recordError("Error registering pull-only provider", ex); } } }.bind(this)).then(onFinished, onFinished); @@ -555,8 +556,8 @@ AbstractHealthReporter.prototype = Object.freeze({ try { yield provider.shutdown(); } catch (ex) { - this._log.warn("Error when shutting down provider: " + - CommonUtils.exceptionStr(ex)); + this._recordError("Error when shutting down provider: " + + provider.name, ex); } finally { this._collector.unregisterProvider(provider.name); } @@ -564,6 +565,35 @@ AbstractHealthReporter.prototype = Object.freeze({ }.bind(this)).then(onFinished, onFinished); }, + /** + * Record an exception for reporting in the payload. + * + * A side effect is the exception is logged. + * + * Note that callers need to be extra sensitive about ensuring personal + * or otherwise private details do not leak into this. All of the user data + * on the stack in FHR code should be limited to data we were collecting with + * the intent to submit. So, it is covered under the user's consent to use + * the feature. + * + * @param message + * (string) Human readable message describing error. + * @param ex + * (Error) The error that should be captured. + */ + _recordError: function (message, ex) { + let recordMessage = message; + let logMessage = message; + + if (ex) { + recordMessage += ": " + ex.message; + logMessage += ": " + CommonUtils.exceptionStr(ex); + } + + this._log.warn(logMessage); + this._errors.push(recordMessage); + }, + /** * Collect all measurements for all registered providers. */ @@ -628,8 +658,8 @@ AbstractHealthReporter.prototype = Object.freeze({ payload = yield this.getJSONPayload(asObject); } catch (ex) { error = ex; - this._log.warn("Error collecting and/or retrieving JSON payload: " + - CommonUtils.exceptionStr(ex)); + this._collectException("Error collecting and/or retrieving JSON payload", + ex); } finally { yield this.ensurePullOnlyProvidersUnregistered(); @@ -689,11 +719,6 @@ AbstractHealthReporter.prototype = Object.freeze({ let outputDataDays = o.data.days; - // We need to be careful that data in errors does not leak potentially - // private information. - // FUTURE ask Privacy if we can put exception stacks in here. - let errors = []; - // Guard here in case we don't track this (e.g., on Android). let lastPingDate = this.lastPingDate; if (lastPingDate && lastPingDate.getTime() > 0) { @@ -716,9 +741,8 @@ AbstractHealthReporter.prototype = Object.freeze({ // is aware of the measurement version. serializer = measurement.serializer(measurement.SERIALIZE_JSON); } catch (ex) { - this._log.warn("Error obtaining serializer for measurement: " + name + - ": " + CommonUtils.exceptionStr(ex)); - errors.push("Could not obtain serializer: " + name); + this._recordError("Error obtaining serializer for measurement: " + + name, ex); continue; } @@ -726,9 +750,8 @@ AbstractHealthReporter.prototype = Object.freeze({ try { data = yield measurement.getValues(); } catch (ex) { - this._log.warn("Error obtaining data for measurement: " + - name + ": " + CommonUtils.exceptionStr(ex)); - errors.push("Could not obtain data: " + name); + this._recordError("Error obtaining data for measurement: " + name, + ex); continue; } @@ -736,8 +759,8 @@ AbstractHealthReporter.prototype = Object.freeze({ try { o.data.last[name] = serializer.singular(data.singular); } catch (ex) { - this._log.warn("Error serializing data: " + CommonUtils.exceptionStr(ex)); - errors.push("Error serializing singular: " + name); + this._recordError("Error serializing singular data: " + name, + ex); continue; } } @@ -762,18 +785,15 @@ AbstractHealthReporter.prototype = Object.freeze({ outputDataDays[dateFormatted][name] = serialized; } catch (ex) { - this._log.warn("Error populating data for day: " + - CommonUtils.exceptionStr(ex)); - errors.push("Could not serialize day: " + name + - " ( " + dateFormatted + ")"); + this._recordError("Error populating data for day: " + name, ex); continue; } } } } - if (errors.length) { - o.errors = errors; + if (this._errors.length) { + o.errors = this._errors.slice(0, 20); } this._storage.compact(); diff --git a/services/metrics/collector.jsm b/services/metrics/collector.jsm index faf1e81c417..faee0020006 100644 --- a/services/metrics/collector.jsm +++ b/services/metrics/collector.jsm @@ -32,7 +32,6 @@ this.Collector = function (storage) { this._providerInitQueue = []; this._providerInitializing = false; - this.providerErrors = new Map(); } Collector.prototype = Object.freeze({ @@ -124,12 +123,9 @@ Collector.prototype = Object.freeze({ constantsCollected: false, }); - this.providerErrors.set(provider.name, []); - deferred.resolve(result); } catch (ex) { - this._log.warn("Provider failed to initialize: " + provider.name + - ": " + CommonUtils.exceptionStr(ex)); + this._recordProviderError(provider.name, "Failed to initialize", ex); deferred.reject(ex); } finally { this._providerInitializing = false; @@ -184,15 +180,14 @@ Collector.prototype = Object.freeze({ try { collectPromise = provider[fnProperty].call(provider); } catch (ex) { - this._log.warn("Exception when calling " + provider.name + "." + - fnProperty + ": " + CommonUtils.exceptionStr(ex)); - this.providerErrors.get(provider.name).push(ex); + this._recordProviderError(provider.name, "Exception when calling " + + "collect function: " + fnProperty, ex); continue; } if (!collectPromise) { - this._log.warn("Provider does not return a promise from " + - fnProperty + "(): " + provider.name); + this._recordProviderError(provider.name, "Does not return a promise " + + "from " + fnProperty + "()"); continue; } @@ -231,19 +226,32 @@ Collector.prototype = Object.freeze({ yield promise; this._log.debug("Provider collected successfully: " + name); } catch (ex) { - this._log.warn("Provider failed to collect: " + name + ": " + - CommonUtils.exceptionStr(ex)); - try { - this.providerErrors.get(name).push(ex); - } catch (ex2) { - this._log.error("Error updating provider errors. This should " + - "never happen: " + CommonUtils.exceptionStr(ex2)); - } + this._recordProviderError(name, "Failed to collect", ex); } } throw new Task.Result(this); }.bind(this)); }, + + /** + * Record an error that occurred operating on a provider. + */ + _recordProviderError: function (name, msg, ex) { + let msg = "Provider error: " + name + ": " + msg; + if (ex) { + msg += ": " + ex.message; + } + this._log.warn(msg); + + if (this.onProviderError) { + try { + this.onProviderError(msg); + } catch (callError) { + this._log.warn("Exception when calling onProviderError callback: " + + CommonUtils.exceptionStr(callError)); + } + } + }, }); diff --git a/services/metrics/tests/xpcshell/test_metrics_collector.js b/services/metrics/tests/xpcshell/test_metrics_collector.js index 590326221b4..4aaccfae34e 100644 --- a/services/metrics/tests/xpcshell/test_metrics_collector.js +++ b/services/metrics/tests/xpcshell/test_metrics_collector.js @@ -29,7 +29,6 @@ add_task(function test_register_provider() { do_check_eq(collector._providers.size, 1); yield collector.registerProvider(dummy); do_check_eq(collector._providers.size, 1); - do_check_eq(collector.providerErrors.size, 1); do_check_eq(collector.getProvider(dummy.name), dummy); let failed = false; @@ -52,7 +51,9 @@ add_task(function test_register_provider() { add_task(function test_collect_constant_data() { let storage = yield Metrics.Storage("collect_constant_data"); + let errorCount = 0; let collector = new Metrics.Collector(storage); + collector.onProviderError = function () { errorCount++; } let provider = new DummyProvider(); yield collector.registerProvider(provider); @@ -62,23 +63,24 @@ add_task(function test_collect_constant_data() { do_check_eq(provider.collectConstantCount, 1); do_check_true(collector._providers.get("DummyProvider").constantsCollected); - do_check_eq(collector.providerErrors.get("DummyProvider").length, 0); yield storage.close(); + do_check_eq(errorCount, 0); }); add_task(function test_collect_constant_throws() { let storage = yield Metrics.Storage("collect_constant_throws"); let collector = new Metrics.Collector(storage); + let errors = []; + collector.onProviderError = function (error) { errors.push(error); }; + let provider = new DummyProvider(); provider.throwDuringCollectConstantData = "Fake error during collect"; yield collector.registerProvider(provider); yield collector.collectConstantData(); - do_check_true(collector.providerErrors.has(provider.name)); - let errors = collector.providerErrors.get(provider.name); do_check_eq(errors.length, 1); - do_check_eq(errors[0].message, provider.throwDuringCollectConstantData); + do_check_true(errors[0].contains(provider.throwDuringCollectConstantData)); yield storage.close(); }); @@ -86,15 +88,17 @@ add_task(function test_collect_constant_throws() { add_task(function test_collect_constant_populate_throws() { let storage = yield Metrics.Storage("collect_constant_populate_throws"); let collector = new Metrics.Collector(storage); + let errors = []; + collector.onProviderError = function (error) { errors.push(error); }; + let provider = new DummyProvider(); provider.throwDuringConstantPopulate = "Fake error during constant populate"; yield collector.registerProvider(provider); yield collector.collectConstantData(); - let errors = collector.providerErrors.get(provider.name); do_check_eq(errors.length, 1); - do_check_eq(errors[0].message, provider.throwDuringConstantPopulate); + do_check_true(errors[0].contains(provider.throwDuringConstantPopulate)); do_check_false(collector._providers.get(provider.name).constantsCollected); yield storage.close();