1878568 - Browser hangs after opening multiple save dialogs and cancelling in non-LIFO order

Status: RESOLVED FIXED

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

Description

[Alice0775 White]

Steps to reproduce:

1. Open web page
2. Key holding Ctrl and press S multiple times. i.e., keydown Ctrl, keypress S S S S ... then keyup Ctrl.
3. Repeatedly click the Cancel button to close all dialogs
4. Attempt to interact with the browser

Actual results:
Browser is not responding.
Can't even close the browser.

Expected results:
Browser should be responding.

Comment 1

[Ray Kraesig [:rkraesig]]

Note: the bug is avoided in the rare case that the last file dialog you happen to close is the first file dialog to be created.

(Of course, their creation order isn't necessarily their display order...)

Comment 2

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1878568 - [1/2] Drive-by cleanup: simplify AutoWidgetPickerState r?#win-reviewers

Remove an unnecessary member function from AutoWidgetPickerState.

Additionally, use a safer method of getting the underlying nsWindow than
static_cast, and add some thread-safety assertions in
nsWindow::Picker{Open,Closed}().

No functional changes on the happy path.

Comment 3

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1878568 - [2/2] Eliminate WindowEnabler r?#win-reviewers

Rewrite WindowEnabler to avoid assuming that multiple instances are
created and destroyed in LIFO order.

Depends on D200741

Comment 4

[Ray Kraesig [:rkraesig]]

Confirmed via mozregression to be a regression due to bug 1858225 (as suspected). This should almost certainly be uplifted to Fx123.

Comment 5

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

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

Comment 6

[Donal Meehan [:dmeehan]]

Tracking for release, it likely won't be able to ship in a Fx122 dot release but we should follow for Fx123 at least

Comment 7

[Donal Meehan [:dmeehan]]

:rkraesig I see the patches are pending review, but just mentioning a note on timing.
Fx123 is in the final week of beta, the last beta builds on Friday. This would need to land, make it to central, and get a beta uplift request.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3f5769f3185
[1/2] Drive-by cleanup: simplify AutoWidgetPickerState  r=win-reviewers,yjuglaret
https://hg.mozilla.org/integration/autoland/rev/644beeaced95
[2/2] Eliminate WindowEnabler  r=win-reviewers,yjuglaret

Comment 9

[Ray Kraesig [:rkraesig]]

Comment on attachment 9378425 [details]
Bug 1878568 - [2/2] Eliminate WindowEnabler r?#win-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Possible lockup of individual windows if multiple file-dialogs are opened.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: As described in comment 0.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Probably not risky: behavior is local and easily manually-tested.
• String changes made/needed:
• Is Android affected?: No

Comment 10

[Ray Kraesig [:rkraesig]]

Addendum: While I'm pretty confident in the patch as it stands... on reflection, given the fact that this bug was in Fx122 and hasn't seen any reports during the entire cycle, I have to admit that I don't think there's likely to be much of an impact if it's not uplifted.

Comment 11

[Pascal Chevrel:pascalc]

Let's reevaluate for the planned 123 dot release, this patch isn't in nightly yet and we ship our last beta tomorrow.

Comment 12

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/b3f5769f3185
https://hg.mozilla.org/mozilla-central/rev/644beeaced95

Comment 13

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Reproduced the issue with Firefox 124.0a1 (2024-02-04) on Windows 10x64. The browser is not responding after following the steps from comment 0.
The issue is verified fixed with Firefox 124.0b2 on Windows 10x64. The browser functions as expected after following the steps from comment 0. I have also verified this on macOS 13 and Ubuntu 23.1.

Comment 14

[Pascal Chevrel:pascalc]

Comment on attachment 9378425 [details]
Bug 1878568 - [2/2] Eliminate WindowEnabler r?#win-reviewers

Let's have it ride the train and benefit from more time on pre-release.