fix: don't propagate GDK_BACKEND to subprocs by codebytere · Pull Request #29606 · electron/electron

codebytere · 2021-06-09T09:57:39Z

Description of Change

#28898

was created before the Chromium changes to absl::optional and so caused build failures when it was merged.

Checklist

PR description included and stakeholders cc'd
npm test passes
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: none.

zcbenz · 2021-06-09T11:57:46Z

The GDK_BACKEND test is failing.

codebytere · 2021-06-09T12:49:39Z

@zcbenz i think that CircleCI may set that 🤔 what do you think we should do? i think this got missed the first time bc of my CircleCI login problem

zcbenz · 2021-06-09T13:11:57Z

Hmm the reliable way to do the test is to start a child app with controlled environment variables. An example is how LC_ALL is tested in chromium-spec.ts.

zcbenz · 2021-06-10T08:39:53Z

spec-main/chromium-spec.ts

+
+  it('does not propagate GDK_BACKEND', async () => {
+    const appPath = path.join(fixturesPath, 'api', 'gdk-backend-check');
+    appProcess = ChildProcess.spawn(process.execPath, [appPath]);


The child process should have GDK_BACKEND explicitly cleared in its environment variables.

Also we can probably add a test to explicitly set the GDK_BACKEND and check the result.

ckerr

Several comments in this review but most of them are nits. I've tested this patch on a Wayland system (hirsute) & it WfM.

ckerr · 2021-06-10T13:54:40Z

shell/common/platform_util_linux.cc

  options.environment["MM_NOTTTY"] = "1";

+  // If the user set a GDK_BACKEND value of their own, use that,
+  // otherwise unset it becuase Chromium is setting GDK_BACKEND


nit: becuase

ckerr · 2021-06-10T14:08:00Z

spec-main/chromium-spec.ts

+      throw new Error(`Process exited with code "${code}" signal "${signal}" output "${output}" stderr "${stderr}"`);
+    }
+
+    output = output.replace(/(\r\n|\n|\r)/gm, '');


nit The global flag isn't needed here

ckerr · 2021-06-10T14:20:28Z

spec-main/chromium-spec.ts

+    const appPath = path.join(fixturesPath, 'api', 'gdk-backend-check');
+    appProcess = ChildProcess.spawn(process.execPath, [appPath], {
+      env: {
+        GDK_BACKEND: 'wayland'


(opinion, style) This test repeats this hardcoded string later in the test. If we make a local variable named GDK_BACKEND then the repetition would be avoided, plus this line could then be slightly tighter as env: { GDK_BACKEND }.

ckerr · 2021-06-10T14:21:02Z

spec-main/chromium-spec.ts

+      throw new Error(`Process exited with code "${code}" signal "${signal}" output "${output}" stderr "${stderr}"`);
+    }
+
+    output = output.replace(/(\r\n|\n|\r)/gm, '');


nit The global flag isn't needed here

ckerr · 2021-06-10T14:24:48Z

spec-main/chromium-spec.ts

+    output = output.replace(/(\r\n|\n|\r)/gm, '');
+
+    const backend = String(process.env.GDK_BACKEND);
+    expect(output).to.not.equal(backend);


I think this test will fail if run on a system where the parent process' GDK_BACKEND actually is 'wayland'?

Maybe we should inject nonsense values into the child instead of using x11/wayland in these tests to ensure there's not conflict with the system the tests are running on

ckerr · 2021-06-10T14:57:42Z

spec-main/chromium-spec.ts

+    }
+  });
+
+  it('does not propagate GDK_BACKEND', async () => {


OK there are some nits in this PR but overall I think the PR is good & works. This right here is the main thing that need to get changed -- the production code in this PR changes the environment variables in XDGUtil(), right before calling base::LaucnhProcess to invoke xdg-open or xdg-mail or (some various distro-specific way of trashing a file). This is fine, I think it addresses the use case in #28436

But this test is just looking at an arbitrarily-spawned ChildProcess. XDGUtil() isn't invoked for that and so I don't think these tests ever reach the codepaths that we're changing.

I see at least three options:

(1) decouple the GDK_BACKEND environment clobbering from Electron's XDGUtil() command, use extract-method to move it somewhere else that is more accessible by our tests

(2) Leave the production code where it is and use xdg-open to test the environment. This requires more complexity & setup in the tests, but should work with the production code as-is.. Loosely speaking, xdg-open works by looking at the filetype of the file being opened and looks for that file suffix in a .desktop file somewhere in the $XDG_DATA_HOME folder. When it finds it, It tries to launch the that .desktop file's app with the specified file. So we could make use of this by creating a script that takes a filename as input and dumps its environment variables to that file. Then we invent a file suffix that's not used by anyone else and create a fake .desktop entry that invokes our env-dumper script to "handle" that fake filesuffix. Then we call XDGOpen() for that file, which does reach the codepath we want to test. It should kick off our env dumper script. Then we can inspect the dumped env to see if the test passed. Then we remove the fake .desktop file from XDG_DATA_HOME to ensure our tests don't pollute the account running the test. 💫

(3) Remove the tests altogether. They are not testing what they're supposed to, so they shouldn't be left as-is. And doing a real test for this is kind of a pain. 🙂

My preference would be to try (2) but I wouldn't block the PR on it.

ckerr · 2021-06-10T15:03:11Z

shell/common/platform_util_linux.cc

+  // will make sure that they get unset from the environment via
+  // AlterEnvironment().


Nice job explaining clearly & succinctly that passing in an empty string will clobber the value altogether 👍

My first thought reading the code was "This isn't unsetting it; it's setting it to an empty string" so it's great explain (a) that's not the case and (b) that it's AlterEnvironment that strips out key/vals where val is an empty string

shell/common/platform_util_linux.cc

Co-authored-by: Charles Kerr <charles@charleskerr.com>

nfp0 · 2023-10-08T11:20:38Z

Has there been any progress on this?
This is now also affecting any app launched in the VSCode integrated terminal window.

georgexu99 · 2025-11-26T17:46:41Z

Closed due to inactivity

codebytere added the fast-track 🚅 Indicates that this PR is intended to bypass the 24 hour rule. Needs approval from Releases label Jun 9, 2021

codebytere requested a review from ckerr June 9, 2021 09:57

codebytere force-pushed the fix-lint branch from 40e88a3 to f4459c9 Compare June 9, 2021 09:59

codebytere added the semver/patch backwards-compatible bug fixes label Jun 9, 2021

fix: don't propagate GDK_BACKEND to subprocs

711d8a4

codebytere force-pushed the fix-lint branch from 8b431a0 to f66bd9e Compare June 10, 2021 07:42

codebytere changed the title ~~chore: fix lint failure~~ fix: don't propagate GDK_BACKEND to subprocs Jun 10, 2021

codebytere removed the fast-track 🚅 Indicates that this PR is intended to bypass the 24 hour rule. Needs approval from Releases label Jun 10, 2021

electron-cation bot added the new-pr 🌱 PR opened recently label Jun 10, 2021

codebytere added target/12-x-y labels Jun 10, 2021

zcbenz reviewed Jun 10, 2021

View reviewed changes

electron-cation bot removed the new-pr 🌱 PR opened recently label Jun 10, 2021

codebytere force-pushed the fix-lint branch from f66bd9e to e572bb1 Compare June 10, 2021 10:30

test: fixup GDK_BACKEND test

fcef5a5

codebytere force-pushed the fix-lint branch from e572bb1 to fcef5a5 Compare June 10, 2021 10:31

ckerr reviewed Jun 10, 2021

View reviewed changes

Update shell/common/platform_util_linux.cc

0ece28f

Co-authored-by: Charles Kerr <charles@charleskerr.com>

jpas mentioned this pull request Jun 10, 2021

signal-desktop cannot open links in the browser NixOS/nixpkgs#124901

Closed

jkleinsc added the target/15-x-y label Jul 22, 2021

jkleinsc added the target/16-x-y label Sep 22, 2021

andersk mentioned this pull request Oct 7, 2021

[Bug]: Electron propagates GDK_BACKEND=x11 to subprocesses, breaking shell.openExternal with Firefox on Wayland #28436

Open

3 tasks

jkleinsc added the target/17-x-y label Nov 17, 2021

bavulapati mentioned this pull request Feb 3, 2022

fix: do not propagate GDK_BACKEND env variable to subproc bavulapati/electron#1

Open

3 tasks

bavulapati mentioned this pull request Feb 16, 2022

fix: do not propagate GDK_BACKEND env variable to subproc #32929

Closed

3 tasks

miniak removed the target/12-x-y label Mar 29, 2022

miniak removed target/13-x-y labels Mar 29, 2022

jkleinsc removed target/15-x-y labels May 24, 2022

codebytere removed the target/17-x-y label Apr 11, 2023

georgexu99 closed this Nov 26, 2025

		// will make sure that they get unset from the environment via
		// AlterEnvironment().

Conversation

codebytere commented Jun 9, 2021

Description of Change

Checklist

Release Notes

Uh oh!

zcbenz commented Jun 9, 2021

Uh oh!

codebytere commented Jun 9, 2021

Uh oh!

zcbenz commented Jun 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ckerr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nfp0 commented Oct 8, 2023

Uh oh!

georgexu99 commented Nov 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants