1006771 - Telemetry Experiments: respect AddonManager's shutdown mechanism?

Status: RESOLVED DUPLICATE of bug 1012466

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Benjamin Smedberg]

In bug 995027, Irving suggests that we should "change your shutdown mechanism to use the AddonManager's shutdown() callback, rather than using AsyncShutdown directly".

I think I disagree, in that the experiment system is not always tied to the addon manager. However, it may make sense to at least unhook the global experiment provider as an addon provider when the addon manager informs us that it is shutting down.

Also, "Experiments.jsm also throws away a lot of exceptions; I'd like to see more _log.warn() calls in your catch clauses."

I see one place which should probably either have more specific catch clauses or should have better logging: 

http://hg.mozilla.org/mozilla-central/annotate/8bd82defae82/browser/experiments/Experiments.jsm#l1116

In every other case we either have a very specific catch clause for expected errors, or we're already logging. Irving did you have some other location in mind?

Comment 1

[:Irving Reid (No longer working on Firefox)]

I'm OK with using AddonManager's shutdown to just detach (which you don't need to do explicitly; AddonManager forgets about your provider after your shutdown() resolves). The important thing is that any attempts to use AddonManager (including its provider deregistration API) could end up racing with AM shutting itself down, and having AM call the provider's shutdown() method is the reliable way to synchronize that.

For logging, the ones I spotted were actually Promise then() error handlers, for example http://hg.mozilla.org/mozilla-central/annotate/8bd82defae82/browser/experiments/Experiments.jsm#l2061, but looking closer those always call back to our internal code (the AsyncObjectCaller in AddonManager.jsm), and we exception-wrap and log the externally provided callback there, so that code should only see exceptions if the whole AddonManager is badly broken.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Bug 1012466 shows an actual issue we ran into around this. Not sure which bug to dupe against which, favoring the newer one with an actual issue.