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: do not propagate GDK_BACKEND env variable to subproc #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/build_config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ executors:
type: enum
enum: ["medium", "xlarge", "2xlarge+"]
docker:
- image: ghcr.io/electron/build:e6bebd08a51a0d78ec23e5b3fd7e7c0846412328
- image: ghcr.io/electron/build@sha256:93975f5641bff21cd9d92da65dbfdfdeee9199ef4ad5cfc4b86af38437ca0e10
resource_class: << parameters.size >>

macos:
Expand Down
2 changes: 1 addition & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ executors:
type: enum
enum: ["medium", "xlarge", "2xlarge+"]
docker:
- image: ghcr.io/electron/build:27db4a3e3512bfd2e47f58cea69922da0835f1d9
- image: ghcr.io/electron/build@sha256:93975f5641bff21cd9d92da65dbfdfdeee9199ef4ad5cfc4b86af38437ca0e10
resource_class: << parameters.size >>

# List of always run steps
Expand Down
16 changes: 16 additions & 0 deletions shell/browser/electron_browser_main_parts.cc
Original file line number Diff line number Diff line change
Expand Up @@ -361,8 +361,24 @@ void ElectronBrowserMainParts::PostDestroyThreads() {
fake_browser_process_->PostDestroyThreads();
}

#if BUILDFLAG(IS_LINUX)
// static
absl::optional<std::string>& ElectronBrowserMainParts::GetGDKBackend() {
static absl::optional<std::string> gdk_backend;
return gdk_backend;
}
#endif

void ElectronBrowserMainParts::ToolkitInitialized() {
#if BUILDFLAG(IS_LINUX)
// This is set by Chromium here:
// https://chromium-review.googlesource.com/c/chromium/src/+/2586184
// and can detrimentally affect external app behaviors, so we want to
// check if the user has set it so we can use it later.
std::string backend;
if (base::Environment::Create()->GetVar("GDK_BACKEND", &backend))
GetGDKBackend() = backend;

auto linux_ui = BuildGtkUi();
linux_ui->Initialize();
DCHECK(ui::LinuxInputMethodContextFactory::instance());
Expand Down
5 changes: 5 additions & 0 deletions shell/browser/electron_browser_main_parts.h
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,11 @@ class ElectronBrowserMainParts : public content::BrowserMainParts {
Browser* browser() { return browser_.get(); }
BrowserProcessImpl* browser_process() { return fake_browser_process_.get(); }

#if BUILDFLAG(IS_LINUX)
// Used by platform_util to set GDK_BACKEND.
static absl::optional<std::string>& GetGDKBackend();
#endif

protected:
// content::BrowserMainParts:
int PreEarlyInitialization() override;
Expand Down
13 changes: 13 additions & 0 deletions shell/common/platform_util_linux.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "dbus/bus.h"
#include "dbus/message.h"
#include "dbus/object_proxy.h"
#include "shell/browser/electron_browser_main_parts.h"
#include "shell/common/platform_util_internal.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "ui/gtk/gtk_util.h"
Expand Down Expand Up @@ -275,6 +276,18 @@ bool XDGUtil(const std::vector<std::string>& argv,
// bring up a new terminal if necessary. See "man mailcap".
options.environment["MM_NOTTTY"] = "1";

// If the user sets a GDK_BACKEND value of their own, use that,
// otherwise unset it because Chromium is setting GDK_BACKEND
// during GTK initialization and we want to respect user preference.
// Setting values in EnvironmentMap to an empty-string
// will make sure that they get unset from the environment via
// AlterEnvironment().
const absl::optional<std::string>& gdk_backend =
electron::ElectronBrowserMainParts::GetGDKBackend();
options.environment["GDK_BACKEND"] = gdk_backend.has_value()
? gdk_backend.value().c_str()
: base::NativeEnvironmentString();

base::Process process = base::LaunchProcess(argv, options);
if (!process.IsValid())
return false;
Expand Down
73 changes: 73 additions & 0 deletions spec-main/chromium-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { promisify } from 'util';
import { ifit, ifdescribe, delay, defer } from './spec-helpers';
import { AddressInfo } from 'net';
import { PipeTransport } from './pipe-transport';
const timersPromises = require('timers/promises');

const features = process._linkedBinding('electron_common_features');

Expand Down Expand Up @@ -364,6 +365,78 @@ describe('web security', () => {
});
});

ifdescribe(process.platform === 'linux' && !process.arch.includes('arm'))('subprocesses', () => {
let appProcess: ChildProcess.ChildProcessWithoutNullStreams | undefined;
const appPath = path.join(fixturesPath, 'api', 'gdk-backend-check');
beforeEach(() => {
ChildProcess.execFileSync(path.join(appPath, 'before-test.sh'), { cwd: appPath, encoding: 'utf8' });
});
afterEach(() => {
ChildProcess.execFileSync(path.join(appPath, 'after-test.sh'), { cwd: appPath, encoding: 'utf8' });
if (appProcess && !appProcess.killed) {
appProcess.kill();
appProcess = undefined;
}
});

it('does not propagate GDK_BACKEND', async () => {
appProcess = ChildProcess.spawn(process.execPath, [path.join(appPath, 'main.js')], { env: { ...process.env, GDK_BACKEND: '' } });

let output = '';
appProcess.stdout.on('data', (data) => { output += data; });
let stderr = '';
appProcess.stderr.on('data', (data) => { stderr += data; });

const [code, signal] = await emittedOnce(appProcess, 'exit');
if (code !== 0) {
throw new Error(`Process exited with code "${code}" signal "${signal}" output "${output}" stderr "${stderr}"`);
}

for await (const startTime of timersPromises.setInterval(10, Date.now())) {
if (fs.existsSync('/tmp/groot-says')) {
let gdkBackend = fs.readFileSync('/tmp/groot-says', 'utf8');
gdkBackend = gdkBackend.trim();

expect(gdkBackend).to.be.empty();
break;
}
const now = Date.now();
if ((now - startTime) > 5000) {
throw new Error('The gdk backend test failed timing out on waiting for the file');
}
}
});

it('successfully honors GDK_BACKEND set in the subproc', async () => {
appProcess = ChildProcess.spawn(process.execPath, [path.join(appPath, 'main.js')], { env: { ...process.env, GDK_BACKEND: 'groot' } });

let output = '';
appProcess.stdout.on('data', (data) => { output += data; });
let stderr = '';
appProcess.stderr.on('data', (data) => { stderr += data; });

const [code, signal] = await emittedOnce(appProcess, 'exit');
if (code !== 0) {
throw new Error(`Process exited with code "${code}" signal "${signal}" output "${output}" stderr "${stderr}"`);
}

for await (const startTime of timersPromises.setInterval(10, Date.now())) {
if (fs.existsSync('/tmp/groot-says')) {
let gdkBackend = fs.readFileSync('/tmp/groot-says', 'utf8');
gdkBackend = gdkBackend.trim();

expect(gdkBackend).to.not.be.empty();
expect(gdkBackend).to.equal('groot');
break;
}
const now = Date.now();
if ((now - startTime) > 5000) {
throw new Error('The gdk backend test failed timing out on waiting for the file');
}
}
});
});

describe('command line switches', () => {
let appProcess: ChildProcess.ChildProcessWithoutNullStreams | undefined;
afterEach(() => {
Expand Down
14 changes: 14 additions & 0 deletions spec/fixtures/api/gdk-backend-check/after-test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#!/bin/sh

set -o errexit
set -o nounset

rm -f /tmp/groot-says
rm -f /tmp/groot
rm -f ~/.local/share/applications/groot.desktop

update-desktop-database ~/.local/share/applications

rm -f ~/.local/share/mime/packages/groot.xml

update-mime-database ~/.local/share/mime
31 changes: 31 additions & 0 deletions spec/fixtures/api/gdk-backend-check/before-test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#!/bin/sh

Choose a reason for hiding this comment

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

if the script somehow fails, it will leave the system in a mutated condition which might not be something the people who run these tests locally or on ci want. can we plz make these tests more robust by using a trap?

Copy link
Owner Author

Choose a reason for hiding this comment

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

does having this in the afterTest help?

Choose a reason for hiding this comment

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

it wouldn't help if the process receives something like a SIGINT, which can only be handled using a trap


set -o errexit
set -o nounset

cleanup () {
exec "./after-test.sh"
}

trap cleanup HUP INT

# Install the application Groot
cp ./groot /tmp/

# Make sure the applications directory exists.
mkdir -p ~/.local/share/applications

# Register Groot as desktop application
cp ./groot.desktop ~/.local/share/applications/

# Update database of desktop entries
update-desktop-database ~/.local/share/applications

# Make sure the packages directory exists.
mkdir -p ~/.local/share/mime/packages

# Associate 'application/x-groot' mime type with '*.groot' files
cp ./groot.xml ~/.local/share/mime/packages/

# update the MIME database
update-mime-database ~/.local/share/mime

Choose a reason for hiding this comment

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

can u verify if these non-trivial commands are blocking? I just wanted to make sure this doesn't cause any race condition when the rest of the test expects these operations to be completed but they are actually in progress

Copy link
Owner Author

Choose a reason for hiding this comment

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

From the documentation, it looks like a synchronous operation as the command can print the summary

Copy link
Owner Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Sounds good! Is that applicable for these too:

  • update-desktop-database
  • desktop-file-install

?

Copy link
Owner Author

Choose a reason for hiding this comment

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

@RaisinTen Can you help me understand if these are blocking?
How can we confirm the same?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Addressed the rest of the review comments and the PR is ready if we can resolve this comment

2 changes: 2 additions & 0 deletions spec/fixtures/api/gdk-backend-check/groot
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
#!/bin/sh
echo $GDK_BACKEND > /tmp/groot-says
5 changes: 5 additions & 0 deletions spec/fixtures/api/gdk-backend-check/groot.desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
[Desktop Entry]

Choose a reason for hiding this comment

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

are all of these fields necessary? can we trim out some of them?

Copy link
Owner Author

Choose a reason for hiding this comment

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

desktop entry must have a Type and a Name key. Our use case needs Exec and MimeType.
We can remove Terminal, as it doesn't affect our use case, not mandatory as well.

Type=Application
Exec=/tmp/groot
Name=Groot
MimeType=application/x-groot
7 changes: 7 additions & 0 deletions spec/fixtures/api/gdk-backend-check/groot.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?xml version="1.0" encoding="UTF-8"?>
<mime-info xmlns="http://www.freedesktop.org/standards/shared-mime-info">
<mime-type type="application/x-groot">
<comment>Groot File</comment>
<glob weight="100" pattern="*.groot"/>
</mime-type>
</mime-info>
1 change: 1 addition & 0 deletions spec/fixtures/api/gdk-backend-check/hello.groot
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
#!/bin/sh
7 changes: 7 additions & 0 deletions spec/fixtures/api/gdk-backend-check/main.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
const { app, shell } = require('electron');
const path = require('path');

app.whenReady().then(async () => {
await shell.openPath(path.join(__dirname, 'hello.groot'));
app.quit();
});