1703205 - Frequent hangs in SHAppBarMessage

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Florian Quèze [:florian]]

When looking at the BHR data for the 20210402 nightly, hangs in SHAppBarMessage represent more than %1 of the total reported hang time.

Here is the reported stack:

    NtUserFindWindowEx win32u
    FindWindowW user32
    SHAppBarMessage shell32
    nsLookAndFeel::NativeGetInt(mozilla::LookAndFeel::IntID, int&) xul
    mozilla::widget::AddIDsToMap(nsXPLookAndFeel*, mozilla::widget::FullLookAndFeel*, bool, bool) xul
    std::_Func_impl_no_alloc<`lambda at /builds/worker/checkouts/gecko/widget/RemoteLookAndFeel.cpp:216:39',void,const mozilla::widget::LookAndFeelTheme &,bool>::_Do_call(mozilla::widget::LookAndFeelTheme xul
    nsXPLookAndFeel::WithThemeConfiguredForContent(std::function<void (const mozilla::widget::LookAndFeelTheme &, bool)> const&) xul
    static mozilla::widget::RemoteLookAndFeel::ExtractData() xul
    static mozilla::dom::ContentParent::BroadcastThemeUpdate(mozilla::widget::ThemeChangeKind) xul
    nsPresContext::ThemeChangedInternal() xul
    mozilla::detail::RunnableMethodImpl<(anonymous namespace)::HangMonitorChild *,void ((anonymous namespace)::HangMonitorChild::*)(),0,mozilla::RunnableKind::Standard>::Run() xul
    mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex &> const&) xul

I don't remember seeing these stacks in hang data before; so it's either a recent regression in our code, or something external (OS code, or third party software).
Emilio, it looks like you touched the RemoteLookAndFeel::ExtractData code in this stack recently in bug 1701825. How likely is this to be a regression from bug 1701825?

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Fairly unlikely. If this is an issue, it is more likely to be from e.g. bug 1697607.

Bug 1701825 changed the stack (introduced the AddIDsToMap function), but there should be no meaningful behavior change on Windows from that bug.

Looking at the code, that points to this line of code. We are querying this in order to send it to the content process, but this particular value is only read from the parent process. So we could potentially optimize this out.

Comment 2

[Florian Quèze [:florian]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)

Fairly unlikely. If this is an issue, it is more likely to be from e.g. bug 1697607.

Thanks!

Looking at the code, that points to this line of code. We are querying this in order to send it to the content process, but this particular value is only read from the parent process. So we could potentially optimize this out.

Would you be the right person to work on this? If not, do you have anybody in mind that we could needinfo?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1703205 - Don't query AlertNotificationOrigin from RemoteLookAndFeel. r=cmartin,mstange

Since it's not read by child processes and it seems it might be
expensive to get on windows.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1697607

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b596dd98fa2e
Don't query AlertNotificationOrigin from RemoteLookAndFeel. r=cmartin

Comment 6

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/b596dd98fa2e

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Is this worth uplifting to Beta given where we are in the cycle? It'll need a bit of rebasing if yes.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Beta patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1697607
[User impact if declined]: comment 0
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: no
[Why is the change risky/not risky?]: Not query stuff that isn't needed.
[String changes made/needed]: none

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9214143 [details]
Beta patch

Approved for 88.0b9.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7c9d1751198c