Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(x11/firefox): fix Segmentation fault in the command "firefox -headless" #22287

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

robertkirkman
Copy link
Contributor

@robertkirkman robertkirkman commented Nov 16, 2024

Fixes #22286

Bypasses the unstable codepath by disabling MOZ_HAS_REMOTE, which does not seem to negatively affect the behavior of the X11 mode. It only fixes the command firefox -headless; I am not 100% sure that nobody else needs MOZ_HAS_REMOTE to be enabled, but disabling it fixes firefox -headless and does not seem to break anything obvious that I have noticed from browsing with the X11 mode for a few minutes.

This patch is a dependency of my project, "How To Run Node.js Puppeteer On Android" https://gist.github.com/robertkirkman/0c2f3426024069546ed9b7bb2f26cb99, which is a fully functional example of a Javascript program that invokes the system call firefox -headless through the Puppeteer library.
I am hoping to possibly upstream the patch part of the necessary code for the headless mode on Termux of the Firefox backend of the Puppeteer Node.js library by submitting this PR.

Copy link
Member

@TomJo2000 TomJo2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch itself is fine the GDB trace is excessive IMO.

x11-packages/firefox/0028-fix-headless-mode.patch Outdated Show resolved Hide resolved
@robertkirkman robertkirkman force-pushed the firefox-fix-headless-mode-crash branch from d549541 to 995e1ad Compare November 16, 2024 15:31
--- a/toolkit/moz.configure
+++ b/toolkit/moz.configure
@@ -2714,8 +2714,8 @@ def has_remote(toolkit):
return True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be better to patch it here.

 @depends(toolkit)
 def has_remote(toolkit):
+    return False
     if toolkit in ("gtk", "windows", "cocoa"):
         return True

Copy link
Contributor Author

@robertkirkman robertkirkman Nov 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@licy183 thank you for the interesting observation,

I can let you know that I actually already tested a very similar patch to that first, before iterating to reach the patch as it is now, and unfortunately, for me, it did not work. Only the way I have written it shown in this PR worked for me.

If you would like, you can test your idea as well and let me know if you are able to get it to work that way and still fix the crash. I would be interested to know if I was just doing it wrong, and it can actually work that way.

@@ -0,0 +1,14 @@
Fixes a crash in the command "firefox -headless"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to put the issue link here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I have added that

@robertkirkman robertkirkman force-pushed the firefox-fix-headless-mode-crash branch from 995e1ad to fd9d377 Compare November 17, 2024 08:28
@TomJo2000
Copy link
Member

Should be ready to merge.
I'll take a final look in the morning.

If another maintainer wants to merge it before I get to that, feel free to.

@robertkirkman
Copy link
Contributor Author

Unfortunately, I have discovered an undesirable bug introduced by this PR.

While this fixes the command firefox -headless,
it breaks the command firefox & sleep 5 && firefox google.com, which currently works, but after this change would create a popup "firefox is running but is not responding".

I would not really feel comfortable about introducing something like that so I will mark this as draft until I can figure out if there is any alternative way to write this that does not break the command firefox & sleep 5 && firefox google.com.

@robertkirkman robertkirkman marked this pull request as draft November 21, 2024 13:09
@ZefianAlfian
Copy link

Hi, how to use this patch?

@robertkirkman robertkirkman force-pushed the firefox-fix-headless-mode-crash branch from fd9d377 to f2f7d30 Compare December 24, 2024 00:41
@robertkirkman
Copy link
Contributor Author

@ZefianAlfian i prepared complete directions for you, but it needed an update so I pushed an update that is compiling now in the GitHub Actions, it will take about 2 hours to compile and then when it finishes I will send you the complete directions how to use it and ping you again!

@TomJo2000
Copy link
Member

@robertkirkman I have a saved reply for installing CI artifact packages.

See if any of this sounds useful to you and you can then add a Saved Reply in your profile settings.


(This is a pre-written, saved reply.)
If you want to test this PR please download the appropriate DEB package(s)
from the build artifacts of the associated PR's latest CI run.
Screenshot_20240619_232413

After downloading the build artifact, make sure to unzip and un-tar it.

Detailed instructions, if needed.

# finding out what architecture you need
# architecture is just below the TERMUX_VERSION
termux-info

# e.g.
# [...]
# TERMUX_MAIN_PACKAGE_FORMAT=debian
# TERMUX_VERSION=0.118.0
# TERMUX__USER_ID=0
# Packages CPU architecture:
# aarch64
# [...]

# =======================

# make sure `unzip` and `tar` are installed using
pkg install unzip tar

# unzip the artifact (if you have a different architecture this might be arm, i686 or x86_64 instead)
unzip debs-aarch64-*.zip

# untar the artifact
tar xf debs-aarch64-*.tar

# You should now have a debs/ directory in your current working directory
# Install the packages from the local source using
pkg install -- ./debs/*.deb

# to clean up, you can remove the debs/ directory, .tar file and .zip file
rm -rfi debs debs-aarch64-*.zip debs-aarch64-*.tar

@robertkirkman
Copy link
Contributor Author

robertkirkman commented Dec 24, 2024

Yes that is true and I was going to hold off on posting basically that message until it completes compiling in case there is an error during the build and I have to further edit it, but additionally, I wrote more directions for @ZefianAlfian at the end of my planned message about what they probably want to do after installing the .deb file artifact from GitHub Actions, so I will post my planned message now with just the part that is now explained in your message removed:

@ZefianAlfian hi i'm really sorry that it's not upstreamed yet i am working on fixing the problem it causes with opening two firefox tabs at once. If you use only one firefox command at a time then this can work for you already. I already have a technique and plan for how I can make a future version that doesn't have the issue with multiple tabs, so I am going to make more updates to this but there are just a lot of other things I am also trying to do at the same time. In the mean time you can use it by downloading the .zip file from this link... follow the directions given by Tomjo2000 above

Then you can use it for what you need, for example if you want Node.js Puppeteer, you can use it as demonstrated in my demo gist if you saw that, and it will just work. The correct version of Puppeteer is the one that comes from this command: npm install puppeteer-core. To make it work with an existing Puppeteer project, you need to first migrate it to work with Firefox, then specifically specify /data/data/com.termux/files/usr/bin/firefox as the browser location. This is the example of that from my gist:

  const browser = await puppeteer.launch({
    product: 'firefox',
    browser: 'firefox',
    executablePath: '/data/data/com.termux/files/usr/bin/firefox',
    headless: true,
    args: ['-width', width.toString(), '-height', height.toString()],
  });

I noticed that in particular, Puppeteer's Firefox support did not seem to work as expected unless explicitly specifying both "product: 'firefox'" and "browser: 'firefox'", and also specifying -width and -height explicitly that way, both of those quirks are either not necessary or just not the same when using chromium with Puppeteer so I'm not sure what causes them, but basically, to get the best results, if you are working on an existing Puppeteer project you probably need to make sure that its puppeteer.launch() call opens Firefox in this very specific way.

@ZefianAlfian
Copy link

Wow, Thank you very much. Your response is very fast (Sorry for my bad English)

@robertkirkman
Copy link
Contributor Author

@ZefianAlfian ok now it finished building so you could try it and check whether this works for what you need

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: the command "firefox -headless" crashes with Segmentation fault
4 participants