Closed Bug 1836348 Opened 11 months ago Closed 6 months ago

Explore candidate workarounds to about:restartrequired in Firefox .deb packages

Categories

(Release Engineering :: General, task)

Product:
Release Engineering
Component:
General
Platform:
Desktop
Linux
Type:
task

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: gabriel, Assigned: gabriel)

References

(Blocks 1 open bug)

Details

We have identified an issue in Firefox .deb packages. The browser's about:restartrequired page interferes with user intent to use the browser post APT upgrade. To alleviate this problem, two potential workarounds were proposed that shift user actions to the update process (where the user intent is upgrading the application.)

Option 1: Pre-install Script

This workaround revolves around implementing a preinst script that can handle some of the upgrade cases.

  • If Firefox is not running, proceed with the upgrade immediately.
  • If a single instance of Firefox is running, prompt the user with a notification that they will need to manually restart or exit Firefox. Should they refuse, halt the package upgrade process.
  • In other scenarios, the script bails out.

Option 2: Create Separate Packages

This workaround suggests creating two separate packages: firefox and firefox-$version.

  • The firefox package would depend on firefox-$version and provide /usr/bin/firefox, which points to /usr/lib/firefox-$version.
  • During an upgrade, firefox-$old_version remains installed, but firefox will point to the next version.
  • Several firefox-$old_version packages will get installed (similar to linux-image-$kernel_version), but they can be purged using APT's autoremove command.

While this method could solve the issue, it may also result in many packages installed on the system. We could make the package conflict with firefox packages 3-10 versions back, though this approach needs more investigation.

Addendum

@jld: Regarding bug 1609882 comment 2 / bug 1752638: Can the forkserver become enabled by default for Nightly users of Mozilla's new debian package?

I'm using dom.ipc.forkserver.enable=true (bug 1609882) without problem so far. Socket, RDD, Utility don't seem to matter as they are already existing when the package update happens.

Flags: needinfo?(jld)

(In reply to Gabriel Bustamante [:gabriel] from comment #0)

  • Launching firefox from another app after an update should theoretically "ask" the old one to open the URL. Further testing is needed to confirm this behavior.

Confirmed on KDE Wayland, Debian Testing. That works if the forkserver is enabled.

  1. Nightly is open (dom.ipc.forkserver.enable is already true)
  2. Downgraded: $ sudo apt install firefox-nightly=115.0a1~20230531041723 firefox-nightly-l10n-de=115.0a1~20230531041723
  3. $ xdg-open https://example.com
  4. tab opened successfully
  5. Closed Nightly.
  1. Started old Nightly (dom.ipc.forkserver.enable is true).
  2. Upgraded: $ sudo apt upgrade
  3. $ xdg-open https://example.org
  4. tab opened successfully.
  1. Disabled dom.ipc.forkserver.enable. Closed Nightly.
  2. Downgraded: $ sudo apt install firefox-nightly=115.0a1~20230531041723 firefox-nightly-l10n-de=115.0a1~20230531041723
  1. Started old Nightly (dom.ipc.forkserver.enable is false).
  2. Upgraded: $ sudo apt upgrade
  3. $ xdg-open https://nasa.gov

    QSocketNotifier: Can only be used with threads started with QThread

  4. https://nasa.gov/ tab opened with about:restartrequired content.
  1. Reenabled dom.ipc.forkserver.enable and restarted Nightly because it's a better experience.

@jld: Regarding bug 1609882 comment 2 / bug 1752638: Can the forkserver become enabled by default for Nightly users of Mozilla's new debian package?

As far as we are concerned the forkserver would be the ideal solution to this problem but the concern has always been that the long tail of subtle and not-so-easy-to-fix breakage could be very long and take substantially more people and time than the other solutions suggested. If there's some breakage that indeed turns out to be problematic to fix, you have to turn it off again, and now the underlying problem is back.

Socket, RDD, Utility don't seem to matter as they are already existing when the package update happens.

Utility isn't a single process, we can launch multiple for various use cases and if the sandboxing needs differ. It's quite possible that isn't a problem on Linux right now, but then having to fix the forkserver just to use Utility for something else is very painful again.

So basically, the forkserver right now has enough unknowns (and some known) issues that I would advise against coupling working Debian updates against it.

Socket, RDD, Utility don't seem to matter as they are already existing when the package update happens.

We would expect the things they are doing to silently break if you do an update that changes the IPC layer or messages used on both sides, FWIW.

For what it's worth, I have been looking into the fork server for this use case; an interesting exercise here is to turn off content process prelaunching, chmod 0 the install directory, and see what's broken. There is some obvious breakage, and I have some preliminary patches for most of it, but there's probably something more subtle I haven't noticed yet, and that's on top of the other concerns with the fork server. So, we're investigating it, but I agree that it's worth looking into other ways of dealing with this for .deb packages, especially if they can be shippable sooner.

Flags: needinfo?(jld)

Thank you all for your detailed input (and testing.)

I agree that the ideal solution would be to use the fork server by default, but from the sound of it, we're not there yet. There are many complexities and potential obstacles to this approach, which might introduce even more issues than we're currently dealing with. We'd like to avoid tying the success of Debian updates too closely to this, at least for the time being.

So, in light of these considerations, it seems to me that Option 2, creating separate firefox and firefox-$version packages, is the more pragmatic route for us to take at this time (sometimes perfect is the enemy of the good.)

I understand that Option 2 will result in more packages being installed on the user's system. So I will be actively looking for ways to mitigate this concern (such as considering conflicts with previous version firefox packages.)

This isn't to say I am giving up on the fork server. I am very interested in its potential and will continue to evaluate its progress. I appreciate the continued work and investigations.

I started working on implementing Option 2, creating separate firefox and firefox-$version packages.

I don't think we can automatically purge the old packages by declaring relationships between packages. The idea of making the package conflict with versions <= the version a couple of versions back seems attractive, but it's not as straightforward as it seems. The firefox-$version packages are distinct entities, they're not merely different versions of the same package. This means that specifying conflicts wouldn't have the desired effect, as the system wouldn't recognize them as different versions of the same package. We would need the full list of firefox-$version packages that we have shipped to remove them.

I tried a couple things around Option 2. But, I forgot to restart Firefox before running APT autoremove to clean old versions and hit the issue. So, I think 2 is out. I am implementing a proof-of-concept of a preinst script that bails out on the update if there are running Firefox instances. But I feel failing preinst scripts results is a not-so-good user experience. :jld you mentioned there's some obvious breakage in the fork server, could you elaborate? Would the preliminary patches for most of it make it nightly experiment worthy?

Something we discussed yesterday is to have a prerm that prevents package removal when files from the package are in use. The interaction with the various APT frontends would need to be checked, but in theory, it would solve most problems.

What would the prerm script check? If there are processes associated with a package? Can the script tell APT not to remove the files after the script executes?

What would the prerm script check? If there are processes associated with a package?

If there are processes running that use files from the package. The simplest form would be checking something like lsof /usr/lib/$package_name/libxul.so

Can the script tell APT not to remove the files after the script executes?

Presumably, if the prerm script fails with an non-zero exit code, APT will not remove the package. What actually happens would need to be checked, obviously. Especially in GUI frontends.

See Also: → 1842885

Hello! Unfortunately, QA can still reproduce the Restart Firefox message and Tab crash issue after an update was applied via the terminal while Firefox is running using Firefox Nightly 118.0a1 deb by following the next steps:

  1. Open a Firefox 118.0a1 deb build older than the current day nightly.
  2. Open the terminal and update Firefox deb (sudo apt-get update && sudo apt-get upgrade) while Firefox deb is running.
  3. After the update was completed, open some random webpages in a new tab from the New tab > Top Sites section by using Left Click and Middle click mouse buttons.
    Actual result: After some pages were opened > the Firefox Restart message is displayed after using the Left click on a link and a Firefox tab crash is displayed when using the middle click on a link.

The above tests were conducted on: Lubuntu 20.04 LXQt, Kubuntu 22.04 KDE, Ubuntu 22.01, and Debian 11

Screen recording: link. If more information is needed please let us know. Thank you!

I tried installing a packaged Nightly (version 118.0a1~20230807092029) and, despite having dom.ipc.forkserver.enable set to true in about:config, it's not running the fork server and the child processes have all been started directly (this can be seen with pstree -pT). I also don't see anything logged when I run with MOZ_LOG=ForkService:5.

But, if I edit the distribution.ini to remove the line that sets the pref, and then set the pref manually in about:config, then it does work.

I notice that dom.ipc.forkserver.enable is a mirror: once pref; is it possible that distribution.ini is being applied too late in startup?

Summary: Explore two candidates for a workaround to about:restartrequired in Firefox .deb packages → Explore candidate workarounds to about:restartrequired in Firefox .deb packages

Thanks Jed. On a call earlier today, glandium suggested using a preference file. Not sure if this is would be early enough in startup.

(In reply to Gabriel Bustamante [:gabriel] from comment #14)

Thanks Jed. On a call earlier today, glandium suggested using a preference file. Not sure if this is would be early enough in startup.

Another option is to have ForkServiceLauncher react to the pref dynamically (using Preferences::RegisterCallbackAndCall). That seems to work with distribution.ini. There are a few subtleties to deal with as far as handling pref changes after processes are already launched, but if that seems like a better option, we could easily do that.

Yeah that seems like a better option. If it can be done easily I think it's the way to go.

:glandium iiuc the suggestion is adding a preference file similar to browser/app/profile/firefox.js?

Mm I'll set it in one of the existing preference files and see if that works.

The browser/app/profile/firefox.js file calls out being about "non-static prefs." Does it makes a difference that dom.ipc.forkserver.enable is a static pref?

You'd want to inject a pref file during the repackage, since we only want the pref to be enabled in the repackaged version. A location for such a pref would be defaults/pref in the firefox directory, next to channel-prefs.js.

(In reply to Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ from comment #15)

Another option is to have ForkServiceLauncher react to the pref dynamically (using Preferences::RegisterCallbackAndCall). That seems to work with distribution.ini. There are a few subtleties to deal with as far as handling pref changes after processes are already launched, but if that seems like a better option, we could easily do that.

Now that I'm back I looked into this again. It's a little more annoying than I thought: if the fork server is running and has already been used to launch child processes, and then the pref is turned off, we don't want to terminate the fork server, but it's not great to have a bunch of extra code (and some extra state) to handle a case that won't normally ever happen. So, the simplest thing is to just ignore that and require restarting to turn the pref off… but then, if/when we turn the pref on by default, if anyone wanted to turn the pref off via distribution.ini then it wouldn't work. Except, I tried it and it does work, because the ForkServerLauncher and the distribution.ini processing both observe final-ui-startup and the ordering of the Observe calls happened to be the right way, but that's not something we can depend on.

So, in decreasing order of simplicity/elegance:

  1. If there is a way to use pref files to change the default, similarly to how it works when the pref is changed by the user in about:config, then that should just work with the current code.
  2. Alternately, if there's a way to apply the pref that happens strictly before final-ui-startup but too late for mirror: once to work, then it would suffice to change the pref to mirror: always and leave a comment warning that it's only checked during late startup.
  3. Or, if it really has to be distribution.ini (and it sounds like it doesn't, but mentioning this for completeness), dispatching a runnable to the main thread should (I think?) allow fork server startup to happen strictly after the rest of final-ui-startup, but this seems fragile.
See Also: → 1850026
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.