1586922 - Windows system modal dialogs are still reported as hangs

Status: RESOLVED WORKSFORME

(Core :: Widget: Win32, task, P5)

Description

[Florian Quèze [:florian]]

(Florian Quèze [:florian] from bug 1512601 comment #0)

The most common stack currently reported as hangs is when we call
CFileOpenSave::Show (from nsFilePicker::ShowFilePicker). PrintDlgW is also
pretty high in the list. These functions start nested event loops, so the
hangs we are reporting in these cases are false positives.

This is unfortunately still true, despite the patch I landed in bug 1512601.

Doug figured out why:
(In reply to Doug Thayer [:dthayer] from bug 1512601 comment #7)

The problem is right here: https://searchfox.org/mozilla-central/rev/e62c920f7f6463239c6634113f8a8351e263b936/widget/windows/nsWindow.cpp#4714

We do correctly set the BackgroundHangThread into a waiting state, but then the WindowProc gets called and we call NotifyActivity, undoing that wait. I believe the correct thing to do here is to re-enter a waiting state at the end of WindowProc if we were already in a waiting state at the beginning of it.

Comment 1

[Florian Quèze [:florian]]

I just captured a profile of showing the file picker on Windows (I just pressed ctrl+o) and it changed my mind about what's happening: https://perfht.ml/2pXuQQr
This is on a fast laptop with an SSD, and BHR detects 2 hangs of less than 200ms while preparing the dialog, but it doesn't report a hang while waiting for the user to accept or dismiss the dialog. Given that we have short hangs on a fast machine with an SSD, and it looks like the OS code running during that time does I/O, I wonder if the long hangs could just be the OS doing plenty of main thread I/O on slow drives to show the content of the dialog (folder names, icons, ...).

I captured another profile on the same machine with the main thread I/O feature of the profiler enabled, and it confirms that there's A LOT of main thread I/O happening when opening the file picker dialog: https://perfht.ml/2IHRS4j

Doug, does that analysis seem sane? What does it mean for the next step here? Is this bug wontfix? Or should we somehow tell BHR to avoid collecting reports for these stacks that are not actionable for us? Is there a way to do this? Maybe a specific profiler label frame?

Comment 2

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

The thing is - even if the OS is doing I/O on our main thread, we have to be calling NotifyActivity for that to even register, which we can and I think should prevent. Whether it's the OS doing I/O or just something circumstantial with the ordering of our NotifyActivity/NotifyWait calls, it's something out of our hands and the outer NotifyWait (which we call when we start the filepicker's event loop) needs to take precedence.

Comment 3

[Jim Porter (:squib)]

Taking a look at this...

Comment 4

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

Chatted with fqueze about this on Slack and forgot to update the bug. In essence, the most recent plan for this was simply to add a new method pair to the BackgroundHangMonitor: NotifyWaitStart and NotifyWaitEnd, which would essentially supersede any calls to NotifyActivity, so we don't inadvertently wake ourselves up with them. This may not be ideal; it could potentially hide hangs inside the nested event loop. But it's better than the pure noise we're seeing today. Leaving this to Squib to determine once he gets into it whether this solution is appropriate or not, but it's the current recommendation.

Comment 5

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

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 6

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.

Comment 7

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

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 8

[Stephen A Pohl [:spohl] (OOO until 3/25)]

Is this still an issue?

Comment 9

[Florian Quèze [:florian]]

(In reply to Stephen A Pohl [:spohl] (OOO until 6/12) from comment #8)

Is this still an issue?

Maybe... as I wrote in comment 1 I changed my mind about it, and would be willing to resolve as WORKSFORME, but comment 2 and 4 sound like Doug thinks there is (or thought there was) something actionable.

BHR still reports these modal dialog as a top hang, but bug 1677170 should address this and has an assignee.

Comment 10

[Ray Kraesig [:rkraesig]]

Bug 1677170 will address the particular case of the file-picker dialogs, but not the print or color-picker dialogs.

Comment 11

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

Sorry for the delay here. I'm totally fine with a WORSFORME on this. I think there's things we could do here but I don't feel like this is really important enough to spend time thinking about.