fix(windows): auto-detect Firefox binary for per-user installs - #174
fix(windows): auto-detect Firefox binary for per-user installs#174freema wants to merge 1 commit into
Conversation
geckodriver only searches the Program Files directories and HKEY_LOCAL_MACHINE for Firefox, so it cannot see an install made without administrator rights: that one lands in %LOCALAPPDATA%\Mozilla Firefox and registers under HKCU. Launching then fails with "Expected browser binary location, but unable to find binary in default location" even though Firefox is installed and working. Resolve the binary ourselves and pass it as moz:firefoxOptions.binary. Candidates are probed in geckodriver's own order (Program Files before the per-user location) so machines where it already works keep resolving to the same binary, then PATH, then the App Paths registry key. reg.exe is resolved under %SystemRoot%\System32 rather than through PATH, because an MCP client can launch the server with a minimal environment and the registry lookup is the last resort. When nothing is found, replace geckodriver's message with one that names --firefox-path. get_firefox_info now reports the detected path so the resolution is visible when debugging. This is a workaround for https://bugzilla.mozilla.org/show_bug.cgi?id=1921933 and can be dropped once geckodriver does the lookup itself.
|
Will finish the review tomorrow, but thanks a lot for splitting the PRs, it is much easier to review for me now!
It's fine to have a workaround temporarily here. Thanks for considering the upstream bug :) |
juliandescottes
left a comment
juliandescottes
left a comment
There was a problem hiding this comment.
Thanks @freema , approach looks good, I think the new error message could also be helpful on other platforms. See suggestions in comments.
| } | ||
| if (this.options.firefoxPath) { | ||
| firefoxOptions.setBinary(this.options.firefoxPath); | ||
| } else { |
There was a problem hiding this comment.
Let's make that branch windows-only
| } else { | |
| } else if (process.platform === 'win32') { |
And since binary.ts is a workaround while waiting for a geckodriver fix, let's also update it to clearly be only about windows. windows-binary.ts, and only expose findFirefoxBinaryWindows
| error instanceof Error ? error.message : String(error) | ||
| }` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Even with this flag, we can't be sure the error is related to the missing binary.
geckodriver will look into locations that binary.ts doesn't check.
We can imagine:
- binary.ts fails to find anything,
windowsBinaryLookupFailed = true - then geckodriver finds a binary
new Builder()...fails for any other reason
-> We would still print an error message suggestion to use --firefox-path, which might be unhelpful.
I would check if the geckodriver error message contains moz:firefoxOptions.binary (that's the capability name for the binary path, only used in that error message and very unlikely to change)
if (message.includes('moz:firefoxOptions.binary')) {
and I would drop windowsBinaryLookupFailed entirely. The missing-binary case isn't Windows-specific: on Linux geckodriver only searches PATH, so an install under /opt hits the same error. No reason to restrict the message to windows only.
2 participants
Split out of #169, which @juliandescottes asked me to break into smaller pieces. This is the part that needs a decision rather than just a review.
geckodriver only searches the Program Files directories and HKEY_LOCAL_MACHINE for Firefox, so it cannot see an install made without administrator rights: that one lands in
%LOCALAPPDATA%\Mozilla Firefoxand registers under HKCU. Launching then fails with "Expected browser binary location, but unable to find binary in default location" even though Firefox is installed and working.You pointed at https://bugzilla.mozilla.org/show_bug.cgi?id=1921933 and you are right that this belongs in geckodriver. I am happy to take that mentored bug. The open question is whether you want an interim workaround in the MCP in the meantime, or would rather wait for the upstream fix and close this. I have referenced the bug in the module header either way, so it is clear this is meant to go away.
Changes from the review on #169:
core.ts, its only callerbinaryLookupFailedtowindowsBinaryLookupFailedpath.win32instead of the ambientpath. That was the failing check on test(windows): run the integration suite on Windows #169: the suite mocksprocess.platformbut ran on a Linux runner, so PATH was split on:instead of;and the lookup returned null.Verified on Windows 11, Node 22.22.0, against a real per-user Firefox 154.0: nothing in either Program Files directory, nothing under HKLM, only
%LOCALAPPDATA%\Mozilla Firefoxand the HKCU registration. Onmainthat machine fails with the "unable to find binary in default location" message above. On this branch:get_firefox_inforeportsBinary: C:\Users\...\AppData\Local\Mozilla Firefox\firefox.exe (auto-detected)navigate_pageandevaluate_scriptboth finereg.exeby absolute path is for--firefox-pathand keeps the original geckodriver message--firefox-pathskips detection entirelyOne gap worth knowing about: a PATH entry wrapped in quotes (
"C:\some dir";...) is not unquoted before probing, so that entry is skipped. It falls through to the registry rather than failing, and I left it alone since this module is meant to be short-lived, but say the word and I will strip them.