Closed Bug 1379763 Opened 2 years ago Closed 2 years ago

Asynchronously fire a "main-thread-hung" observer notification when BHR detects a hang

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: Nika, Assigned: Nika)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

We could use this for our foxfooding addon to trigger profile collection when the browser hangs for >128ms.
Attached patch Add a bhr-thread-hang observer (obsolete) — Splinter Review
Not putting this patch up for review yet. It seems like BHR may be more unreliable than we had previously thought at guessing the duration of a hang. For example, the test which I had attached here hangs for approximately 1000ms, but in my testing it seems like I get reports of the hang only lasting 140ish ms. I think we can probably do better than that.

MozReview-Commit-ID: 4g8VUjZBiYH
Oop, nevermind. That was just a bug in the test (It takes some time after startup until BHR is running, and this test started running before we had finished initializing it).
Asking digitarald for feedback, as they are the ones who wanted this API for something they were working on.

MozReview-Commit-ID: 4g8VUjZBiYH
Attachment #8885366 - Flags: review?(mconley)
Attachment #8885366 - Flags: feedback?(hkirschner)
Attachment #8885002 - Attachment is obsolete: true
Comment on attachment 8885366 [details] [diff] [review]
Add a bhr-thread-hang observer

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

Some drive-by comments, feel free to ignore :)

::: xpcom/threads/BackgroundHangMonitor.cpp
@@ +625,5 @@
> +  SystemGroup::Dispatch("NotifyBHRHangObservers", TaskCategory::Other,
> +                        NS_NewRunnableFunction("NotifyBHRHangObservers", [=] {
> +    nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
> +    if (os) {
> +      // NOTE: Make sure to construct this on the main thread.

It might be clearer to say why, presumably it uses non-thread-safe ref counting?

@@ +626,5 @@
> +                        NS_NewRunnableFunction("NotifyBHRHangObservers", [=] {
> +    nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
> +    if (os) {
> +      // NOTE: Make sure to construct this on the main thread.
> +      nsCOMPtr<nsIHangDetails> hangDetails = new HangDetails(aHangTime, name);

nit: IIRC we prefer RefPtr for concrete classes.

::: xpcom/threads/nsIHangDetails.idl
@@ +19,5 @@
> +
> +  /**
> +   * The name of the thread which hung
> +   */
> +  readonly attribute ACString threadName;

Would we want the stack too? Comment 0 isn't particularly clear on what you're trying to accomplish, but that seems like the most interesting bit.
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #4)
> It might be clearer to say why, presumably it uses non-thread-safe ref
> counting?

Yup. I'll add a comment mentioning that.

> nit: IIRC we prefer RefPtr for concrete classes.

nsIHangDetails is an interface. I could use a RefPtr<HangDetails>, but I wrote the code this way so that we get the implicit cast to nsISupports even if HangDetails in the future gets an ambiguous nsISupports base.

> Would we want the stack too? Comment 0 isn't particularly clear on what
> you're trying to accomplish, but that seems like the most interesting bit.

I was considering including the stack in the future, but the original purpose which digitarald was wanting this for was to trigger the foxfooding addon to capture a profile whenever the browser hangs, so any information at all is just extra.

The reason why I don't include the stack right now is because of 2 reasons:

a) We're planning to change the format of the stack right now, and I'd rather not have to rewrite as much code when we do,
b) We asynchronously process the stack and it might delay reporting this message to the main thread more than would be nice

I'll definitely consider adding stack information (perhaps a promise which resolves to the stack?) if this is something which people actually want in the future.
Comment on attachment 8885366 [details] [diff] [review]
Add a bhr-thread-hang observer

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

Tentative r+... this seems to do what it advertises, though I'm not fully up to speed on our new runnable tagging stuff. If you're not sure that you got that right, you might want someone from the Quantum DOM team to give it a once over.

Thanks!
Attachment #8885366 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) - Digging out of needinfo / review backlog. from comment #6)
> Tentative r+... this seems to do what it advertises, though I'm not fully up
> to speed on our new runnable tagging stuff. If you're not sure that you got
> that right, you might want someone from the Quantum DOM team to give it a
> once over.

I'm pretty confident that I did :) Thanks!
Unfortunately during shutdown we can sometimes leak the runnable passed into
SystemGroup::Dispatch. It is leaked instead of being freed off main thread
because we sometimes are passing data which can only be freed on the main thread
safely to the main thread, and running the destructor on the wrong thread could
be really bad.

This is a really really gross workaround for that issue which helps to avoid the
XPCOM leak checker failures which were appearing on try.

MozReview-Commit-ID: GTfdxKnsTae
Attachment #8885832 - Flags: review?(bugs)
Comment on attachment 8885832 [details] [diff] [review]
Part 2: Avoid leaking the bhr-thread-hang runnable when shutting down

This is indeed ugly. Dispatch taking already_AddRefed wasn't perhaps the best idea.
Attachment #8885832 - Flags: review?(bugs) → review+
Could you at least make this a bit less ugly by keeping a strong reference to the runnable. Just to ensure it stays alive.
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbb7c080c1df
Part 1: Add a bhr-thread-hang observer, r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/633c98dc992f
Part 2: Avoid leaking the bhr-thread-hang runnable when shutting down, r=smaug
Comment on attachment 8885366 [details] [diff] [review]
Add a bhr-thread-hang observer

LGTM. I will file a big about exposing it as an event on geckoProfiler (or wait for better ideas come up, as this is needed for a web extension)
Attachment #8885366 - Flags: feedback?(hkirschner) → feedback+
https://hg.mozilla.org/mozilla-central/rev/dbb7c080c1df
https://hg.mozilla.org/mozilla-central/rev/633c98dc992f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.