1737489 - Nested macOS sheets (modal dialogs) cause app lock-ups

Status: RESOLVED DUPLICATE of bug 1884631

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

Description

[Kai Engert [:KaiE:]]

Bug 1691171 caused a major regression for Thunderbird in several scenarios with nested modal dialogs, and it took us a while to identify the cause.

I have suggested that we back out this change for Thunderbird 91, because we have an imminent need to upgrade the userbase of TB 78 to TB 91, and a lot of functionality is affected by this regression.

For comm-central and future versions, can we try to modify the change from bug 1691171 to be compatible with Thunderbird?

I wonder why we couldn't reproduce the issue with nested modal dialogs in Firefox. For example, when using the certificate manager, it opens a content area dialog, and when opening an additional modal dialog, it's shown as another dialog on top, and isn't causing any problems.

In Thunderbird's scenario however, the first dialog gets hidden, and at the time the topmost dialog closes, the earlier dialog is restored incorrectly (as described in bug 1715740 etc.) and with window lockup issues.

Do you understand why nested modal dialogs in Thunderbird behave differently?

Do you have ideas how we could change the code to avoid those problems?

Comment 1

[Wayne Mery (:wsmwk)]

I'll point out the worst case situation, which is the user can't open Thunderbird and therefore Thunderbird is unusable.

When a user has a primary password, and also has at least one account where the account password is not saved, then both dialogs will pop up. I myself have experienced this. This also affects openpgp users (at least one has been reported). The (non-obvious) workaround is bug 1712980 comment 2.

The other scenarios are less severe only in that they occur while user is actually using Thunderbird. But they still are effectively hangs.

Comment 2

[Markus Stange [:mstange]]

(In reply to Kai Engert (:KaiE:) from comment #0)

Bug 1691171 caused a major regression for Thunderbird in several scenarios with nested modal dialogs, and it took us a while to identify the cause.

What can be done to make these kinds of bugs easier to identify in the future? Does mozregression --app thunderbird not work for this use case?

I have suggested that we back out this change for Thunderbird 91, because we have an imminent need to upgrade the userbase of TB 78 to TB 91, and a lot of functionality is affected by this regression.

See bug 1691171 comment 9 - it's a little late for that.

For comm-central and future versions, can we try to modify the change from bug 1691171 to be compatible with Thunderbird?

We can certainly try, yes! Bug 1691171 was not intended to affect observable behavior.

I wonder why we couldn't reproduce the issue with nested modal dialogs in Firefox. For example, when using the certificate manager, it opens a content area dialog, and when opening an additional modal dialog, it's shown as another dialog on top, and isn't causing any problems.

Firefox has switched to non-native dialogs in many places where it used native macOS sheets in the past, so it's possible that there isn't actually a way anymore to encounter nested sheets in Firefox.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Not a Firefox issue. Happy to consider a low-risk uplift if/when a patch is ready, but this isn't something we're going to actively monitor either.

Comment 4

[Kai Engert [:KaiE:]]

Attachment: Bug 1737489 - Application specific backout of bug 1691171, only for MOZ_THUNDERBIRD. r=mstange

Comment 5

[Kai Engert [:KaiE:]]

Comment on attachment 9247742 [details]
Bug 1737489 - Application specific backout of bug 1691171, only for MOZ_THUNDERBIRD. r=mstange

Beta/Release Uplift Approval Request

• User impact if declined: Thunderbird is very broken on macOS
• Is this code covered by automated tests?: Unknown
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Does not affect Firefox at all, because of #ifdef MOZ_THUNDERBIRD
• String changes made/needed: none

Comment 6

[Kai Engert [:KaiE:]]

The attached fix is only a temporary measure. We'd want to continue working on a better fix, so setting leave-open keyword.

Comment 7

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

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9247742 [details]
Bug 1737489 - Application specific backout of bug 1691171, only for MOZ_THUNDERBIRD. r=mstange

94 is on release now.

Comment 9

[Markus Stange [:mstange]]

(In reply to Kai Engert (:KaiE:) from comment #6)

The attached fix is only a temporary measure. We'd want to continue working on a better fix

Let's do that in a new bug though, so that the tracking flags on this bug make sense.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Did you want to nominate this for ESR91 approval also?

Comment 11

[Kai Engert [:KaiE:]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)

Did you want to nominate this for ESR91 approval also?

I think Magnus is worried that in theory the backout could result in a behavior change, and I think he wants to wait a couple of days for testing with TB nightly and TB beta, before uplifting to 91.

Although I personally see very little risk here. Given this patch only changes macOS, and given TB on macOS is unusable in many ways, I'd consider any potential backout effects as less problematic and something that could be followed up separately.

So, yes most likely we want to request uplift to mozilla-esr91 very soon, but I'd want to wait for Magnus' decision how soon we'll request it.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Note that we're in RC week, so I'm going to have to build the 91.3esr RC Really Soon Now. Not sure if that's a big deal for TB or not (or if y'all will just take a later commit if this lands after that).

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d72f32d61af4
Application specific backout of bug 1691171, only for MOZ_THUNDERBIRD. r=mstange

Comment 14

[Kai Engert [:KaiE:]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)

94 is on release now.

Why this is an argument for not landing into mozilla-beta. Are you unable to make any changes to mozilla-beta this week? Thunderbird is often slightly behind, so if we have to wait until after the next merge, it will mean a delay for being able to roll out this fix for testing with Thunderbird beta users.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Yes, beta is frozen until 95 merges to it next week. This has been standard practice for as long as I can remember.

Comment 16

[Kai Engert [:KaiE:]]

thanks for the explanation!

Comment 17

[Steven Michaud [:smichaud] (Retired)]

(Tangential comment, in reply to part of comment 11)

TB on macOS is unusable in many ways

Sorry to butt in like this. And feel free to take your time answering my question. But I use TB all the time on macOS, and am very happy with it. So I'd like to know more about the issues that make some people consider it unusable. (Besides those related to this bug, of course.)

Comment 18

[Kai Engert [:KaiE:]]

(In reply to Steven Michaud [:smichaud] (Retired) from comment #17)

(Tangential comment, in reply to part of comment 11)

TB on macOS is unusable in many ways

Sorry to butt in like this. And feel free to take your time answering my question. But I use TB all the time on macOS, and am very happy with it. So I'd like to know more about the issues that make some people consider it unusable. (Besides those related to this bug, of course.)

Hi, sorry if my comment sounded too strongly!
I should have explained my thought in a better way.

Because of this lockup bug, you could very easily run in one of the many scenarios that caused Thunderbird to completely lock up. You could then lose emails you're actively working on and that haven't been saved as drafts. And Wayne found a way that for some users, based on their configuration, you wouldn't even be able to start up at all, because you'd get a lockup immediately on startup.

In order to use Thunderbird 91.x+ on macOS with this bug present, you'd have to be very careful and knowledgeable about all the scenarios that trigger the lockup, and would have to avoid them.

I was trying to emphasize the severity of this bug, which is making Thunderbird on macOS very difficult to use, and some users might get the impression that it is completely unusable. That is why this bug also has gotten a very high priority and we worked on it with urgency. I hope that we'll soon be able to release this bugfix.

Comment 19

[Kai Engert [:KaiE:]]

With this bug fixed, I consider Thunderbird very usable.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

Thanks for the reply :-)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9247742 [details]
Bug 1737489 - Application specific backout of bug 1691171, only for MOZ_THUNDERBIRD. r=mstange

Per discussion with Kai on Element, uplifting this to mozilla-release doesn't help the TB team because they don't build any releases from there. Given that, I'm OK with landing this on Beta since the change is scoped to only Thunderbird and doesn't need to ride to release anyway. This will at least enable them to spin a new TB beta with this change included for faster verification.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Pushed to Beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/aeedacfec98685e1e4fc4d760b9fd59e79afe764

Comment 23

[Marian-Vasile Laza]

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

Comment 24

[Magnus Melin [:mkmelin]]

In bug 1699514 comment 27 we have someone reporting a way to reproduce the problem in Firefox. (Bug 1712980.)

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Let's reopen this then to track landing the more general fix.

Comment 26

[Magnus Melin [:mkmelin]]

We haven't seen any problems on beta, so please land on esr91.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Pushed to ESR91:
https://hg.mozilla.org/releases/mozilla-esr91/rev/133cb250fa145cba5cc7ed95f75bb3c1b02e6916

Comment 28

[Kai Engert [:KaiE:]]

I think I'm not the ideal assignee for this.
Could we get help?

Comment 29

[Stephen A Pohl [:spohl]]

Adjusting the priority and severity since a temporary fix has landed for Thunderbird. However, we'd want to get this investigated and fixed since it also affects Firefox.

From bug 1699514 comment 27:

[...] If multiple Master Password dialog boxes are triggered then one of them will become attached/stacked behind the parent window and not be able to be interacted with. Firefox must be force-quit, and the the user must aggressively cancel the Master Password boxes as they open and before they can stack. This behavior can be triggered by having multiple tabs/browser windows open that will need access to the password store, and then restart Firefox. As it reloads the prior state the different tabs/windows will trigger their own Master Password prompt. The video in 1712980 (a Thunderbird bug) shows the behavior.

Comment 30

[Marco Castelluccio [:marco]]

(In reply to Stephen A Pohl [:spohl] from comment #29)

Adjusting the priority and severity since a temporary fix has landed for Thunderbird. However, we'd want to get this investigated and fixed since it also affects Firefox.

From bug 1699514 comment 27:

[...] If multiple Master Password dialog boxes are triggered then one of them will become attached/stacked behind the parent window and not be able to be interacted with. Firefox must be force-quit, and the the user must aggressively cancel the Master Password boxes as they open and before they can stack. This behavior can be triggered by having multiple tabs/browser windows open that will need access to the password store, and then restart Firefox. As it reloads the prior state the different tabs/windows will trigger their own Master Password prompt. The video in 1712980 (a Thunderbird bug) shows the behavior.

Stephen, this doesn't seem a super common use case, maybe we can reassess the severity of this bug?

Comment 31

[Stephen A Pohl [:spohl]]

Per bug 1712980 and bug 1699514, this appears to be fixed (or at least no longer reproducible).

Comment 32

[Kai Engert [:KaiE:]]

Stephen, are you sure you didn't misunderstand this bug?
The issue can no longer be reproduced, because we removed the code that triggers it.

But we're still requiring a fix, that can allow us to undo the workaround - because the current code requires a deprecated API that might go away soon.

See also comment 25, where Ryan suggested to reopen this bug, to track solving the general issue (as far as I can tell, this hasn't happened yet, but I'm happy to be corrected).

Comment 33

[Kai Engert [:KaiE:]]

Please read bug 1755330 for the more detailed explanation.

Comment 34

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

The severity field for this bug is relatively low, S3. However, the bug has 6 See Also bugs.
:spohl, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 35

[Wayne Mery (:wsmwk)]

Bug 1884631 desheets us in beta 126.
Is this reported issue now behaving?

Comment 36

[Kai Engert [:KaiE:]]

Yes, this issue was fixed by bug 1884631.