desktop: disable the webkitgtk dmabuf renderer on nvidia, not only on wayland - #8884
danielhanchen merged 14 commits into
Conversation
… wayland WebKit's renderer selection has no session branch: AcceleratedBackingStore::rendererBufferTransportMode reads WEBKIT_DISABLE_DMABUF_RENDERER and WEBKIT_DMABUF_RENDERER_FORCE_SHM and picks the hardware transport on X11 the same way it does on Wayland, so an X11 session on the proprietary driver got no workaround. configure_linux_renderer now probes /proc/driver/nvidia/version before GTK initialization and applies the workaround on either display server, matching WebKit's own isNVIDIA fix for bug 262607. nouveau publishes nothing there and stays on the defaults.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15a185957b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn nvidia_driver_loaded() -> bool { | ||
| std::path::Path::new(NVIDIA_DRIVER_VERSION_PATH).exists() |
There was a problem hiding this comment.
On PRIME/Optimus hybrid systems, /proc/driver/nvidia/version exists whenever the NVIDIA kernel module is loaded even when GTK/WebKit is rendering on the integrated Intel or AMD GPU. In that common laptop configuration this probe falsely selects WEBKIT_DISABLE_DMABUF_RENDERER, disabling WebKit's DMABUF renderer on X11 (and replacing the less disruptive shared-memory workaround on Wayland) despite the affected NVIDIA renderer not being in use; base the workaround on the selected rendering GPU rather than system-wide module presence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The premise is right: /proc/driver/nvidia/version reports module presence, not the selected renderer, and on most Optimus laptops the internal panel is driven by the iGPU. The consequence is a slower buffer transport rather than a defect, though. WebKit falls back to its legacy path and rendering stays correct, which is the same fallback the Tauri docs recommend for this driver.
Basing it on the selected rendering GPU is not available at this call site. configure_linux_renderer runs before GTK initialization, and resolving the active renderer needs an EGL/GL context, which is exactly what WebKit's isNVIDIA() does inside rendererBufferTransportMode(). WebKit can afford that because the function is called lazily once GTK is already up. Creating a GL context here would defeat the ordering the workaround depends on.
The two failure directions are not symmetric. Over-triggering costs the accelerated transport on a hybrid host. Under-triggering brings back the compositor freeze and the session kill this PR is fixing, on the same laptops, since the reported host is itself a hybrid RTX 4050.
An affected user already has an exit: the operator override is tested before the NVIDIA branch, so WEBKIT_DISABLE_DMABUF_RENDERER=0 preserves the environment and WebKit keeps the DMABUF path (its own check treats "0" as off). Covered by an_operator_override_outranks_the_nvidia_default.
There was a problem hiding this comment.
Correct, and the code now says so. One correction to the reply above, because it changes the weight of the trade: on WebKitGTK 2.44 and later the cost of over-triggering is not a slower buffer transport. WEBKIT_DISABLE_DMABUF_RENDERER returns before mode.add(RendererBufferTransportMode::SharedMemory), so the transport set is empty, checkRequirements() is false and accelerated compositing is off for the whole process (#9393). A hybrid host presenting on the integrated GPU loses compositing, not a transport.
The probe stays on module presence, since resolving the rendering GPU needs a GL context and this function runs before GTK is initialized precisely so that no GL state exists yet. 94d2c95 names the PRIME case in the code and points at the WEBKIT_DISABLE_DMABUF_RENDERER=0 opt-out, which on a patched library hands the decision back to the library's own GL_VENDOR probe.
Measured on an NVIDIA X11 host here, running libwebkit2gtk-4.1-0 2.52.3-0ubuntu0.24.04.1, which carries the patch: the GBM and surfaceless EGL contexts both report NVIDIA Corporation, so the two probes agree. That box has two discrete cards and no integrated GPU, so it cannot exercise the disagreement this comment is about.
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f266a9a2bf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -52,6 +59,11 @@ fn rendering_plan( | |||
| return RenderingPlan::PreserveEnvironment; | |||
There was a problem hiding this comment.
When an NVIDIA/Wayland user updates from the previous build, that running build has already set WEBKIT_DMABUF_RENDERER_FORCE_SHM=1; the inspected update flow in studio/frontend/src/hooks/use-tauri-update.ts lines 416-427 starts the replacement as a child, which inherits this environment. This early return therefore prevents the replacement from setting the stronger WEBKIT_DISABLE_DMABUF_RENDERER workaround that this change requires for NVIDIA, so the first post-update process remains exposed to the freeze/crash until a clean external launch. The NVIDIA branch must distinguish inherited app state from a deliberate operator override or clear/replace the old app-applied variable during relaunch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not fixing. The mechanism is right: tauri 2.11.5 process::restart runs Command::new(path).args(...).spawn() with no env_clear, so the replacement inherits the environment. The scenario still is not fixable from this side. The inherited WEBKIT_DMABUF_RENDERER_FORCE_SHM=1 was set by the previous build, and that build exports no marker saying it applied it, so a marker added here can only tag state written by builds that already carry the marker. The other option, treating a bare FORCE_SHM=1 on NVIDIA as app state instead of an override, breaks the override contract this change tests in an_operator_override_outranks_the_nvidia_default: an operator on NVIDIA who prefers shared memory over an empty transport set has to be able to say so.
The gap is one session, NVIDIA plus Wayland only, and it leaves that session on the pre-update behaviour rather than regressing it. NVIDIA on X11, the case this change exists for, inherits nothing because the previous build applied nothing there, so the workaround takes effect on the first post-update launch. Same-build relaunches are already idempotent, since the inherited variable equals the one the new process would set.
There was a problem hiding this comment.
Fixed in 69781084c1: inherited FORCE_SHM=1 is replaced on NVIDIA relaunches; all 377 Tauri tests pass.
There was a problem hiding this comment.
Right, and 6978108 fixed it. 94d2c95 replaces that fix with a simpler one: the NVIDIA branch no longer reads WEBKIT_DMABUF_RENDERER_FORCE_SHM at all, rather than carving out the single value 1.
WebKit reads the variable as forceSHM && g_strcmp0(forceSHM, "0"), so =0 is indistinguishable from unset. The carve-out therefore honoured the one value that states nothing and overrode the one WebKit acts on, which left a stale FORCE_SHM=0 able to suppress the workaround on an unpatched NVIDIA host with nothing in the startup log. Ignoring the variable outright covers the relaunch case this comment raised and that one together.
WEBKIT_DISABLE_DMABUF_RENDERER=0 alongside FORCE_SHM=1 is still how an operator asks for shared memory on NVIDIA, covered by explicit_disable_override_preserves_force_shm_on_nvidia.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Went through this one carefully because I was looking at the same file from the performance side. The NVIDIA probe is the right instinct and I am not arguing with the direction. One factual correction though, and it changes which variable you want. The transport set is not empty on NVIDIA. The PR body says mode.add(RendererBufferTransportMode::SharedMemory);
+ if (isNVIDIA())
+ return;
+
const char* forceSHM = getenv("WEBKIT_DMABUF_RENDERER_FORCE_SHM");
if (forceSHM && g_strcmp0(forceSHM, "0"))
return;The early return lands between the Why that matters:
bool AcceleratedBackingStore::checkRequirements()
{
if (!rendererBufferTransportMode().isEmpty())
return gtkCanUseHardwareAcceleration();
return false;
}and const char* disableCompositing = getenv("WEBKIT_DISABLE_COMPOSITING_MODE");
if ((disableCompositing && strcmp(disableCompositing, "0")) || !AcceleratedBackingStore::checkRequirements())
m_canUseHardwareAcceleration = false;So since 2.44, Suggestion: use Two smaller things from reading the same patch. The patch ships a documented opt-out, Upstream detects NVIDIA by creating a throwaway GL context and testing whether On distro patch coverage, which cuts both ways. I checked two:
That does not invalidate the probe, since we cannot assume the patch, but it does mean that on Debian and Ubuntu this PR's only real change over upstream is the escalation from shared memory to compositing off. Which is the part I would drop. Last one, and it is a genuine open question rather than a correction. I could not find a single report anywhere of Whichever variable you land on, could the PR body spell out the failure path? Something like: if the workaround is wrong for a given host the symptom is X, and the user recovers by setting |
|
One follow up, because it may be decisive for the variable choice and it is cheap to check. Your description says both crash reporters are on Ubuntu. If that means Ubuntu 24.04 with the archive If that holds for their machines, then they crashed while already running SHM-only on NVIDIA, which would mean shared memory does not prevent this failure and Worth confirming before landing, since it decides between the two variables:
Related precedent that argues against ever assuming this one is fixed upstream: Debian removed the patch at 2.46.0-1 with the note "all Nvidia-related bugs are supposed to be fixed upstream", then brought it back in the very next revision, 2.46.0-2, "Bring back this patch (Closes: #1082139)". So the "upstream fixed it" hypothesis has been tried once and reverted within a single revision. Not an argument against your PR, just a reason to keep the probe and to be careful about which lever it pulls. |
|
Correcting myself on the part of my first comment that was advice rather than fact. The facts stand; the recommendation I drew from them was probably wrong, and I would rather say so before you act on it. What stands. The patch ordering is not in question. I have now read the raw hunk three times: mode.add(RendererBufferTransportMode::SharedMemory);
+ if (isNVIDIA())
+ return;
+
const char* forceSHM = getenv("WEBKIT_DMABUF_RENDERER_FORCE_SHM");Upstream's patch leaves NVIDIA at What I got wrong. I told you to switch the NVIDIA branch to The root cause looks like it is in GTK3, not in WebKit's transport. GNOME/gtk#8056, "Wayland: Attaching buffer to same EGL surface that window is backed by causes a crash (Error 71 (Protocol error) dispatching to Wayland display)", puts it in If that analysis is right, then the crash is on the shared-memory attach path. It also fits the field evidence, which I had already noticed but read the wrong way round: I could not find a single report anywhere of So your choice of if os.Getenv("WEBKIT_DISABLE_DMABUF_RENDERER") == "" && isNVIDIAGPU() {
_ = os.Setenv("WEBKIT_DISABLE_DMABUF_RENDERER", "1")
}
func isNVIDIAGPU() bool {
if _, err := os.Stat("/sys/module/nvidia"); err == nil {
return true
}
return false
}Same probe, same variable, same "both display servers" decision. It was originally gated on The one thing I would still change is not the variable but the trigger order. Tauri's Linux graphics doc, added 2026-06-15, ranks the levers:
and warns:
Worth trying |
|
Confirmed this hits studio/src-tauri/src/linux_webkit.rs, where the workaround is still gated on a Wayland session and NVIDIA X11 launches keep the dmabuf transport. |
mahiatlinux
left a comment
mahiatlinux
left a comment
There was a problem hiding this comment.
studio/src-tauri/src/linux_webkit.rs:70: the NVIDIA branch honours the one FORCE_SHM value WebKit ignores, and overrides the one it acts on
WebKit reads the variable as a value, not a presence:
const char* forceSHM = getenv("WEBKIT_DMABUF_RENDERER_FORCE_SHM");
if (forceSHM && g_strcmp0(forceSHM, "0"))
return;Same shape in every shipped version: AcceleratedBackingStoreDMABuf.cpp:78 at webkitgtk-2.44.0 and :97 at 2.48.0, AcceleratedBackingStore.cpp:99 at 2.50.0 and 2.52.0. So WEBKIT_DMABUF_RENDERER_FORCE_SHM=0 is indistinguishable from the variable being unset: it forces nothing and suppresses nothing.
Line 70 treats it as an operator override regardless. On an NVIDIA host whose WebKitGTK does not carry the Debian disable-nvidia-dmabuf.patch (Fedora, Arch, openSUSE, Nix, the Flatpak runtime, upstream tarballs; nothing of this is upstream, see below), a stale WEBKIT_DMABUF_RENDERER_FORCE_SHM=0 in environment.d or a launcher gives:
- line 72 returns
PreserveEnvironment, so nothing is applied andconfigure_linux_rendererreturnsNone; - WebKit adds
SharedMemory, skips the return because the value is"0", and addsHardware; - the host runs the full hardware dmabuf transport on NVIDIA, the state this change exists to prevent, with no line in the startup log, because nothing was applied.
The polarity is inverted. =1, the only value that states a preference, is overridden as inherited app state; =0, which states nothing, wins. WEBKIT_DISABLE_DMABUF_RENDERER is already tested first at line 57 and is the only variable WebKit accepts as "keep the dmabuf renderer" (=0 short-circuits its own check, AcceleratedBackingStore.cpp:87 at 2.50.0), so the escape hatch documented at line 68 survives without this branch.
Delete lines 70-73. an_operator_override_outranks_the_nvidia_default (:246) then asserts the opposite of the code and should become:
for value in ["0", "1", "", "true"] {
assert_eq!(
plan_on_nvidia(&[(FORCE_SHARED_MEMORY, value)]),
RenderingPlan::Apply(RenderingWorkaround::DisableDmabuf, NVIDIA_REASON)
);
}With that edit the module still compiles and all 16 tests pass, explicit_disable_override_preserves_force_shm_on_nvidia included, so the DISABLE_DMABUF=0 hatch is unaffected.
studio/src-tauri/src/linux_webkit.rs:222 and :63: the patch this reasoning rests on is described backwards, and it is not a WebKit fix
// isNVIDIA returns before SharedMemory is added, so FORCE_SHM is not equivalentThe shipped patch does the opposite. Debian webkit2gtk 2.53.90-1, debian/patches/disable-nvidia-dmabuf.patch, whose headers are Bug: https://bugs.webkit.org/show_bug.cgi?id=262607 and Origin: https://github.com/WebKit/WebKit/pull/18614:
mode.add(RendererBufferTransportMode::SharedMemory);
+ if (isNVIDIA())
+ return;
+
const char* forceSHM = getenv("WEBKIT_DMABUF_RENDERER_FORCE_SHM");The early return lands between the SharedMemory add and the FORCE_SHM check, so a patched build leaves NVIDIA on {SharedMemory}, exactly what FORCE_SHM=1 produces. WEBKIT_DISABLE_DMABUF_RENDERER=1 returns before the add and leaves {}, a different and stronger state. The same sentence appears in the PR body.
Line 63 calls it "webkit's isNVIDIA fix (bug 262607)". Bug 262607 is RESOLVED WONTFIX, PR 18614 was closed unmerged, and the string NVIDIA appears in neither backing-store file on any release branch from 2.42 to 2.52. It is a distribution patch. The PR body's next sentence already says so; the code comment contradicts it.
studio/src-tauri/src/linux_webkit.rs:140: remove_var(FORCE_SHARED_MEMORY) cannot change what WebKit selects
WEBKIT_DMABUF_RENDERER_FORCE_SHM has exactly one reader in WebKit, and it sits after the WEBKIT_DISABLE_DMABUF_RENDERER check that returns first: lines 66 and 78 at webkitgtk-2.44.0, 85 and 97 at 2.48.0, 87 and 99 at 2.50.0 and 2.52.0. The next statement, line 142, sets WEBKIT_DISABLE_DMABUF_RENDERER=1, so the removal is never observed. On the other path that reaches DisableDmabuf (non-NVIDIA Wayland below 2.44) the variable cannot be set, because line 77 returns first. Nothing else in the repository reads it.
Deleting lines 139-141 leaves all 16 tests green, the only coverage statement available for it: every new test asserts on rendering_plan, and configure_linux_renderer has none.
studio/src-tauri/src/main.rs:1784: the startup log no longer records the display server for the hosts this change targets
The NVIDIA branch returns at linux_webkit.rs:74, before the Wayland/X11 determination at :82-99 ever runs, so an NVIDIA X11 host and an NVIDIA Wayland host emit the identical line:
NVIDIA driver loaded; set WEBKIT_DISABLE_DMABUF_RENDERER=1 for WebKitGTK compatibility
The merge base always recorded Wayland detected. The two triggers are not mutually exclusive, so "the startup log now records which of the two triggered" holds only for the first match. Nothing else carries the display server: studio/src-tauri/src/diagnostics/report.rs:32-46 records app_version, os, arch and the AppImage flag, and tauri.log enters the support bundle as a 1000-line, 200 KiB tail (studio/src-tauri/src/diagnostics/mod.rs:30-31), so the startup line ages out of any long session. Compose both when both hold.
tests/studio/playwright_research_freeze.py:10: stale after the rename
studio/src-tauri/src/linux_webkit.rsforces that webview onto the SHM software renderer on Wayland
Both halves are now wrong on an NVIDIA host: linux_webkit.rs:74 selects WEBKIT_DISABLE_DMABUF_RENDERER, not the shared-memory transport, and it applies on X11 as well. The docstring exists to state what the harness cannot prove about the reporter's machine, so the wrong transport undercuts the caveat it is making.
PR body, "Testing": six new cases, not four
grep -c '#\[test\]' gives 10 at the merge base and 16 at 69781084c. The body lists four and was not updated after 69781084c added nvidia_relaunch_replaces_inherited_force_shm (:230) and explicit_disable_override_preserves_force_shm_on_nvidia (:238). Those two carry the override-contract change, the part a reviewer most needs pointed at.
|
I went back through the transport question, and the correction earlier in this thread is wrong for the versions Studio actually ships against. Your PR body has the ordering right. Sorry for the detour. The patch ordering depends on the WebKit versionThe
From Reproducible with: 2.53.90 is a development release on the way to 2.54, so the reordered form is not what Studio currently ships. The v0.1.801-beta AppImage bundles 2.50.4 (read from the bundled library through What that changes about the change itselfThree populations, and they behave differently: 1. Patched library, vendor probe returns NVIDIA. The library already returns an empty transport set on either display server, with no help from Studio. Setting 2. Patched library, module loaded, vendor probe returns non-NVIDIA. PRIME and Optimus laptops rendering on the integrated GPU. Here the library deliberately keeps the DMA-BUF transport and this change turns compositing off. This is the case the automated review flagged, and it is also the case where the change could genuinely be doing something for the RTX 4050 report. Same mechanism, opposite signs, and which applies depends on whether the discrete GPU is involved in presenting that window. That is the decision this PR needs to make out loud. 3. Distros without the patch. Fedora 44 ships The two variables are not interchangeable, measuredI ran the shipped v0.1.801-beta AppImage under Xvfb in a container with no host GTK or WebKit, and compared the GL objects mapped into
It also means the reply to the hybrid-GPU comment, that the consequence is a slower buffer transport rather than a defect, does not hold on 2.44 and later. The objection still needs an answer, just not that one. What I need before this can merge
One adjacent note: whichever variable you settle on, the same argument applies to the AppImage-on-Wayland branch that #9113 added, which sets |
|
I pushed two commits to this branch rather than only asking for them, because the conflict with #9113 had to be resolved before anything else could be checked. Revert either if you disagree with it.
|
| package | isNVIDIA() placement |
NVIDIA transport set |
|---|---|---|
Ubuntu webkit2gtk 2.50.4-0ubuntu0.22.04.1 |
before mode.add(SharedMemory) |
{} |
Debian webkit2gtk 2.52.6-1 |
before | {} |
Debian webkit2gtk 2.53.90-1 |
after | {SharedMemory} |
isNVIDIA appears nowhere in AcceleratedBackingStore.cpp at webkitgtk-2.50.4 or -2.52.5, so it only ever arrives as a distribution patch. Your PR body has the ordering right for every version Studio ships; it is 2.53.90 that changed it, which is why the code comment now states a version rather than a behaviour.
Verification
19 unit tests pass. Separately I ran the merged rendering_plan against main's across 6720 non-NVIDIA cases, every combination of GDK_BACKEND, WAYLAND_DISPLAY, FORCE_SHM, DISABLE_DMABUF and APPIMAGE values, both GLES states and four WebKitGTK versions, and got zero differences. So nothing outside the NVIDIA branch moved. On NVIDIA the only transitions against main are PreserveEnvironment -> DisableDmabuf in 1476 cases and ForceSharedMemory -> DisableDmabuf in 60; no host loses a workaround it had.
Requirement 4 is still open, and it is the only one that cannot be closed from a machine
It needs the reporters. The three populations behave differently, so which one each reporter is in decides whether this change fixes the failure they hit or broadens a workaround for a related one, and that is a fact about their hosts.
For the record on what a lab can and cannot settle here: the box I used is an NVIDIA X11 host running libwebkit2gtk-4.1-0 2.52.3-0ubuntu0.24.04.1, which does carry the patch (WEBKIT_FORCE_DMABUF_RENDERER is present in the shipped library and appears nowhere upstream). Its GBM and surfaceless EGL contexts both report NVIDIA Corporation, so the module probe and the library's own probe agree, which puts it in population 1 where this variable changes nothing. It has two discrete cards and no integrated GPU, so it cannot exercise the PRIME disagreement, and it has no Wayland session, so the Error 71 disconnect cannot occur on it at all. Neither absence is fixable by testing harder.
So please still collect, per reporter: the exact Studio asset or package, the WebKitGTK version, the display backend, the failure signature, and whether WEBKIT_DISABLE_DMABUF_RENDERER=1 alone changes it. For a Wayland Error 71, __NV_DISABLE_EXPLICIT_SYNC=1 on its own is worth the same test.
On your own review of 08-19
The :70 finding, the :140 finding and the main.rs:1784 finding are all implemented above. The :63 half of the second finding is implemented, since bug 262607 is WONTFIX and PR 18614 was closed unmerged; the :222 half is not, because the ordering it calls backwards is the ordering Ubuntu 2.50.4 and Debian 2.52.6 actually ship. I left tests/studio/playwright_research_freeze.py:10 alone: its docstring is already imprecise on main, since the AppImage branch selects DISABLE_DMABUF on Wayland too, so it belongs with #9393 rather than here.
Two things left that are yours to write: the body still says four new cases, and the file is now at 19 tests, six more than main; and the ordering claim in the body wants the same version scope the code comment now carries.
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
CI on
So the checks that can speak to this change are green: |
…r overrides Three things, all found while checking this branch against the WebKit sources rather than against descriptions of them. ## The two switches are not interchangeable, and X11 wants the lighter one Traced in webkitgtk-2.50.4, the version the AppImage bundles: DISABLE_DMABUF returns before mode.add(SharedMemory), so the set stays empty, checkRequirements() is false, AcceleratedBackingStore::create() returns nullptr and webkitWebViewBaseEnterAcceleratedCompositingMode() dereferences that behind an ASSERT that release builds compile out. FORCE_SHM returns after the add and leaves a valid backing store. That is not only a smoothness question. block/buzz#3654 reports a SIGSEGV on Ubuntu 24.04, WebKitGTK 2.52.3, X11, NVIDIA module loaded but EGL on Mesa, with DISABLE_DMABUF, and no crash with FORCE_SHM. That is exactly the iGPU-presenting topology the module probe over-triggers on. So the branch now picks per failure mode rather than per GPU. Wayland keeps DISABLE_DMABUF: the failure there is the explicit-sync disconnect, FORCE_SHM routes every commit down the wl_shm path that trips it (WebKit bug 315436), and DISABLE_DMABUF is the switch with reports of fixing Error 71. X11 on 2.44+ takes FORCE_SHM, which drops the hardware transport that fails there without emptying the set. Old WebKitGTK and the missing-GLES AppImage keep the stronger switch. ## Operator overrides win again, without pinning a stale decision Ignoring every FORCE_SHM value on NVIDIA also discarded a deliberate setting, and it was the setting that avoids the crash above. Both variables are operator overrides again. The relaunch case that motivated ignoring them is handled by naming the variable we set in UNSLOTH_WEBKIT_RENDERER_WORKAROUND, so a launch can tell its own inherited output from an instruction, re-decide, and clear the previous variable when the new decision differs. Without that a launch reads its own output back and the first decision is pinned for the life of the process tree; a new test asserts the plan is a fixed point instead. WEBKIT_FORCE_DMABUF_RENDERER is honoured too. It is the opt-out disable-nvidia-dmabuf.patch ships for itself, checked inside isNVIDIA(), which WebKit never reaches once DISABLE_DMABUF is set, so this branch was silently outranking the only lever a Debian or Ubuntu user has. It stands the NVIDIA branch down and nothing else: the missing-GLES fallback answers a packaging failure, not a GPU policy, and that launch cannot render without it. ## Diagnostics An NVIDIA AppImage that cannot load GLES reported only NVIDIA and hid the packaging defect. It now reports both. Two harness docstrings still said this module forces SHM on Wayland; both switches and both display servers are now in scope, so they say so. 19 tests to 27; the whole crate is 415 passing. Verified against the extracted Ubuntu 2.50.4 and Debian 2.52.6 patches (isNVIDIA() before the SharedMemory add, empty set) and 2.53.91 (after it, {SharedMemory}). A differential over 4860 host, session, packaging, inherited-environment and version combinations shows no host losing a workaround it had, and no operator override ignored.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aec95dcd4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Same rule for the shared-memory switch: do not combine it with legacy | ||
| // instructions a user may already carry in a launcher or environment.d file. | ||
| if env(FORCE_SHARED_MEMORY).is_some() && !ours(FORCE_SHARED_MEMORY) { | ||
| return RenderingPlan::PreserveEnvironment; |
There was a problem hiding this comment.
When an NVIDIA/Wayland user installs this revision through the in-app updater, the old process has already set WEBKIT_DMABUF_RENDERER_FORCE_SHM=1, and relaunch() starts the replacement as a child that inherits it. Fresh evidence in this revision is that ownership now depends solely on APPLIED_WORKAROUND, but the parent revision never emitted that marker, so this check classifies the inherited app value as an operator override and skips the required NVIDIA/Wayland DISABLE_DMABUF decision. Handle the legacy unmarked FORCE_SHM=1 state during the first post-update relaunch rather than preserving it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not fixing this one, and the mechanism is not in dispute: an in-app update does relaunch as a child, the parent revision never wrote a marker, and so that first relaunch does read its inherited WEBKIT_DMABUF_RENDERER_FORCE_SHM=1 as an operator value and stands down.
Three reasons it stays.
The cost is one session, and it is the previous release's own behaviour. That user was already on FORCE_SHM before the update; they are not worse off, they just pick up the new decision one launch later. The next start from the launcher has a clean environment and decides normally, so it self-heals without the user doing anything.
The migration cannot be written without guessing. An unmarked FORCE_SHM=1 on NVIDIA/Wayland is byte for byte what an operator gets by exporting it, so any rule that reclassifies it as app state also silently overrides that operator. That is the rule 94d2c95 had and the one this branch removed. It matters more now than it did then: on NVIDIA, FORCE_SHM=1 is the setting that keeps a valid backing store, so overriding it is the direction that can crash rather than merely slow things down.
The right place is not this module. If we want a post-update relaunch to start clean, the updater should not hand the replacement the old process's renderer decision at all, alongside the PYTHONHOME/PYTHONPATH scrubbing update.rs already does. That fixes the class rather than special-casing one inherited value, and it is a separate change from this one.
Happy to be argued out of it if there is a signal that separates our own inherited value from an operator's without a marker, but I could not find one.
Same decisions, fewer lines: 94 comment lines out, 58 back. Verified behaviour preserving by comparing the plan and the reason string against the previous commit over 420000 input combinations, 0 differences. 415 crate tests pass, rustfmt clean.
…nd-down for PR unslothai#8884 Two genuine Codex findings, both reproduced against the shipped sources. FORCE_SHM alone never reached selection on a patched library: isNVIDIA() returns before mode.add(SharedMemory) in Ubuntu 2.50.4 and Debian 2.52.6, so the X11 branch still handed NVIDIA an empty transport set. Its own opt-out is checked first inside isNVIDIA(), so ForceSharedMemory now sets WEBKIT_FORCE_DMABUF_RENDERER alongside WEBKIT_DMABUF_RENDERER_FORCE_SHM. Unpatched libraries ignore it, and 2.53.91, which moved isNVIDIA() after the add, lands on the same {SharedMemory}. Standing down dropped the ownership marker but left the variable set, so a relaunch that no longer wanted a workaround kept it, and the launch after that read the now unmarked value as an operator override and preserved it for good. release_claimed clears the values we set, never an operator's. The marker holds the variable list rather than one name, so ownership still covers a workaround that takes two, and our own FORCE_DMABUF no longer reads back as an operator opting out. 419 crate tests pass. The 4860-case host matrix still shows no regression, and the NVIDIA X11 rows now reach {SharedMemory} instead of the empty set.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…fault socket for PR unslothai#8884
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a411741f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return true; // a TCP or otherwise unusual display; not ours to judge | ||
| } | ||
| let path = format!("{socket_dir}/X{number}"); | ||
| if UnixStream::connect(&path).is_ok() { |
There was a problem hiding this comment.
When DISPLAY points to a reachable X socket but the X11 setup is rejected—for example because XAUTHORITY is missing or stale—this bare UnixStream::connect succeeds even though GDK's X11 opener fails and proceeds to the next backend. With GDK_BACKEND=x11,wayland, the plan therefore misclassifies the actual Wayland session as X11: non-NVIDIA hosts omit the Wayland workaround, while NVIDIA hosts choose the X11 FORCE_SHM path instead of the required Wayland workaround. Verify the X11 setup/authentication response rather than treating transport-level connection success as an opened display.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one, and the reason is the direction of the risk rather than the premise.
The premise is right: a reachable socket is not an opened display, and a stale or missing XAUTHORITY makes GDK skip the x11 opener. But the default configuration cannot reach the misclassification. With GDK_BACKEND unset the backend list is *, and that arm answers wayland before x11, so a live compositor already wins whatever DISPLAY says. It takes an explicit x11-first GDK_BACKEND and broken X authentication and a live compositor to land here.
Against that, the only correct implementation is the real thing: parse the .Xauthority binary format, pick the entry matching family, address, display and name, send the setup request and read the reply. That is an X11 protocol implementation running before GTK init, and its failure mode is the bad one. The probe is fail-safe on purpose, because calling a live server closed selects the Wayland branch, which on NVIDIA is WEBKIT_DISABLE_DMABUF_RENDERER on an X11 webview, and that is the empty transport set and the startup SIGSEGV the rest of this file is written around. A cheap version does not exist either: a setup request sent with no auth data gets Failed back from any server with access control on, which is every healthy session.
So the trade is a third-order misclassification that costs the wrong renderer switch, against new protocol code whose bugs cost a crash. Leaving the transport-level probe as the line, and it is documented as fail-safe for exactly this reason.
|
@codex review |
1 similar comment
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
5 participants
Two Ubuntu users report Unsloth Desktop freezing and crashing. Their hosts share
nothing on the GPU side, one an RTX 4050 laptop with 6 GB and one a 96 GB card,
so memory pressure does not explain the second. Both run NVIDIA.
configure_wayland_rendererapplied its workaround only on Wayland, andx11_session_keeps_webkit_defaultsasserted that an X11 session kept WebKit'sdefaults. WebKit's renderer selection has no session branch:
AcceleratedBackingStore::rendererBufferTransportModereadsWEBKIT_DISABLE_DMABUF_RENDERERandWEBKIT_DMABUF_RENDERER_FORCE_SHMandpicks the hardware transport on X11 the same way, so an Ubuntu X11 session on
the proprietary driver got no workaround at all.
WebKit's own fix for this (bug 262607)
adds
isNVIDIA()and drops every buffer transport for the proprietary driverregardless of display server. Distributions carry it as a patch, so it cannot be
assumed present in the host library.
configure_linux_renderernow probes/proc/driver/nvidia/versionbefore GTKinitialization and applies the workaround on either display server. The probe
reads a file rather than an EGL vendor string, so it touches no GL state before
GTK comes up. nouveau publishes nothing there and renders through Mesa, so it
stays on the defaults.
On NVIDIA the variable is
WEBKIT_DISABLE_DMABUF_RENDERER, matching whatupstream produces:
isNVIDIA()returns beforemode.add(RendererBufferTransportMode::SharedMemory), so the transport set isempty rather than shared-memory. Operator overrides still win, and non-NVIDIA
Wayland keeps
WEBKIT_DMABUF_RENDERER_FORCE_SHM.configure_linux_rendererreturns the reason alongside the variable, and thestartup log now records which of the two triggered.
Testing
Four new cases in
linux_webkit.rs: NVIDIA on X11, NVIDIA under an explicitGDK_BACKEND=x11, NVIDIA on Wayland taking the stronger switch, and an operatoroverride outranking the NVIDIA default.