fix: do not propagate GDK_BACKEND env variable to subproc by bavulapati · Pull Request #1 · bavulapati/electron

bavulapati · 2022-02-03T18:05:11Z

Description of Change

Closes electron#28436.
This PR tries to cherry-pick the commits from an open PR electron#29606 and address the review comments and add tests for the changes.
cc: @dsanders11 @RaisinTen @jviotti

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added

Release Notes

Notes: none

jviotti · 2022-02-07T13:39:41Z

spec-main/chromium-spec.ts

+    await promisifiedTimers.setTimeout(1000);
+
+    let gdkBackend = fs.readFileSync('/tmp/groot-says', 'utf8');
+    gdkBackend = gdkBackend.replace(/(\r\n|\n|\r)/gm, '');


I'm not 100% sure, but I think .trim() will do exactly this

@jviotti I see that now. Used the snippet from Shelley's PR

jviotti · 2022-02-07T13:40:18Z

spec/fixtures/api/gdk-backend-check/hello.groot

@@ -0,0 +1,3 @@
+#!/bin/sh
+
+echo 'I am goot!'


Did you mean groot?

RaisinTen

@bavulapati added some comments. can you also try checking if reverting https://chromium-review.googlesource.com/c/chromium/src/+/2586184 fixes the issue completely? It seems to be the source of the problem but I'm not sure if Weston sessions are even relevant to electron.

In the current state of the pr, changes made to process.env doesn't get propagated to shell.openExternal() which does not seem correct.

shell/common/platform_util_linux.cc

RaisinTen · 2022-02-09T09:22:26Z

spec-main/chromium-spec.ts

+  it('does not propagate GDK_BACKEND', async () => {
+    const appPath = path.join(fixturesPath, 'api', 'gdk-backend-check');
+
+    console.log('gdk_backend before shell open', process.env.GDK_BACKEND);


does electron use console.log() statements in the rest of their tests for general logging purposes? does it show up when you run the test suite or only when the specific test fails?

I think the console.log statements should be removed, maybe they were just for debugging?

I think we can remove the log as it was for debugging purposes.

RaisinTen · 2022-02-09T09:27:32Z

spec/fixtures/api/gdk-backend-check/groot

@@ -0,0 +1,3 @@
+#!/bin/sh
+echo "I'm Groot!"


why are we logging this? the test doesn't seem to validate if this has been printed

I think the echo statements in both scripts should be dropped.

This again is just for debugging purposes.

RaisinTen · 2022-02-09T09:28:05Z

spec/fixtures/api/gdk-backend-check/groot.desktop

@@ -0,0 +1,6 @@
+[Desktop Entry]


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

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.

RaisinTen · 2022-02-09T09:31:05Z

spec/fixtures/api/gdk-backend-check/before-test.sh

@@ -0,0 +1,17 @@
+#!/bin/sh


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?

does having this in the afterTest help?

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

RaisinTen · 2022-02-09T09:32:39Z

spec/fixtures/api/gdk-backend-check/before-test.sh

+cp ./groot.xml ~/.local/share/mime/packages/
+
+# update the MIME database
+update-mime-database ~/.local/share/mime


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

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

https://linux.die.net/man/1/update-mime-database#:~:text=update%2Dmime%2Ddatabase%20is%20a,from%20the%20X%20Desktop%20Group.

Sounds good! Is that applicable for these too:

update-desktop-database

desktop-file-install

?

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

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

dsanders11 · 2022-02-10T12:36:56Z

spec-main/chromium-spec.ts

+  it('does not propagate GDK_BACKEND', async () => {
+    const appPath = path.join(fixturesPath, 'api', 'gdk-backend-check');
+
+    console.log('gdk_backend before shell open', process.env.GDK_BACKEND);


I think the console.log statements should be removed, maybe they were just for debugging?

dsanders11 · 2022-02-10T12:51:16Z

spec/fixtures/api/gdk-backend-check/groot

@@ -0,0 +1,3 @@
+#!/bin/sh
+echo "I'm Groot!"


I think the echo statements in both scripts should be dropped.

RaisinTen · 2022-02-16T09:15:41Z

~~Is 220f582 necessary? Did you face any errors locally in spite of having the await promisifiedTimers.setTimeout(1000); calls?~~

test changes are the same, so I'm fine with it

RaisinTen

lgtm!

fixes electron#28436

bavulapati self-assigned this Feb 3, 2022

jviotti reviewed Feb 7, 2022

View reviewed changes

RaisinTen suggested changes Feb 9, 2022

View reviewed changes

dsanders11 suggested changes Feb 11, 2022

View reviewed changes

RaisinTen approved these changes Feb 16, 2022

View reviewed changes

bavulapati force-pushed the do-not-propagate-x11-backend-env branch 5 times, most recently from 8239336 to 15af724 Compare February 17, 2022 05:36

bavulapati force-pushed the do-not-propagate-x11-backend-env branch 2 times, most recently from 3bd1262 to 20f953c Compare February 24, 2022 08:29

bavulapati force-pushed the do-not-propagate-x11-backend-env branch 2 times, most recently from 42c3150 to e5fc898 Compare March 3, 2022 10:09

fix: do not propagate GDK_BACKEND env variable to subproc

a0e5726

fixes electron#28436

bavulapati force-pushed the do-not-propagate-x11-backend-env branch from e5fc898 to a0e5726 Compare April 28, 2022 16:04

Conversation

bavulapati commented Feb 3, 2022

Description of Change

Checklist

Release Notes

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!

RaisinTen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

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!

RaisinTen commented Feb 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RaisinTen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

RaisinTen commented Feb 16, 2022 •

edited

Loading