STR: 1. Launch Firefox (Nightly 40 & Aurora 39 are both affected), don't do anything that would trigger a new Telemetry subsession 2. Open about:telemetry 3. Compare the values of any subsession and full-session histograms, e.g. GC_MS The subsession histogram will have 1 more value than the full-session histogram

From my debug log: **** DEBUG Adding to GC_MS (1) **** DEBUG Cloning GC_MS (2) **** DEBUG Adding to sub#GC_MS (3) We're adding to the existing histogram, then cloning it to create the subsession histogram, then adding to the subsession histogram. Step (1) should happen after creating the subsession histogram. Keyed histograms already behave like that.

Comment on attachment 8597950 [details] [diff] [review] bug1158251 Review of attachment 8597950 [details] [diff] [review]: ----------------------------------------------------------------- Can we have test-coverage for this? ::: toolkit/components/telemetry/Telemetry.cpp @@ +1085,5 @@ > if (Histogram* subsession = GetSubsessionHistogram(histogram)) { > subsession->Add(value); > } > #endif > + // It is safe to add to the histogram now: the subsession histogram was already Nit: Line-break before this.

Adds test coverage.

Comment on attachment 8597978 [details] [diff] [review] bug1158251 - v2 Review of attachment 8597978 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/tests/unit/test_nsITelemetry.js @@ +613,5 @@ > let h = Telemetry.getHistogramById(ID); > let flag = Telemetry.getHistogramById(FLAG); > > + // Instantiate the subsession histogram through |add| and make they match. > + h.add(1); This only instantiates the subsession histogram on first use (not after being cleared). I don't think we have a useful way to assert this scenario, so we should either: * make it very clear in a comment that this should be the first use of TELEMETRY_TEST_COUNT in this file * or put in a separate test file Also, are keyed histograms prone to the same issue?

Comment on attachment 8598011 [details] [diff] [review] bug1158251.patch - v3 Review of attachment 8598011 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/tests/unit/test_nsITelemetry.js @@ +614,5 @@ > let flag = Telemetry.getHistogramById(FLAG); > > + // Instantiate the subsession histogram through |add| and make sure they match. > + // This MUST be the first use of "TELEMETRY_TEST_COUNT" in this file, otherwise > + // |add| will not instantiate the histogram. Ok, last request: Lets move this check into say test_instantiate() and call this as the very first test function from run_test(). This should make it much less likely to be a footgun.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d87abe47fd51

(In reply to Georg Fritzsche [:gfritzsche] from comment #4) > > Also, are keyed histograms prone to the same issue? No, they behaved correctly already.

Fixes the broken Android tests. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6fd09cea65f

Needs rebasing.

Rebased