Closed Bug 1097762 Opened 5 years ago Closed 5 years ago

Clearing flag histograms causes crashes

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: miker, Assigned: froydnj)

References

Details

Attachments

(3 files, 3 obsolete files)

Attached patch gistfile1.diffSplinter Review
STR:
1. Apply the patch.

If it doesn't apply cleanly then make sure that only the following tests are the only ones available in browser.ini:
[browser_telemetry_button_paintflashing.js]
[browser_telemetry_button_responsive.js]

2. ./mach mochitest-devtools browser/devtools/shared/test/

When the second test starts the browser crashes.
Clearing flag histograms didn't work, because flag histograms aren't
"initialized" to all-zeroes, their zero-bucket contains a 1 upon
initialization.  We need to reproduce this for ::Clear().
Attachment #8521498 - Flags: review?(georg.fritzsche)
Actually, since I showed Mike that his patches still had some test failures, what do folks think the correct behavior of flag histograms should be when clear() is invoked:

1) Revert to false, which is what attachment 8521498 [details] [diff] [review] does;
2) Remain as-is, which I think is what Mike expects, assuming I understand his patches correctly.
Flags: needinfo?(mratcliffe)
(In reply to Nathan Froyd (:froydnj) from comment #2)
> Actually, since I showed Mike that his patches still had some test failures,
> what do folks think the correct behavior of flag histograms should be when
> clear() is invoked:
> 
> 1) Revert to false, which is what attachment 8521498 [details] [diff] [review]
> [review] does;
> 2) Remain as-is, which I think is what Mike expects, assuming I understand
> his patches correctly.

I expect you to revert to false.

The patch doesn't quite fix it... after calling clear() any new pings are ignored.

I have attached an xpcshell test that highlights the problem (with your patch applied):

./mach test toolkit/components/telemetry/tests/unit/test_TelemetryFlagClear.js

output:
 0:00.21 PROCESS_OUTPUT: Thread-1 (pid:86265) "Original value:"
 0:00.21 TEST_STATUS: Thread-1 run_test PASS [run_test : 12] "[1,0,0]" == "[1,0,0]"
 0:00.21 PROCESS_OUTPUT: Thread-1 (pid:86265) "Sending ping"
 0:00.21 PROCESS_OUTPUT: Thread-1 (pid:86265) "New value:"
 0:00.21 TEST_STATUS: Thread-1 run_test PASS [run_test : 19] "[0,1,0]" == "[0,1,0]"
 0:00.21 PROCESS_OUTPUT: Thread-1 (pid:86265) "Clearing histogram"
 0:00.21 PROCESS_OUTPUT: Thread-1 (pid:86265) "After clearing:"
 0:00.21 TEST_STATUS: Thread-1 run_test PASS [run_test : 25] "[1,0,0]" == "[1,0,0]"
 0:00.21 PROCESS_OUTPUT: Thread-1 (pid:86265) "After sending ping:"
 0:00.21 TEST_STATUS: Thread-1 run_test FAIL [run_test : 30] "[1,0,0]" == "[0,1,0]"
Flags: needinfo?(mratcliffe)
Comment on attachment 8521498 [details] [diff] [review]
correctly re-initialize flag histograms after clearing them

Review of attachment 8521498 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/chromium/src/base/histogram.cc
@@ +152,5 @@
>    SampleSet ss;
>    ss.Resize(*this);
>    sample_ = ss;
> +
> +  if (histogram_type() == FLAG_HISTOGRAM) {

Can't we just make Histogram::Clear() virtual and override from FlagHistogram?

@@ +154,5 @@
>    sample_ = ss;
> +
> +  if (histogram_type() == FLAG_HISTOGRAM) {
> +    size_t zero_index = BucketIndex(0);
> +    static_cast<FlagHistogram*>(this)->LinearHistogram::Accumulate(0, 1, zero_index);

Also need to reset FlagHistogram::mSwitched.
Attachment #8521498 - Flags: review?(georg.fritzsche)
Summary: Telemetry crashes in tests after clearing histograms → Clearing flag histograms doesn't work
Summary: Clearing flag histograms doesn't work → Clearing flag histograms causes crashes
Attached patch xpcshell testSplinter Review
I didn't realize that we can now use equal etc. in xpcshell tests... updated.
Attachment #8522135 - Attachment is obsolete: true
Indeed, that would be better.  Thanks for the catch on |mSwitched|; that is
probably what was causing the test failures.
Attachment #8521498 - Attachment is obsolete: true
Attachment #8522249 - Flags: review?(georg.fritzsche)
Attachment #8522203 - Flags: review?(nfroyd) → review+
Attachment #8522249 - Flags: review?(georg.fritzsche) → review+
https://hg.mozilla.org/mozilla-central/rev/e0c591e12fea
https://hg.mozilla.org/mozilla-central/rev/44d6420a3cc7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.