Review changes done by Bug 41565

While trying to uplift #41565 (closed)/!505 (merged), I got some feedback, see Bug 1829140.

• fission.experiment.* has gone in Bug 1671548
• browser.js changes are done to avoid calls to histogram functions that become a no-op with telemetry disabled
  • not a good approach for Mozilla: the assumption is that all the telemetry checks are done by these functions, not by the caller
  • after all, not good also for us: we don't patch any other block of calls (unless it's for other reasons, such as the download code, in which we drop an entire telemetry block because our patch changes several scopes)
  • as a result, dropping these changes seems a sensible option
• for BrwoserGlue, I've been asked to move these changes closer to where the writing actually happens
  • possibly document the reasons for which the usual Firefox's assumptions (call Telemetry, it will check if it's allowed to run instead) don't hold
  • makes sense, and I'm doing it also for the C++ changes (i.e., not change nsAppRunner.cpp, but WriteFailedProfileLock in Telemetry.cpp)
  • reportInstallationTelemetry doesn't actually write anything: it depends on installation_telemetry.json, written by the installer. We don't add that file, so as a matter of fact, we don't need to do anything on that function. It looks like a performance improvement, but upstream says that they should be measurable, and if they are, probably the code with telemetry enabled should be improved, instead of having these changes
    • it could make sense to drop this change, too. I will comment upstream, to deepen the performance improvement argument (it's async stuff, so it should not be that heavy)
  • _collectStartupConditionsTelemetry does not write anything on disk directly, either, so upsteram is less happy to take it.
    • In general, disk leak of times is a very weak argument, because there are probably plenty of other ways to get the same information.
• for times.json, I've kept thinking about it for a few days, and I think it was a misunderstanding on our side. times.json is used also for profile health and something for the Activity Stream. But, as a coincidence, the only piece of code calling ProfileAge() in Tor Browser was that telemetry file.
  • I'm leaning toward not patching it anymore. What is its role in the security treat model, exactly?
  • times.json is created with the profile, so we still have the profile creation time. Also, if we didn't have it, there would probably be some file leaking some date.
  • I don't think fighting timestamps is worth the issue. There are several other things that change them, including preferences and even our Onion Aliases patch.
    • This can be disabled, but I'm sure many more remain.
  • I don't think we need profile health at the moment, neither in Tor Browser, nor in Mullvad Browser, since we should hardly write anything on them. Maybe in the future we might, but if we have a stronger reason to disable timestamp collection, I'd be up for just disabling it.
  • The I/O argument hardly holds, as it's implemented in an async way. Also, performance improvements should be measured/measurable to make sure they're worth it (and upstream should improve the code in the first place, if that's the case).

I'd be happy if upstream took the disk changes. But after having re-reviewed the patch, I think I'd be okay in making it lighter even if they didn't (i.e., take what I proposed upstream).