1833841 - 'Close all windows' option from taskbar icon context menu not working reliably

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect, P3)

Description

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Notes

• This issue happens in most of the cases only when opening a PBM window with the keyboard shortcut(CTRL+SHIFT+P), but it can also happen intermittently when opening PBM windows from the context menu.
• This issue happens whilst closing normal browsing mode windows as well.

Found in

• Beta 114.0b5;

Affected versions

• Beta 114.0b5;
• Nightly 115.0a1;

Tested platforms

• Windows 11;
• Windows 10;
• Ubuntu 22;
• macOS12;

Affected platforms

• Windows 11;
• Windows 10;

Unaffected platforms

• macOS;
• Ubuntu;

Preconditions

• Have a Firefox .exe build installed on your machine.

Steps to reproduce

1. Launch Firefox.
2. Open a few Private Windows using either the keyboard shortcut or the context menu from the taskbar icon.
3. Right click on the icon on the taskbar and select ‘Close all Windows’.

Expected result

• All windows close.

Actual result

• Not all windows close.

Regression range

• Will look for one ASAP.

Comment 1

[BugBot [:suhaib / :marco]]

:oardelean, if you think that's a regression, could you try to find a regression range using for example mozregression?

Comment 2

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Attachment: fx screen recording.webm

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Hey bhearsum, I know back in the day you worked on the separate private browsing window taskbar thing... do you know who I'd point this to these days?

Comment 4

[[e-mail] (:bhearsum)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #3)

Hey bhearsum, I know back in the day you worked on the separate private browsing window taskbar thing... do you know who I'd point this to these days?

nrishel is probably the right person to start with these days.

Comment 5

[:Gijs (he/him)]

I think this menu just invokes the app startup's quit logic - either that or the win32 widget windows all receive WM_CLOSE, in https://searchfox.org/mozilla-central/rev/0c2945ad4769e2d4428c72e6ddd78d60eb920394/widget/windows/nsWindow.cpp#5648-5651 or similar, so this belongs in widget. Either way, not in the frontend menus component - we don't create these menus or even localize them.

Comment 6

[Dave Townsend [:mossop]]

(In reply to :Gijs (he/him) from comment #5)

I think this menu just invokes the app startup's quit logic - either that or the win32 widget windows all receive WM_CLOSE, in https://searchfox.org/mozilla-central/rev/0c2945ad4769e2d4428c72e6ddd78d60eb920394/widget/windows/nsWindow.cpp#5648-5651 or similar, so this belongs in widget. Either way, not in the frontend menus component - we don't create these menus or even localize them.

I can reproduce this and the way it appears to be implemented is that Windows does send the WM_CLOSE message to the windows when clicking on this menu option. But in the cases that I managed to reproduce the OS only sent WM_CLOSE to some of the windows.

These steps seem to reproduce the issue very reliably for me:

1. Open Firefox normally.
2. From the app menu click "New private window"
3. From the app menu of the new window click "New private window"
4. From the app menu of the new window click "New private window"

(You now have one normal window and three private windows open)

5. Right click the private icon in the taskbar and click "Close all windows"

The first and third private window close, the second remains open.

Comment 7

[Dave Townsend [:mossop]]

(In reply to Dave Townsend [:mossop] from comment #6)

(In reply to :Gijs (he/him) from comment #5)

I think this menu just invokes the app startup's quit logic - either that or the win32 widget windows all receive WM_CLOSE, in https://searchfox.org/mozilla-central/rev/0c2945ad4769e2d4428c72e6ddd78d60eb920394/widget/windows/nsWindow.cpp#5648-5651 or similar, so this belongs in widget. Either way, not in the frontend menus component - we don't create these menus or even localize them.

I can reproduce this and the way it appears to be implemented is that Windows does send the WM_CLOSE message to the windows when clicking on this menu option. But in the cases that I managed to reproduce the OS only sent WM_CLOSE to some of the windows.

These steps seem to reproduce the issue very reliably for me:

1. Open Firefox normally.
2. From the app menu click "New private window"
3. From the app menu of the new window click "New private window"
4. From the app menu of the new window click "New private window"

(You now have one normal window and three private windows open)

5. Right click the private icon in the taskbar and click "Close all windows"

The first and third private window close, the second remains open.

If you change focus to the normal window between steps 4 and 5 then all the private windows close correctly. I'm not sure what to make of this, as far as I can tell from Spy++ all of the windows are configured the same as far as Windows is concerned.

Comment 8

[Daniel Bodea [:danibodea]]

I have attempted to investigate the appearance of this issue, but mozregression would not go further than the range of a day due to not having enough data to bisect. Unfortunately, this result seems trustworthy because it was done twice and gave the same result.

Results:
Tested mozilla-central build: 2020-04-07 (verdict: g)
Tested mozilla-central build: 2020-04-08 (verdict: b)

Conclusion: This issue has started reproducing since the first build of Nightly v77.0a1 from 2020-04-08, but a specific regressor could not be determined.

Comment 9

[:Gijs (he/him)]

(In reply to Daniel Bodea [:danibodea] from comment #8)

I have attempted to investigate the appearance of this issue, but mozregression would not go further than the range of a day due to not having enough data to bisect. Unfortunately, this result seems trustworthy because it was done twice and gave the same result.

Results:
Tested mozilla-central build: 2020-04-07 (verdict: g)
Tested mozilla-central build: 2020-04-08 (verdict: b)

Conclusion: This issue has started reproducing since the first build of Nightly v77.0a1 from 2020-04-08, but a specific regressor could not be determined.

Could you provide a pushlog, or the build IDs for the builds in question? There are two nightlies on both of those days... Even an m-c window might give us some clues...

Comment 10

[Daniel Bodea [:danibodea]]

Unfortunately, no. Mozregression gave out the error that says it cannot continue because it does not have enough data to bisect. When this happens, the results provided in comment 8 is all the information mozregression provides. I am unsure which build mozregression takes when it downloads a specific day's build, but I am pretty sure it points to the first build of the day.
This pushlog should should include the regressor:
https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2020-04-07&enddate=2020-04-08
(In reply to :Gijs (he/him) from comment #9)

Could you provide a pushlog, or the build IDs for the builds in question? There are two nightlies on both of those days... Even an m-c window might give us some clues...

Comment 11

[:Gijs (he/him)]

(In reply to Daniel Bodea [:danibodea] from comment #10)

Unfortunately, no. Mozregression gave out the error that says it cannot continue because it does not have enough data to bisect. When this happens, the results provided in comment 8 is all the information mozregression provides. I am unsure which build mozregression takes when it downloads a specific day's build, but I am pretty sure it points to the first build of the day.

I think it should give you a revision hash when it runs the individual build?

Anyway, from the list, off-hand bug 1616353 seems like it's plausibly related here. Nika?

Comment 12

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Gijs (he/him) from comment #10)

Anyway, from the list, off-hand bug 1616353 seems like it's plausibly related here. Nika?

While that bug had a lot of changes related to opening windows (specifically pop-up windows from content), it shouldn't have touched anything at the widget layer - it was purely operating at the DOM level. I can't think of any way it could be related to Windows not sending the WM_CLOSE method to all windows when the option is selected from the task bar. This seems far more likely to be some issue on the widget side to me. A bug like https://bugzilla.mozilla.org/show_bug.cgi?id=1627505 was landed in that timeframe which did touch the widget layer (though I don't know if it could possibly be related, as I don't know much about how this stuff works under the hood). Something like https://bugzilla.mozilla.org/show_bug.cgi?id=1630361 also touched our windows.h wrapper, though I can't imagine how it would be causing this kind of issue.

Comment 13

[Daniel Bodea [:danibodea]]

I have remade the investigation 2 more times:

• the first time I got different results:
  **Tested mozilla-central build: 2020-04-05 (verdict: g)
  **Tested mozilla-central build: 2020-04-06 (verdict: b)
• the second time I got the same results as before and I took the build ID of the last affected build in the investigation.
  **Version 77.0a1
  **Build ID 20200408214238

I hope this helps in determining the actual regressor.

Comment 15

[brad]

[Tracking Requested - why for this release]: Problem in 126.0.1 (64-bit) OS Version: 10.0.19045 N/A Build 19045

Comment 16

[Donal Meehan [:dmeehan]]

Rejecting tracking since this is an older issue not new in 126.
For context, release tracking tracks issues that will be fixed in an upcoming dot release, blockers during a cycle, etc.

Comment 17

[Greg Stoll :gstoll]

I can easily reproduce this - with three private windows, only one gets closed, and I hear a Windows system sound. I suspect this is bug 1661127 again (thanks :handyman!), and that we're simply taking too long to process WM_CLOSE. I'll take a look.

Comment 18

[Greg Stoll :gstoll]

Yup, we're calling IVirtualDesktopManager::GetWindowDesktopID() on the main thread while processing WM_CLOSE. If I take that call out then all three private windows close correctly.

I know this is a long-standing issue, but bug 1969799 also changed some code around here recently.

In my tests I never moved these windows to a different virtual desktop; I wonder if simply initializing nsWindow::mDesktopId when it's created will make this problem go away, since we won't try to do a synchronous update?

Comment 19

[Greg Stoll :gstoll]

One trickiness here is that session restore will save state when idle, which does populate the mDesktopId. If I open a few private windows then wait a little bit, that gets called and then Close All Windows does work as expected.

So I'm not sure how big a problem this is in practice. But I still like the idea of asynchronously updating the workspace ID when a window gets created.

Comment 20

[Greg Stoll :gstoll]

Attachment: Bug 1833841 - make "close all windows" work more reliably r=#win-reviewers!

"Close all windows" sends WM_CLOSE messages serially to the open windows.
If we take too long to process the WM_CLOSE message, Windows will give
up and stop sending messages to subsequent windows.

In this case, getting the workspace ID was taking too long and triggering
this behavior. This change makes it so when a window is created, it queues
an update to get its workspace ID so mDesktopId won't be empty when we
try to close it, and so we won't do a synchronous update.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/af17369b9496
https://hg.mozilla.org/integration/autoland/rev/151998d5170f
make "close all windows" work more reliably r=win-reviewers,handyman

Comment 22

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/151998d5170f

Comment 23

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Reproducible on Firefox 143.0 on Windows 10.
Verified as fixed on Firefox 145.0a1 and Firefox 144.0b1 on Windows 10 and Windows 11.