1586920 - The dynamic string of label frames is missing in BHR stacks

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Description

[Florian Quèze [:florian]]

Some label frames provide a very useful value (eg. nsObserverService::NotifyObservers provides the notification topic). This value is missing in BHR stacks.

I think the problem is in the code at https://searchfox.org/mozilla-central/rev/b6f088f2f68d2a8ae0b49d6c8fbd7cbd3a65fa5b/toolkit/components/backgroundhangmonitor/ThreadStackHelper.cpp#253-276 that only collects the frame's label and not the associated dynamic string.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

At a glance this would seem trivial to add, "just" concatenate the label and the dynamic string (if present)...
However, as the selected code in comment 0 indicates, we can't allocate memory in this function (because it runs during the "critical" section of profiler_suspend_and_sample_thread()).
In the profiler, the dynamic string is stored in a big pre-allocated buffer. I see that BHR has some kind of pre-allocated HangStack of HangEntry's (defined in an ipdlh), we would need to store the dynamic string in there, but it looks small by default.

cc: Nika and Markus: You have worked around these files, ideas welcome!

Also, in the profiler we don't include the dynamic string when "Privacy" is enabled:
https://searchfox.org/mozilla-central/rev/5cb522c7baba24e55874809e0e206b001494c1e9/tools/profiler/core/ProfileBuffer.cpp#235-236
Should we worry about that in BHR as well?

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Oh, I see now there's an mStackToFill->strbuffer() for just these kinds of dynamic strings, e.g.:
https://searchfox.org/mozilla-central/rev/b6f088f2f68d2a8ae0b49d6c8fbd7cbd3a65fa5b/toolkit/components/backgroundhangmonitor/ThreadStackHelper.cpp#325-341

So it may be trivial-ish after all. 😉

Comment 3

[Alex Thayer [:alexical] (she/her)]

I think we'll have to add specific macros for dynamic strings which we want BHR to include. Because I strongly suspect we won't want to include things like this or this or this, but we probably do want things like this. The lines linked in ThreadStackHelper.cpp specifically mention space-saving, so I think the more sparing we can be with what's included, the better.

Comment 4

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1586920 - Sometimes include dynamic string of label frames in BHR

This adds two AUTO_PROFILER_LABEL_DYNAMIC_... macros and updates select
usages of the old macros to use the new ones. These new macros cause
the dynamic string of the label to be included in BHR stacks.

We don't want to do this all of the time, as in many cases we may not
be interested enough in the dynamic string or it may be sensitive
information, but it is rather important information for certain cases.

This uses the same buffer that we use for the strings for JS frames,
and if we fail to fit into that buffer we just append the raw label.

If the string is too long for our static buffer (128 bytes), we just
leave it truncated, as it should be stable and we may be able to infer
from the truncated form what the full form would be.

Comment 5

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1586920 - Strip non-chrome/resource URLs from BHR r?mconley

This is just a precaution - I can't actually observe any instances of the
dynamic strings added in the parent revision including anything other than
chrome or resource URLs. However, since we are including URLs and since
there's no hard guarantees that we won't accidentally cause, say, a file://
URL to make it in here, it seems like a reasonable precaution to strip
them out.

Depends on D51665

Comment 6

[Alex Thayer [:alexical] (she/her)]

Attachment: BHR Dynamic Strings Data Collection Review

Comment 7

[Chris H-C :chutten]

Comment on attachment 9107268 [details]
BHR Dynamic Strings Data Collection Review

PRELIMINARY NOTES:

Collection of paths and URIs often accidentally include paths we don't want to collect. Please double-check that you're only including chrome:// and resource:// URIs. (looks like you might already be doing this)

Could you please explain the domain over which these Flush type strings and observer topic strings live? Can they be arbitrary strings? Are they known at build-time? Is there any potential for these to contain information we don't want to collect? (like anything a user might provide).

This collection should be documented in the [BHR ping docs](https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/data/backgroundhangmonitor-ping.html). They must be updated to describe this expanded collection.

Comment 8

[Alex Thayer [:alexical] (she/her)]

(In reply to Chris H-C :chutten from comment #7)

Comment on attachment 9107268 [details]
BHR Dynamic Strings Data Collection Review

PRELIMINARY NOTES:

Collection of paths and URIs often accidentally include paths we don't want
to collect. Please double-check that you're only including chrome:// and
resource:// URIs. (looks like you might already be doing this)

Double checked - yes, we are filtering out non-chrome/resource URIs when we
submit the ping, and I can't actually find a path that would lead to us including
one in the first place.

Could you please explain the domain over which these Flush type strings and
observer topic strings live? Can they be arbitrary strings? Are they known
at build-time? Is there any potential for these to contain information we
don't want to collect? (like anything a user might provide).

Flush type strings are specified by an index into a static array of strings. So there
is a finite number enumerable at build time. Observer topic strings on the other
hand are finite as far as I can tell, but this is not verifiable at build time because
it is called from chrome JS. The only assurances I can give that there is nothing
in here that I user might provide is that 1) I've searched through the code for
usages which might include an arbitrary string and can't find any, and 2) I can't
conceive of a use case for such a thing, as you would also have to subscribe to
observer notifications for that arbitrary string in advance. There is a slight chance
that there could be dynamic strings in there, but I think there is a very very low
chance that user provided strings could make it in.

This collection should be documented in the [BHR ping
docs](https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/
data/backgroundhangmonitor-ping.html). They must be updated to describe this
expanded collection.

Updated!

Comment 9

[Chris H-C :chutten]

Comment on attachment 9107268 [details]
BHR Dynamic Strings Data Collection Review

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes. This collection is documented in its [in-tree documentation](https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/data/backgroundhangmonitor-ping.html).

    Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

    If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, Doug Thayer is responsible.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical.

    Is the data collection request for default-on or default-off?

Default on for Nightly only.

    Does the instrumentation include the addition of any new identifiers?

No.

    Is the data collection covered by the existing Firefox privacy notice?

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

No. This collection is permanent.

---
Result: datareview+

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd28a94055a3
Sometimes include dynamic string of label frames in BHR r=nika
https://hg.mozilla.org/integration/autoland/rev/60db8c551c38
Strip non-chrome/resource URLs from BHR r=mconley

Comment 11

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/bd28a94055a3
https://hg.mozilla.org/mozilla-central/rev/60db8c551c38