Skip to content

webui-desktop: redirect browser stdout/stderr to a log file#1252

Merged
rvykydal merged 1 commit intorhinstaller:mainfrom
rvykydal:fix-reboot-on-boot-iso
Apr 16, 2026
Merged

webui-desktop: redirect browser stdout/stderr to a log file#1252
rvykydal merged 1 commit intorhinstaller:mainfrom
rvykydal:fix-reboot-on-boot-iso

Conversation

@rvykydal
Copy link
Copy Markdown
Contributor

@rvykydal rvykydal commented Apr 14, 2026

The browser process inherits the stdout pipe from webui-desktop back
to anaconda's proc.communicate(). When exitGui() kills webui-desktop,
the cleanup trap kills the browser, but Firefox may survive (it forks
into its own process group). While Firefox is alive it holds the pipe
open, so proc.communicate() never returns, anaconda never exits, and
the atexit handler that calls systemctl reboot is never reached.

Redirect the browser's stdout/stderr to webui-browser.log file so that
even if Firefox outlives webui-desktop, it cannot block anaconda's pipe.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request modifies the webui-desktop script to redirect standard output and error to /dev/null when launching the browser, preventing potential pipe blocking issues. Review feedback highlights that the HOME environment variable is currently lost because pkexec clears the environment; suggestions were made to explicitly pass HOME within the command execution to ensure the browser correctly uses the isolated temporary directory.

Comment thread webui-desktop Outdated
Comment on lines +221 to +222
pkexec --user "$INSTALLER_USER" env XDG_CURRENT_DESKTOP=GNOME "${user_environment[@]}" "${BROWSER[@]}" "$WEBUI_URL" \
>/dev/null 2>&1 &
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

While the redirection to /dev/null correctly addresses the pipe blocking issue, there is a pre-existing bug in how the environment is passed to pkexec. pkexec clears the environment and sets HOME to the target user's default home directory, which means the isolation intended by the temporary directory created at line 214 is lost. To ensure the browser uses the correct home directory, HOME="$BROWSER_HOME" should be explicitly passed as an argument to the env command. Note that it should be placed after "${user_environment[@]}" to avoid being overwritten if that array contains a HOME entry.

Suggested change
pkexec --user "$INSTALLER_USER" env XDG_CURRENT_DESKTOP=GNOME "${user_environment[@]}" "${BROWSER[@]}" "$WEBUI_URL" \
>/dev/null 2>&1 &
pkexec --user "$INSTALLER_USER" env XDG_CURRENT_DESKTOP=GNOME "${user_environment[@]}" HOME="$BROWSER_HOME" "${BROWSER[@]}" "$WEBUI_URL" \
>/dev/null 2>&1 &

Comment thread webui-desktop Outdated
Comment on lines +244 to +245
dbus-run-session -- "${BROWSER[@]}" "$WEBUI_URL" \
>/dev/null 2>&1 &
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Similar to the graphical session case, the HOME variable should be explicitly passed to the browser via the env command. Since pkexec clears the environment, the HOME="$BROWSER_HOME" assignment at line 238 does not reach the browser process, defeating the isolation provided by the temporary home directory.

Suggested change
dbus-run-session -- "${BROWSER[@]}" "$WEBUI_URL" \
>/dev/null 2>&1 &
HOME="$BROWSER_HOME" \
dbus-run-session -- "${BROWSER[@]}" "$WEBUI_URL" \
>/dev/null 2>&1 &

@rvykydal rvykydal marked this pull request as ready for review April 14, 2026 10:56
Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link
Copy Markdown
Contributor

@adamkankovsky adamkankovsky left a comment

Choose a reason for hiding this comment

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

I'm just wondering if it might be better to redirect these logs to some other log file rather than discarding them entirely, for example, for future debugging.

Copy link
Copy Markdown
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

If it's tested and it works it looks reasonable.

@rvykydal
Copy link
Copy Markdown
Contributor Author

If it's tested and it works it looks reasonable.

Yes, tested to be working.

The browser process inherits the stdout pipe from webui-desktop back
to anaconda's proc.communicate(). When exitGui() kills webui-desktop,
the cleanup trap kills the browser, but Firefox may survive (it forks
into its own process group). While Firefox is alive it holds the pipe
open, so proc.communicate() never returns, anaconda never exits, and
the atexit handler that calls systemctl reboot is never reached.

Redirect the browser's stdout/stderr to webui-browser.log file so that
even if Firefox outlives webui-desktop, it cannot block anaconda's pipe.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@rvykydal rvykydal force-pushed the fix-reboot-on-boot-iso branch from 5160945 to 5c02074 Compare April 15, 2026 13:38
@rvykydal rvykydal changed the title webui-desktop: redirect browser stdout/stderr to /dev/null webui-desktop: redirect browser stdout/stderr to a log file Apr 15, 2026
@rvykydal
Copy link
Copy Markdown
Contributor Author

I'm just wondering if it might be better to redirect these logs to some other log file rather than discarding them entirely, for example, for future debugging.

Good point, updated.

@rvykydal
Copy link
Copy Markdown
Contributor Author

I updated the PR actually limiting the redirection to the boot.iso case (the Live ISO is not using proc.communicate code path)

Copy link
Copy Markdown
Contributor

@adamkankovsky adamkankovsky left a comment

Choose a reason for hiding this comment

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

Now it looks great, and if it works, I have no objection to it

@rvykydal
Copy link
Copy Markdown
Contributor Author

rvykydal commented Apr 15, 2026

Yes, it works.

FYI The contents of the webui-browser.log from my test run:

[anaconda root@ibm-p8-kvm-03-guest-02 ~]# cat /tmp/webui-browser.log.
restorecon: SELinux: Could not get canonical path for /tmp/install-user/.config/mozilla/firefox/*/gmp-widevinecdm/* restorecon: No such file or directory.
/run/user/1001/anaconda/firefox-profile/user.js:22: prefs parse error: unexpected character
dbus-daemon[3271]: [session uid=1001 pid=3271 pidfd=5] Activating service name='org.a11y.Bus' requested by ':1.0' (uid=1001 pid=3272 comm="/usr/lib64/firefox/firefox --new-instance --profil" labe
l="system_u:system_r:kernel_t:s0")
dbus-daemon[3271]: [session uid=1001 pid=3271 pidfd=5] Successfully activated service 'org.a11y.Bus'
Safe Browsing server returned a 400 during update:request url = https://safebrowsing.googleapis.com/v5/hashLists:batchGet?key=AIzaSyBPGXa4AYD4FC3HJK7LnIKxm4fDusVuuco, payload =.
dbus-daemon[3271]: [session uid=1001 pid=3271 pidfd=5] Reloaded configuration
dbus-daemon[3271]: [session uid=1001 pid=3271 pidfd=5] Reloaded configuration
dbus-daemon[3271]: [session uid=1001 pid=3271 pidfd=5] Reloaded configuration
dbus-daemon[3271]: [session uid=1001 pid=3271 pidfd=5] Reloaded configuration
dbus-daemon[3271]: [session uid=1001 pid=3271 pidfd=5] Reloaded configuration

@rvykydal rvykydal merged commit 7576440 into rhinstaller:main Apr 16, 2026
17 checks passed
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.

3 participants