Telemetry review of the simple measures AMI_startup_end, XPI_startup_end, and XPI_bootstrap_end show that in roughly 1 in 10 000 Firefox Nightly / Windows sessions, one of these phases of the start up process does not complete - the _end timestamp is not in the telemetry packet, even though other later timestamps are present. A triage pass over bugs in the Add-on Manager component didn't find any obvious reports of problems, so I'd like to record an exception summary in the simpleMeasures/addonManager section to see if we can identify any problems based on reports from the field. Open question: do we record a one line entry, exception text and file/line, or a full stack? Note that Android telemetry does include some stacks (e.g. bug 826053, for Application Not Responding events) but these are Java, not JS.

Comment on attachment 8350731 [details] [diff] [review] Catch exceptions in several places and record in telemetry Review of attachment 8350731 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/AddonManager.jsm @@ +469,5 @@ > * Initializes the AddonManager, loading any known providers and initializing > * them. > */ > startup: function AMI_startup() { > + try { Nice for review but before landing make the indent right @@ +576,5 @@ > Components.utils.import(url, {}); > } > catch (e) { > + AddonManagerPrivate.recordSimpleMeasure("exception", > + LogManager.formatLogMessage("ERROR", "AMI", "provider " + url + " load failed", e)); These exceptions are already caught, doesn't that mean they can't be the cause of problems? @@ +599,5 @@ > delete this.startupChanges[type]; > } > > + this.foo.bar(); > + I suspect you didn't mean to include this in the patch

Comment on attachment 8350731 [details] [diff] [review] Catch exceptions in several places and record in telemetry Review of attachment 8350731 [details] [diff] [review]: ----------------------------------------------------------------- Aside from dtownsend's remarks, looks good

I was just wrapping up testing my cleanup from these comments when I remembered bug 661982, and figured that if I'm helping to get rid of AddonLogging.jsm I'd better not add a new dependency on it in this patch. I changed things to record the exception details as a JSON blob rather than a string. (In reply to Dave Townsend (:Mossop) from comment #2) > Review of attachment 8350731 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +576,5 @@ > > Components.utils.import(url, {}); > > } > > catch (e) { > > + AddonManagerPrivate.recordSimpleMeasure("exception", > > + LogManager.formatLogMessage("ERROR", "AMI", "provider " + url + " load failed", e)); > > These exceptions are already caught, doesn't that mean they can't be the > cause of problems? Yes, but I figured an exception here is serious enough that we'd be interested to find out about it. > @@ +599,5 @@ > > delete this.startupChanges[type]; > > } > > > > + this.foo.bar(); > > + > > I suspect you didn't mean to include this in the patch Oops, that was how I caused the exceptions for manual testing...

Comment on attachment 8356781 [details] [diff] [review] Record exceptions in telemetry, without depending on AddonLogging.jsm Review of attachment 8356781 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/AddonManager.jsm @@ +2239,5 @@ > + report.file = aException.fileName; > + report.line = aException.lineNumber; > + } > + } > + Nit: Trailing whitespace

Carrying forward Dave's r+ in comment 5

This doesn't need a build of its own

Backed out in https://hg.mozilla.org/integration/fx-team/rev/2645530519bb for xpcshell failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=32720598&tree=Fx-Team

Bah, test_shutdown.js / test_functions() assumes that every method in AddonManager and AddonManagerPrivate should throw a NOT_INITIALIZED exception, unless the method is explicitly ignored in the test. This is the n+1'th time I've been burned by this. Updated patch just adds AddonManagerPrivate.recordException() to the ignore list in test_shutdown.js; rs=mossop in IRC #fx-team.