Opened 2 months ago

Closed 5 weeks ago

Last modified 3 weeks ago

#31607 closed defect (fixed)

App menu items stop working

Reported by: mcs Owned by: tbb-team
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff68-esr, tbb-9.0-must-alpha, TorBrowserTeam201910R
Cc: acat, upsuper, baddles Actual Points: 5
Parent ID: Points: 0.5
Reviewer: Sponsor:

Description

In the ESR68-based Tor Browser on macOS, the App menu items are not working. For example, choosing Quit or pressing Cmd+Q has no effect. Same for Preferences and Cmd+,

I observed this problem while testing the es-ES and en-US builds from the following location on an older macOS 10.11.6 system:
https://people.torproject.org/~gk/builds/9.0a6-build4/

I will re-test on a 10.14.x system to make sure it isn't specific to macOS 10.11.x.

Child Tickets

Change History (24)

comment:1 Changed 2 months ago by mcs

A little more testing shows that About Tor Browser never works, but items like Preferences do work at first but stop after you try to access About Tor Browser. I saw the same behavior on macOS 10.14.x.

comment:2 Changed 2 months ago by mcs

Summary: App menu items not workingApp menu items stop working

comment:3 Changed 2 months ago by cypherpunks

Cc: ff68-esr, tbb-9.0-must-alpha?

comment:4 in reply to:  3 Changed 2 months ago by mcs

Cc: ff68-esr tbb-9.0-must-alpha removed
Keywords: ff68-esr tbb-9.0-must-alpha added

Replying to cypherpunks:

Cc: ff68-esr, tbb-9.0-must-alpha?

Thanks and fixed.

comment:5 Changed 2 months ago by gk

Cc: acat added
Keywords: TorBrowserTeam201909 added
Points: 0.5

Do we get any errors in the error console? I wonder whether we made a mistake when integrating the About dialog from Torbutton directly into tor-browser.

comment:6 in reply to:  5 Changed 2 months ago by mcs

Replying to gk:

Do we get any errors in the error console? I wonder whether we made a mistake when integrating the About dialog from Torbutton directly into tor-browser.

When I choose "About Tor Browser" from the app menu the error console shows:

NS_ERROR_NOT_AVAILABLE: utilityOverlay.js:994

That line in utilityOverlay.js is:

 window.openDialog("chrome://browser/content/aboutDialog.xul", "", features);

If I enter chrome://browser/content/aboutDialog.xul in the URL bar, the about box XUL loads in a regular tab. Kathy and I will look deeper.

Last edited 2 months ago by mcs (previous) (diff)

comment:7 Changed 2 months ago by mcs

Here is some more info:

  • This bug does not occur if the modal Tor Launcher window is not opened at startup.
  • The menu items stop working because the underlying C++ and Objective-C menu objects are destroyed.

Kathy and I will continue to debug this.

comment:8 Changed 2 months ago by mcs

A quick update: this bug occurs without any of our Tor Browser patches. Specifically, I checked out revision a37fc1d6835dcebc36ae80f1d81f06d7807988b4 and then applied the #28044 patch (e05db1ef99bf1fa0717e357d7359ab6f7104a8c6). It is easy to reproduce this bug by starting with TOR_FORCE_NET_CONFIG=1, clicking "Connect", and then hitting Cmd+, (which should open the about:preferences but does not).

comment:9 Changed 8 weeks ago by mcs

Kathy and I have not found a fix for this yet, and unfortunately we will not be able to work on this ticket again until the week of September 30th. We have reached out to Mozilla for help (via tjr) but it sounds like not many people have experience with the widget/cocoa code. Here is part of what we sent to Mozilla:

We have determined the following:

1) The nsMenuBarX destructor is being called at some point after the Tor Launcher window closes. This causes the application menu to stop working because mMenuGroupOwner has been cleared within the MenuItemInfo objects that are associated with the Cocoa application menu items.

2) During construction of the nsMenuBarX object, ConstructFallbackNativeMenus() is called (it doesn't seem to be called if Tor Launcher is not opened at startup).

It would be helpful if someone could give us an overview (or in-depth?) of how the menus / menubars are expected to work on Mac. In particular these are some of our questions:
Should the nsMenuBarX destructor be called only before exiting the browser? Or is it OK that it is called while a browser window is open?

Should ConstructFallbackNativeMenus() be getting called at startup?

comment:10 Changed 8 weeks ago by cypherpunks

Could it be that Firefox no longer supports modal windows?

comment:11 Changed 8 weeks ago by cypherpunks

Oh, why didn't you mention that it could be still not fixed https://bugzilla.mozilla.org/show_bug.cgi?id=1151345#c86?

comment:12 Changed 8 weeks ago by cypherpunks

Should ConstructFallbackNativeMenus() be getting called at startup?

No: https://bugzilla.mozilla.org/show_bug.cgi?id=1127205#c0

comment:13 Changed 7 weeks ago by rex4539

Just to note, this is definitely a regression and Firefox nightly doesn't have this bug.

I initially tested with 8.5.5 and the bug is not there so it was introduced in 9.0a6

I tested the nightly builds and the bug was introduced in 2019-09-01.

This will hopefully help folks pinpoint the commit that broke the build.

comment:14 Changed 6 weeks ago by pili

Keywords: TorBrowserTeam201910 added

comment:15 Changed 6 weeks ago by pili

Keywords: TorBrowserTeam201909 removed

comment:16 Changed 6 weeks ago by gk

Priority: MediumHigh

comment:17 Changed 6 weeks ago by mcs

After a lot of debugging, Kathy and I learned two things:

  1. The menu-related code in widget/cocoa is fragile and does not recover well when things occur in a different order than in Firefox.
  2. The root cause of this bug is that the browser's hidden window is created earlier than usual when Tor Launcher is present, and later during browser startup that hidden window is replaced with a new one. Some of the app menu data structures are associated with the first hidden window and they are freed when that hidden window is freed which breaks the app menu functionality.

We found two possible ways to fix this bug:
(a) Change Tor Launcher to open its wizard/progress window later during the startup sequence.
(b) Change the hidden window creation code inside the guts of the browser to not re-create the window.

Kathy and I believe option (b) is a safer fix. We will post both patches so other people can take a look.

comment:18 Changed 6 weeks ago by mcs

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201910 removed
Status: newneeds_review

Here is the Tor Launcher solution:
https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug31607-01&id=122ed31416cd08ea2892b1c7d6c6f362d8ecf006

The risk is that this significantly changes the point in time at which Tor Launcher blocks browser startup with its modal window, and it is difficult to analyze everything that happens between profile-after-change and final-ui-startup.

And here is the "just one hidden window, please" solution:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug31607-01&id=8024e654cd1c2bbcefc82bfd11099c9659abfc7b

If we like this one, we should try to upstream it to Firefox.

Please choose one :)

Last edited 6 weeks ago by mcs (previous) (diff)

comment:19 Changed 6 weeks ago by mcs

As suggested by Georg, I filed a Mozilla bug in the hope of getting feedback on the hidden window patch:
https://bugzilla.mozilla.org/show_bug.cgi?id=1586061

comment:20 Changed 6 weeks ago by gk

Cc: upsuper added

Closed #31974 as a duplicate.

comment:21 Changed 5 weeks ago by gk

Cc: baddles added

Closed #32008 as a duplicate.

comment:22 Changed 5 weeks ago by gk

Okay, still no feedback from Mozilla. I think we should take the patch, though, for the alpha to give it a wider testing (and, yes, I think we should fix that directly in Firefox code).

comment:23 Changed 5 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

This works for me. Cherry-picking it to tor-browser-68.1.0esr-9.0-2 (commit 1da04ce775a4994924a2c8154fb1f76c663a5eff) for now.

comment:24 Changed 3 weeks ago by mcs

Actual Points: 5
Note: See TracTickets for help on using tickets.