1661127 - Taskbar menu item 'Close all windows' only closes one window (from firefox 77.0+)

Status: VERIFIED FIXED

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

Description

[u669010]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:80.0) Gecko/20100101 Firefox/80.0

Steps to reproduce:

Open multiple windows in firefox.
Right-click on the firefox icon in the taskbar and select close all windows.

Actual results:

Only one windows closed instead of all windows (version 77.0+)

Version 76.0b8 or lower correctly closed all windows.

Expected results:

Version 77.0+ didn't close all windows.

I've bisected the version of firefox. This stopped working in version 77.0, but worked in version 76.0b8 and lower.

Comment 1

[BugBot [:suhaib / :marco]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b2f426661f29c62797aabc51f57add257f40eec2&tochange=8d31ce849dfba1102adb28e29feffcdfced2610c

Comment 3

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:jimm, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[Jim Mathies [:jimm]]

Mike, looks like a regression from your work. Can you take a look?

Comment 5

[Jim Mathies [:jimm]]

I returned early from all three calls to nsWindow and the bug went away, so confirming bug 890125 regressed this. Although it's probably just exposing a bug in the virtual desktop code in front-end, vs. something with the calls in widget.

Comment 6

[Mike de Boer [:mikedeboer]]

Alright, thanks Jim, for narrowing down the regression! I'll take a look asap!

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Changing severity to S3 because of increasing number of complaints.

Comment 8

[David Parks [:handyman]]

The issue is that IVirtualDesktopManager methods take a very, very long time and Windows decides that our UI thread is too busy to send WM_CLOSE messages for more windows. I don't know why it decides this is the right behavior but shoving the WM_CLOSE handler into a runnable to run async "fixes" the issue, although it's not intended as a real solution. (It also didn't fix the issue -- I went from closing 10 out of 20 windows to closing 19 out of 20, but I think this may be a general perf issue.)

The key is to cache the workspace GUID for GetWorkspaceId calls and to handle MoveWindowToDesktop calls asynchronously. There are a bunch of ways to do this but we already essentially poll this when windows are moved or whatever, which is how workspace changes are supposed to be detected on Windows. I don't know if other platforms need that, but it seems to be done by agnostic JS code. If they don't need it then we can probably more efficiently update during specific windows messages (like WM_KILLFOCUS) and/or browser events (we only ever care about this value in SessionStore, at shutdown and at startup).

Probably the easiest robust solution is to keep the setup as-is and use the GetWorkspaceId calls to initiate an async call to Windows' GetWorkspaceId but immediately returning the cached (possibly stale) value. We can throttle and even delay the task to consolidate a stream of successive, redundant calls into one (which is the common case in practice).

(MoveWindowToDesktop could just update the cache value and run async, although it is not part of this bug -- that would just be for responsiveness of its threads, which is not currently known to be a problem. It is only used at browser-start.)

Comment 9

[David Parks [:handyman]]

Attachment: Bug 1661127: Cache Windows IVirtualDesktopManager::GetWindowDesktopId value r=cmartin!

GetWindowDesktopId is a slow function and is called on the UI thread during WM_CLOSE message handing (as part of SessionStore). This was causing Windows to assume we are stalled and it abandons some messages, namely future WM_CLOSE messages sent for other windows during a "Close All Windows" operation.

In this patch, GetWorkspaceId uses a cached value for GetWindowDesktopId (if available) but also queues an update for that cache value if one isn't already in progress. Since SessionStore already regularly calls GetWorkspaceId in response to window moves, this keeps the value fresh in plases where it might change. (The SessionStore architecture is designed that way because Windows does not notify the application if the user moves the window to another virtual desktop.)

Comment 10

[David Parks [:handyman]]

Attachment: Bug 1661127: Cache Windows IVirtualDesktopManager::GetWindowDesktopId value r=cmartin!

GetWindowDesktopId is a slow function and is called on the UI thread during WM_CLOSE message handing (as part of SessionStore). This was causing Windows to assume we are stalled and it abandons some messages, namely future WM_CLOSE messages sent for other windows during a "Close All Windows" operation.

In this patch, GetWorkspaceId uses a cached value for GetWindowDesktopId (if available) but also queues an update for that cache value if one isn't already in progress. Since SessionStore already regularly calls GetWorkspaceId in response to window moves, this keeps the value fresh in plases where it might change. (The SessionStore architecture is designed that way because Windows does not notify the application if the user moves the window to another virtual desktop.)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/542710f708e3
Cache Windows IVirtualDesktopManager::GetWindowDesktopId value r=cmartin

Comment 12

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/542710f708e3

Comment 13

[Giorgia Nichita, Release Desktop QA]

Reproduced the issue on an affected Nightly build 82.0a1 (2020-08-25), Win 10 x64.
Verified fixed on Firefox 96.0 (20220106144528) on Win 10 x64. All windows were closed.