1461262 - Re-factor JS-exposed TelemetryHistogram:: internal_JSKeyedHistogram_Clear to avoid both races and deadlocks

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Alessio Placitelli [:Dexter]]

In bug 1430531 we de-raced the snapshotting functions for keyed histograms. We still need to apply the same technique to the |internal_JSKeyedHistogram_Keys| [1] and |KeyedHistogram::GetJSKeys| functions, as they both interleave JS and Telemetry core function calls.

In this bug, we need to:

- Change |GetJSKeys| [2] to fill and array of nsTArray<nsCString> instead of generating a JS-friendly structure. The new function should also get a proof of lock as an input argument, e.g. |const StaticMutexAutoLock& aLock|.
- Since GetJSKeys changed the semantics, we might want to rename it to just |GetKeys|.
- In |internal_JSKeyedHistogram_Keys|, while holding the mutex, we should get a copy of the keys using |GetKeys| and convert it to JS-friendly structures outside of the locked section.

To run the tests, one can simply try:

> ./mach test toolkit/components/telemetry/tests/unit

[1] - https://searchfox.org/mozilla-central/rev/a85db9e29eb3f022dbaf8b9a6390ecbacf51e7dd/toolkit/components/telemetry/TelemetryHistogram.cpp#1520
[2] - https://searchfox.org/mozilla-central/rev/a85db9e29eb3f022dbaf8b9a6390ecbacf51e7dd/toolkit/components/telemetry/TelemetryHistogram.cpp#827

Comment 1

[maharsh312]

If it is open I'll like to work on it. Can you assign it to me?

Comment 2

[Alessio Placitelli [:Dexter]]

(In reply to maharsh312 from comment #1)
> If it is open I'll like to work on it. Can you assign it to me?

Hi and welcome! Sure, the bug is up for grabs ;) I've assigned it to you, please let me know if there's anything else that you need or if you have any doubt. Please refer to comment 0 for a description of the required changes.

Comment 3

[Alessio Placitelli [:Dexter]]

Hi maharsh312!

Are you still working on this? Are you blocked on something and need help?

Comment 4

[maharsh312]

I tried to understand the problem but I am not able to. I not able to work on it right now. You can remove it from the assignee. Thank You.

Comment 5

[Alessio Placitelli [:Dexter]]

(In reply to maharsh312 from comment #4)
> I tried to understand the problem but I am not able to. I not able to work
> on it right now. You can remove it from the assignee. Thank You.

No worries, feel free to ask question or reach out if you want to dig deeper into this.

Comment 6

[Thi Huynh]

Hi, I would like to work on this bug.

Comment 7

[Thi Huynh]

Attachment: patch_v1.diff

I have refactored the code and run the test you mentioned. All 39 tests are passed.

Comment 8

[Alessio Placitelli [:Dexter]]

Comment on attachment 8996340 [details] [diff] [review]
patch_v1.diff

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

This looks great, thanks Thi! I just have a little nit. After you fix that, would you mind uploading a new patch and flagging me again for review?

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +1856,4 @@
>      return false;
>    }
>  
> +  for (auto const& key : keys) {

nit: let's use |const auto&| here, changing the position of the qualifier. While I think they are equivalent, this version is coherent to other uses within the Telemetry component (https://searchfox.org/mozilla-central/search?q=const+auto&case=false&regexp=false&path=telemetry%2F)

Comment 9

[Thi Huynh]

Attachment: patch_v2.diff

(In reply to Alessio Placitelli [:Dexter] from comment #8)
> Comment on attachment 8996340 [details] [diff] [review]
> patch_v1.diff
> 
> Review of attachment 8996340 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks great, thanks Thi! I just have a little nit. After you fix that,
> would you mind uploading a new patch and flagging me again for review?
> 
> ::: toolkit/components/telemetry/TelemetryHistogram.cpp
> @@ +1856,4 @@
> >      return false;
> >    }
> >  
> > +  for (auto const& key : keys) {
> 
> nit: let's use |const auto&| here, changing the position of the qualifier.
> While I think they are equivalent, this version is coherent to other uses
> within the Telemetry component
> (https://searchfox.org/mozilla-central/
> search?q=const+auto&case=false&regexp=false&path=telemetry%2F)

I've updated the patch, please help to take a look.

Comment 10

[Alessio Placitelli [:Dexter]]

Comment on attachment 8998174 [details] [diff] [review]
patch_v2.diff

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

This looks good to me, thanks Thi! I think this finally clears out the last raciness in the telemetry histogram code :) Let me push it to try for you, before landing, to make sure it doesn't break anything.

Comment 11

[Thi Huynh]

(In reply to Alessio Placitelli [:Dexter] from comment #10)
> Comment on attachment 8998174 [details] [diff] [review]
> patch_v2.diff
> 
> Review of attachment 8998174 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me, thanks Thi! I think this finally clears out the last
> raciness in the telemetry histogram code :) Let me push it to try for you,
> before landing, to make sure it doesn't break anything.

Thank you! Let's wait to see how well my patch is :) .

Comment 12

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea281b62fed62d291e527f25fd6a0a240c284ba6

Comment 13

[Alessio Placitelli [:Dexter]]

(In reply to Alessio Placitelli [:Dexter] from comment #12)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ea281b62fed62d291e527f25fd6a0a240c284ba6

All green, this looks good to go!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5450f4d94dbf
Re-factor KeyedHistogram::GetJSKeys and internal_JSKeyedHistogram_Clear to avoid both races and deadlocks. r=dexter

Comment 15

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/5450f4d94dbf