1586061 - hidden window churn causes app menu to stop working

Status: RESOLVED FIXED

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

Description

[Mark Smith [:mcs]]

Attachment: tor-31607.patch

Tor Browser 9.0a7 (available at https://dist.torproject.org/torbrowser/9.0a7/) is based on Firefox ESR68. One of the components of Tor Browser is Tor Launcher, a XUL/XPCOM extension that is integrated into the browser similar to how pdfjs is integrated. Tor Launcher is a weird beast: to interact with the user for configuration and to ensure that the Tor proxy is up and running before a browser window opens, Tor Launcher opens a modal window when it receives a profile-after-change notification. On macOS at least, this causes a hidden window to be opened before the call to appStartup->CreateHiddenWindow() inside XREMain::XRE_mainRun() (which is how the one and only hidden window is typically opened).

The problem comes in when the second hidden window is opened from XREMain::XRE_mainRun(): that causes the original hidden window to be freed. Unfortunately, that causes the nsMenuBarX object that is associated with the app menu to be freed which in turn causes all of the app menu items to stop working (the nsMenuBarX destructor clears the menuGroupOwner property within each Cocoa menu items's MenuItemInfo object , and this breaks the link that is necessary for NativeMenuItemTarget's menuItemHit method to dispatch a menu item event).

Our proposed fix for Tor Browser is to add a check to ensure that the hidden window is only created once. We would like to get feedback on this approach; see the attached patch.

See also: https://trac.torproject.org/projects/tor/ticket/31607

Comment 1

[Mark Smith [:mcs]]

It is possible this is a duplicate of bug 1534965 (but it is difficult to tell for sure).

Comment 2

[Tom Ritter [:tjr]]

Poking the Mac folks I know - thanks in advance!

Comment 3

[Haik Aftandilian [:haik]]

Sorry, I'm not the right person to review this.

Comment 4

[Stephen A Pohl [:spohl]]

Markus, this seems like it would be right in your wheelhouse. Let me know if you'd like me to take this instead.

Comment 5

[Georg Koppen]

Markus: any chance to look at this patch soonish? We lack the macOS experience to thoroughly review the approach but will ship the patch in our upcoming alpha to give it a bit more testing until we need to build Tor Browser 9 (based on 68.2.0esr) next week. Having some feedback by then would be neat.

Comment 6

[Mark Smith [:mcs]]

Attachment: patch to add an extension that demonstrates the bug

Revisiting this bug. I confirmed that it still exists with the current mozilla-central code running on macOS 10.15.5. This patch adds a built-in extension that opens the migration wizard window at profile-after-change time. When you see the migration wizard, just click Cancel and allow the browser to finish starting up. Notice that choosing any item from the application menu (or pressing its associated command key) has no effect (e.g., choosing Quit and pressing Cmd+Q do not do anything).

I also confirmed that checking for mHiddenWindow inside nsAppShellService::CreateHiddenWindow() and returning early when mHiddenWindow is set is still an effective fix.

Comment 7

[Georg Koppen]

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

Markus, this seems like it would be right in your wheelhouse. Let me know if you'd like me to take this instead.

I'd interpret the two years' silence by Markus as a "yes, please take it instead of me". What do you think, Stpehen? The patch we propose is a 2-3 liner which is working fine for us, so it might not be too difficult to review. As a bonus point we'd carry yet another patch less in our patch set if we'd finally be able to upstream it.

Comment 8

[Stephen A Pohl [:spohl]]

Attachment: Bug 1586061: Prevent additional hidden windows from being created if one already exists on macOS. r=mstange

Comment 9

[Stephen A Pohl [:spohl]]

Thanks for the ping. I sent this for review and we'll iterate on it there, if necessary.

Comment 10

[Markus Stange [:mstange]]

Oh wow, yeah this definitely slipped through the cracks. Thanks for the ping.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15b61919eb33
Prevent additional hidden windows from being created if one already exists on macOS. r=mstange

Comment 12

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/15b61919eb33

Comment 13

[Georg Koppen]

Thanks everyone, much appreciated. Stephen: would you be amenable to uplift this patch to ESR 91 as that would mean we'd have to carry one patch less in our patch set? If so, could you file the respective request? The patch seems simple enough...

Comment 14

[Stephen A Pohl [:spohl]]

Comment on attachment 9255765 [details]
Bug 1586061: Prevent additional hidden windows from being created if one already exists on macOS. r=mstange

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Tor is carrying an extra patch with this workaround in their patch set. This would avoid the need of doing so.
• User impact if declined: n/a
• Fix Landed on Version: 97
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Tor has been using this workaround for approximately 2 years. The change consists of only three lines.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9255765 [details]
Bug 1586061: Prevent additional hidden windows from being created if one already exists on macOS. r=mstange

Approved for 96.0b8 and 91.5esr.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/aecc104f3229

Comment 17

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5713a33fe021